Skip to content

Enable nullability in Link class#7992

Merged
RussKie merged 2 commits intodotnet:mainfrom
gpetrou:Link
Oct 26, 2022
Merged

Enable nullability in Link class#7992
RussKie merged 2 commits intodotnet:mainfrom
gpetrou:Link

Conversation

@gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Oct 22, 2022

Proposed changes

  • Enable nullability in Link class.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner October 22, 2022 10:51
@ghost ghost assigned gpetrou Oct 22, 2022
@ghost ghost added the area: NRT label Oct 22, 2022
Region region = _owningLink.VisualRegion;
using Graphics graphics = Graphics.FromHwnd(_owningLink.Owner.Handle);
Region? region = _owningLink.VisualRegion;
using Graphics graphics = Graphics.FromHwnd(_owningLink.Owner?.Handle ?? IntPtr.Zero);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you forced to pass IntPtr.Zero here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if Owner is null (which it can be) then we have no handle.
I would, however, move the check up to line:31 - if we don't have a valid handle, then we create Graphics object for the desktop area, which feels wrong. In this case it probably better to return an empty rectangle.

@Tanya-Solyanik @dmitrii-drobotov what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know owner can be null. I mean, is null not accepted for this call? what is forcing to explicit pass of IntPtr.Zero?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your questions... Please help me.
IntPtr is a struct, Graphics.FromHwnd(IntPtr) doesn't accept null.

Copy link
Member

@dreddy-work dreddy-work Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake. i thought that method is accepting null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if Owner is null (which it can be) then we have no handle. I would, however, move the check up to line:31 - if we don't have a valid handle, then we create Graphics object for the desktop area, which feels wrong. In this case it probably better to return an empty rectangle.

@Tanya-Solyanik @dmitrii-drobotov what do you think?

I agree that returning an empty rectangle is better in this case. But this is probably an extremely rare scenario since _owningLink.Owner should be the same object as _owningLinkLabel:

internal LinkAccessibleObject AccessibleObject
=> _accessibleObject ??= Owner is not null ? new(this, Owner) : null;

public LinkAccessibleObject(Link link, LinkLabel owner)
{
_owningLink = link.OrThrowIfNull();
_owningLinkLabel = owner.OrThrowIfNull();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@dreddy-work dreddy-work added the waiting-author-feedback The team requires more information from the author label Oct 22, 2022
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 modulo the discussion in LAO.

@RussKie RussKie added waiting-review This item is waiting on review by one or more members of team and removed waiting-author-feedback The team requires more information from the author labels Oct 24, 2022
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Oct 26, 2022
@RussKie RussKie merged commit 358a2f0 into dotnet:main Oct 26, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Oct 26, 2022
@RussKie
Copy link
Contributor

RussKie commented Oct 26, 2022

Thank you

@gpetrou gpetrou deleted the Link branch October 26, 2022 16:05
@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants