Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable nullability in ToolStripItemInternalLayout #7298

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Jun 14, 2022

Proposed changes

  • Enable nullability in ToolStripItemInternalLayout.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner June 14, 2022 04:47
@ghost ghost assigned gpetrou Jun 14, 2022
@ghost ghost added the area: NRT label Jun 14, 2022
@@ -169,7 +166,7 @@ public virtual Size GetPreferredSize(Size constrainingSize)

if (_ownerItem is not null)
{
_lastPreferredSize = _currentLayoutOptions.GetPreferredSizeCore(constrainingSize);
_lastPreferredSize = _currentLayoutOptions!.GetPreferredSizeCore(constrainingSize);
Copy link
Member

Choose a reason for hiding this comment

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

GetLayoutData() must be annotated with MemeberNotNull. Though I'm not sure, if the analyzer will see it.

_currentLayoutOptions will always be initialised if EnsureLayout() is called, because it will get called at least once (for _layoutData is null) and, in turn, it'll invoke PerformLayout() that'll unconditionally invoke GetLayoutData(). So EnsureLayout() must be annotated with MemeberNotNull(_currentLayoutOptions).

Please add a comment above EnsureLayout explaining this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, adding MemberNotNull for _currentLayoutOptions will not work, so I added a comment instead.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please elaborate why it won't work?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I see:
image
Perhaps it is because I still use VS 2022 17.2 version? I see lots of other warnings, but the build still works.

Copy link
Member

Choose a reason for hiding this comment

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

👍

We still need to do this:

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.ToolStripItemInternalLayout.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.ToolStripItemInternalLayout.cs
index 6019222bb..469a5a04c 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.ToolStripItemInternalLayout.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.ToolStripItemInternalLayout.cs
@@ -135,17 +135,15 @@ namespace System.Windows.Forms
             }
 
             [MemberNotNull(nameof(_layoutData))]
-            private bool EnsureLayout()
+            private void EnsureLayout()
             {
                 if (_layoutData is null || _parentLayoutData is null || !_parentLayoutData.IsCurrent(ParentInternal))
                 {
                     PerformLayout();
-                    return true;
                 }
-
-                return false;
             }
 
+            [MemberNotNull(nameof(_currentLayoutOptions))]
             private ButtonBaseAdapter.LayoutData GetLayoutData()
             {
                 _currentLayoutOptions = CommonLayoutOptions();
@@ -181,6 +179,7 @@ namespace System.Windows.Forms
             }
 
             [MemberNotNull(nameof(_layoutData))]
+            [MemberNotNull(nameof(_currentLayoutOptions))]
             internal void PerformLayout()
             {
                 _layoutData = GetLayoutData();

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label Jun 15, 2022
@gpetrou gpetrou force-pushed the ToolStripItemInternalLayout branch from 8df1e42 to d6bcf36 Compare June 15, 2022 04:46
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jun 15, 2022
@gpetrou gpetrou removed their assignment Jun 15, 2022
@ghost ghost assigned gpetrou Jun 15, 2022
@gpetrou gpetrou force-pushed the ToolStripItemInternalLayout branch from d6bcf36 to 096472f Compare June 15, 2022 04:49
@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jun 15, 2022
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jun 16, 2022
@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jun 16, 2022
@gpetrou gpetrou force-pushed the ToolStripItemInternalLayout branch from 096472f to 4aacaab Compare June 16, 2022 11:45
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jun 16, 2022
@gpetrou gpetrou force-pushed the ToolStripItemInternalLayout branch from 4aacaab to 4d745ee Compare June 16, 2022 18:00
@gpetrou gpetrou force-pushed the ToolStripItemInternalLayout branch from 4d745ee to 915add7 Compare June 16, 2022 19:08
@RussKie RussKie merged commit 35c2cf7 into dotnet:main Jun 17, 2022
@ghost ghost added this to the 7.0 Preview6 milestone Jun 17, 2022
@RussKie
Copy link
Member

RussKie commented Jun 17, 2022

Thank you

@gpetrou gpetrou deleted the ToolStripItemInternalLayout branch June 17, 2022 10:25
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 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.

2 participants