Refactored ContentAlignmentEditor. It should work as DockEditor by NikitaSemenovAkvelon · Pull Request #7831 · dotnet/winforms

NikitaSemenovAkvelon · 2022-09-23T10:59:18Z

Fixes #7800

Proposed changes

Refactored ContentAlignmentEditor.ContentUI and DockEditor.DockUI.
Moved common logic to SelectionPanelBase.
Reworked ContentAlignmentEditor.ContentUI, currently it works in the same way as DockEditor.DockUI.

Customer Impact

Narrator announcements will be more transparent for users.

Regression?

No

Risk

Minimal

Screenshots

Before

ImageAlign, 10 of 20, expanded,
Expanded
Alignment Picker, Middle Center, radio button, selected,
Middle Center, radio button, non-selected,
Middle Right, radio button, selected,
Middle Right, radio button, non-selected,
Bottom Left, radio button, selected,
Bottom Left, radio button, non-selected,
Bottom Middle, radio button, selected,
Bottom Middle, radio button, non-selected,
Bottom Right, radio button, selected,
Bottom Right, radio button, non-selected,
Middle Right, radio button, selected,
Middle Right, radio button, non-selected,
Middle Center, radio button, selected,
ImageAlign, 10 of 20, collapsed,
MiddleCenter selected.

After

ImageAlign, 10 of 20, expanded,
Expanded
Alignment Picker, Middle Center, radio button, selected,
Middle Right, radio button, non-selected,
Bottom Left, radio button, non-selected,
Bottom Middle, radio button, non-selected,
Middle Center, radio button, selected,
Top Center, radio button, non-selected,
ImageAlign, 10 of 20, collapsed,
Open, button,
Open, button,
Expanded
Alignment Picker, Top Center, radio button, selected,
Middle Center, radio button, non-selected,
Bottom Middle, radio button, non-selected,
ImageAlign, 10 of 20, collapsed,

Test methodology

Manually
Accessibility tools
CTI

Accessibility testing

Narrator
Inspect

Test environment(s)

.NET SDK:
Version: 8.0.100-alpha.1.22423.9
Commit: b9635390c8

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22621

Microsoft Reviewers: Open in CodeFlow

dmitrii-drobotov

Nice refactoring!

Tanya-Solyanik · 2022-09-23T19:14:00Z

Build failure:

(NETCORE_ENGINEERING_TELEMETRY=NativeToolsBootstrap) Unable to find directory for cmake 3.21.0; please make sure the tool is installed on this image.

dreddy-work · 2022-09-23T19:22:40Z

It seems X86 machine pools have this problem. From: Tanya Solyanik ***@***.***> Sent: Friday, September 23, 2022 12:14 PM To: dotnet/winforms ***@***.***> Cc: Devendar Reddy A ***@***.***>; Review requested ***@***.***> Subject: Re: [dotnet/winforms] Refactored ContentAlignmentEditor. It should work as DockEditor (PR #7831) Build failure: (NETCORE_ENGINEERING_TELEMETRY=NativeToolsBootstrap) Unable to find directory for cmake 3.21.0; please make sure the tool is installed on this image. - Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fwinforms%2Fpull%2F7831%23issuecomment-1256582166&data=05%7C01%7Cdreddy%40microsoft.com%7C85359506fd014deddd5a08da9d97cc7a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637995572542089383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=BG4QVI6NLh3H6MkWqkLBFrbvAbcNSKy7f%2FU%2F6jS0%2BEs%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAI2BRWYDOP2UWX52EUKG6Z3V7X6QHANCNFSM6AAAAAAQT35HCE&data=05%7C01%7Cdreddy%40microsoft.com%7C85359506fd014deddd5a08da9d97cc7a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637995572542089383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Lw3MJ5mPXgUaeCI4W3C6y0tjq1%2BqGkj3OYhtRoWLKek%3D&reserved=0>. You are receiving this because your review was requested.Message ID: ***@***.******@***.***>>

Tanya-Solyanik

Looks good, I added some nits. Please send to testing.

Tanya-Solyanik

Please see my comment to confirm that Focus event is raised.

Tanya-Solyanik

Looks good

elachlan · 2022-09-29T00:41:31Z

        ///  Control we use to provide the content alignment UI.
        /// </summary>
-        private class ContentUI : Control
+        private class ContentUI : SelectionPanelBase


Is this inherited from anywhere internally? Maybe mark it as sealed if not?

I agree with you. It makes sense in this case.

@RussKie Should this have been picked up by the analyzer?

I vaguely remember there were discussions in dotnet/runtime about creating an analyzer for this, but I don't know whether this has been done and whether this analyzer is available now. We can certainly enable the analyzer, if it's now available.

I believe its CA1852
https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/#analyzers

Nice, thank you.

We can certainly turn it on, though it doesn't appear to be documented yet. https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1812 is another one to consider turning on.
It could be great to turn these on before the change to confirm.

The configs are stored here: https://github.com/dotnet/winforms/blob/main/eng/CodeAnalysis.src.globalconfig.

Created at #7879

Awesome, thank you!

elachlan · 2022-09-29T00:43:52Z

        ///  User Interface for the DockEditor.
        /// </summary>
-        private class DockUI : Control
+        private class DockUI : SelectionPanelBase


Same here, not inherited mark as sealed.

Olina-Zhang · 2022-09-29T06:45:19Z

Tested this PR fixing with no new issue found.

…et#7831) Changes: Refactored ContentAlignmentEditor.ContentUI and DockEditor.DockUI. Moved common logic to SelectionPanelBase. Reworked ContentAlignmentEditor.ContentUI, currently it works in the same way as DockEditor.DockUI. Customer Impact: Narrator announcements will be more transparent for users.

NikitaSemenovAkvelon requested a review from a team as a code owner September 23, 2022 10:59

ghost assigned NikitaSemenovAkvelon Sep 23, 2022

NikitaSemenovAkvelon requested review from Tanya-Solyanik, dkazennov, dmitrii-drobotov and dreddy-work September 23, 2022 11:00

dmitrii-drobotov reviewed Sep 23, 2022

View reviewed changes

Comment thread src/System.Windows.Forms.Design/src/System/Drawing/Design/SelectionPanelBase.cs Outdated

Comment thread src/System.Windows.Forms.Design/src/System/Drawing/Design/ContentAlignmentEditor.ContentUI.cs

Tanya-Solyanik requested a review from v-elnovikova September 23, 2022 19:01

Tanya-Solyanik reviewed Sep 23, 2022

View reviewed changes

Tanya-Solyanik added waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) waiting-author-feedback The team requires more information from the author labels Sep 23, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Sep 26, 2022

NikitaSemenovAkvelon force-pushed the Issue_7800-Refactoring_ContentAligmentEditor branch from ea70fa4 to f67ac24 Compare September 26, 2022 06:21

Tanya-Solyanik reviewed Sep 27, 2022

View reviewed changes

Comment thread src/System.Windows.Forms.Design/src/System/Drawing/Design/ContentAlignmentEditor.ContentUI.cs Outdated

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Sep 27, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Sep 27, 2022

NikitaSemenovAkvelon force-pushed the Issue_7800-Refactoring_ContentAligmentEditor branch from f67ac24 to b206590 Compare September 27, 2022 11:23

Tanya-Solyanik previously approved these changes Sep 28, 2022

View reviewed changes

elachlan reviewed Sep 29, 2022

View reviewed changes

Olina-Zhang removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 29, 2022

dmitrii-drobotov previously approved these changes Sep 29, 2022

View reviewed changes

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Sep 29, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Sep 30, 2022

Refactored ContentAlignmentEditor. It should work as DockEditor

d250500

NikitaSemenovAkvelon dismissed stale reviews from dmitrii-drobotov and Tanya-Solyanik via d250500 September 30, 2022 04:52

NikitaSemenovAkvelon force-pushed the Issue_7800-Refactoring_ContentAligmentEditor branch from b206590 to d250500 Compare September 30, 2022 04:52

dmitrii-drobotov approved these changes Sep 30, 2022

View reviewed changes

RussKie mentioned this pull request Oct 4, 2022

Enable CA1852/CA1812 Analyzers #7881

Merged

Tanya-Solyanik merged commit 70364ba into dotnet:main Oct 5, 2022

ghost added this to the 8.0 Preview1 milestone Oct 5, 2022

ghost locked as resolved and limited conversation to collaborators Nov 4, 2022

Conversation

NikitaSemenovAkvelon commented Sep 23, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Before

After

Test methodology

Accessibility testing

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

Uh oh!

dmitrii-drobotov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik commented Sep 23, 2022

Uh oh!

dreddy-work commented Sep 23, 2022 via email

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olina-Zhang commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

NikitaSemenovAkvelon commented Sep 23, 2022 •

edited by ghost

Loading