Enable nullability in DesignerUtils and replace ArrayList with List #9381

halgab · 2023-06-27T20:47:29Z

Proposed changes

Enable nullability in internal helper DesignerUtils.
Replace a few ArrayLists with List<T>s (contributes to Convert ArrayList usage to List<T> where possible #8140). This change should make it easier to annotate all the files touched by this refactoring

Microsoft Reviewers: Open in CodeFlow

elachlan · 2023-06-28T23:13:12Z

Why the change away from IComponent to Control?

halgab · 2023-06-29T05:42:59Z

Because we keep casting the IComponents to Control, to the point where they can't not be Control instances. So I figured it would be simpler to be more specific and stop casting around all the time.

elachlan · 2023-06-29T08:45:42Z

Awesome. Sounds good.

halgab

I think I might have been a bit too optimistic. I'll rollback to a less ambitious version. I'll put this PR back to draft in the meantime

halgab · 2023-06-30T07:30:26Z

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/DesignerUtils.cs

        {
            return;
        }

-        foreach (IComponent childComp in designer.AssociatedComponents)
+        foreach (T childComp in designer.AssociatedComponents)


I think that if this doesn't hold, we're going to break later, but I'm not 100% sure

dreddy-work

Logically looks good but hard to validate these design scenarios at runtime.

Any possibility to add some unit tests around these changes?

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/DropSourceBehavior.cs

halgab · 2023-07-05T18:28:47Z

I'm usually having a pretty hard time running tests locally, but sure, I'll have a look. I understand your concerns

dreddy-work · 2023-07-06T22:21:11Z

I'm usually having a pretty hard time running tests locally, but sure, I'll have a look. I understand your concerns

Let me know what kind of issue you see when running tests locally. If you start VS using start-vs.cmd from the repo root, it should load everything needed.

dreddy-work · 2023-07-21T20:07:09Z

We will take this early in 9.0

Epica3055 · 2023-09-14T02:55:22Z

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/DropSourceBehavior.cs

@@ -216,16 +216,16 @@ private void SetDesignerHost(Control c)
            SetDesignerHost(control);
        }

-        if (c.Site is not null && !(c.Site is INestedSite) && destHost is not null)
+        if (c.Site is not null && !(c.Site is INestedSite) && _destHost is not null)


For readability, maybe we should rename c to control

This file is not annotated as part of this PR. Do you still want me to address this here, or should I wait for a dedicated PR?

I think maybe you can do it in this PR, since it doesn't require many changes.

That's done. This file could use some cleanup on other aspects, but I'll leave that for a subsequent PR

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/DropSourceBehavior.cs

KlausLoeffelmann

I have only started to look through this, but I am not at all feeling comfortable taking this.

Why can we assume, that the behaviors/designers only process Controls and no (I)Components or other objects?

I know in the Out-Of-Proc Designer, for example, this would not work.

But maybe I am missing some additional information or have not read previous discussions, so I am happy to hear more about this!

Thx!

KlausLoeffelmann · 2023-10-10T22:09:17Z

...m.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/ContainerSelectorBehavior.cs

@@ -205,7 +205,7 @@ private void StartDragOperation(Point initialMouseLocation)

        //must identify a required parent to avoid dragging mixes of children
        Control requiredParent = _containerControl.Parent;
-        List<IComponent> dragControls = new();
+        List<Control> dragControls = new();


Why do we want to change this from IComponent to Control?

KlausLoeffelmann · 2023-10-10T22:11:13Z

...ystem.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/DragAssistanceManager.cs

    {
    }

    /// <summary>
    ///  Internal constructor that takes the service provider, the list of dragComponents, and a boolean
    ///  indicating that we are resizing.
    /// </summary>
-    internal DragAssistanceManager(IServiceProvider serviceProvider, List<IComponent> dragComponents, bool resizing) : this(serviceProvider, null, dragComponents, null, resizing, false)
+    internal DragAssistanceManager(IServiceProvider serviceProvider, IReadOnlyList<IComponent> dragComponents, bool resizing) : this(serviceProvider, null, dragComponents, null, resizing, false)


Would this not break backwards-compatibility in certain custom Designer scenarios? What makes us sure, that it doesn't?

KlausLoeffelmann · 2023-10-10T22:24:40Z

...rms.Design/src/System/Windows/Forms/Design/Behavior/DropSourceBehavior.BehaviorDataObject.cs

@@ -16,7 +15,7 @@ internal class BehaviorDataObject : DataObject
    {
        private readonly DropSourceBehavior _sourceBehavior;

-        public BehaviorDataObject(ICollection dragComponents, Control source, DropSourceBehavior sourceBehavior) : base()
+        public BehaviorDataObject(List<Control> dragComponents, Control source, DropSourceBehavior sourceBehavior) : base()


How can we be sure only be getting controls at this point?

RussKie · 2023-10-11T06:34:40Z

Enable nullability in internal helper DesignerUtils.

Replace a few ArrayLists with List<T>s (contributes to Convert ArrayList usage to List<T> where possible #8140). This change should make it easier to annotate all the files touched by this refactoring

These are two different kind of changes that must not be mixed together. The latter one has much higher risk of introducing a breaking change.

halgab · 2023-10-11T09:05:19Z

Yes, I get now that my attempt at a refactoring here got out of hand and shouldn't be mixed with the nullability here. I'll revisit this PR. My feeling is that replacing ArrayLists with Lists help with the nullability annotations, so if that's okay with the team, I'll attempt to address the second bullet point in a first PR (staying conservative this time, again sorry for the mess here), and then follow-up with a NRT annotation PR.

halgab · 2023-10-11T09:25:11Z

I've just opened #10091 that focus solely on the ArrayList removal. This should be easy to review

halgab · 2023-10-27T20:14:41Z

Closed in favor of #10091, #10092 and #10183

ghost assigned halgab Jun 27, 2023

ghost added the area: NRT label Jun 27, 2023

halgab force-pushed the DesignerUtils-nullable branch from b1cb929 to 8c1e1c0 Compare June 27, 2023 21:06

halgab marked this pull request as ready for review June 27, 2023 21:24

halgab requested a review from a team as a code owner June 27, 2023 21:24

halgab commented Jun 30, 2023

View reviewed changes

halgab marked this pull request as draft June 30, 2023 07:32

ghost added the draft draft PR label Jun 30, 2023

halgab force-pushed the DesignerUtils-nullable branch from 1511576 to 8a7de66 Compare June 30, 2023 19:52

halgab marked this pull request as ready for review June 30, 2023 21:32

dreddy-work marked this pull request as draft June 30, 2023 21:46

halgab marked this pull request as ready for review July 1, 2023 12:19

ghost removed the draft draft PR label Jul 1, 2023

Tanya-Solyanik requested review from LeafShi1, SimonZhao888 and Epica3055 July 3, 2023 17:37

LeafShi1 previously approved these changes Jul 5, 2023

View reviewed changes

dreddy-work requested changes Jul 5, 2023

View reviewed changes

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/Behavior/DropSourceBehavior.cs Outdated Show resolved Hide resolved

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Jul 5, 2023

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 5, 2023

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Jul 6, 2023

halgab dismissed LeafShi1’s stale review via 66fad1c July 11, 2023 01:20

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 11, 2023

dreddy-work added the Net9.0 label Jul 21, 2023

halgab force-pushed the DesignerUtils-nullable branch from 86ac0bc to 77459b8 Compare September 4, 2023 12:24

Epica3055 reviewed Sep 14, 2023

View reviewed changes

halgab force-pushed the DesignerUtils-nullable branch from eb5d403 to c86efc3 Compare September 20, 2023 09:47

halgab added 7 commits September 28, 2023 09:40

annotate DesignerUtils

33972e7

safer refactoring

b5ad910

fix names

6e98f24

_dragObjects can stay on as a List

9bd561e

post-merge follow-ups

c494ef7

change several variable names in DropSourceBehavior

9254810

post-rebase follow-ups

906cd5f

halgab force-pushed the DesignerUtils-nullable branch from c86efc3 to 906cd5f Compare September 28, 2023 07:41

halgab requested a review from dreddy-work October 7, 2023 12:05

KlausLoeffelmann requested changes Oct 10, 2023

View reviewed changes

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Oct 10, 2023

Tanya-Solyanik removed the Net9.0 label Oct 10, 2023

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Oct 11, 2023

halgab mentioned this pull request Oct 11, 2023

Replace ArrayList with List in DesignerUtils #10091

Merged

halgab mentioned this pull request Oct 11, 2023

DesignerUtils refactorings #10092

Merged

halgab mentioned this pull request Oct 24, 2023

Enable nullability in DesignerUtils #10183

Merged

halgab closed this Oct 27, 2023

halgab deleted the DesignerUtils-nullable branch October 27, 2023 20:14

ghost locked as resolved and limited conversation to collaborators Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable nullability in DesignerUtils and replace ArrayList with List #9381

Enable nullability in DesignerUtils and replace ArrayList with List #9381

halgab commented Jun 27, 2023 •

edited

Loading

elachlan commented Jun 28, 2023

halgab commented Jun 29, 2023

elachlan commented Jun 29, 2023

halgab left a comment

halgab Jun 30, 2023

dreddy-work left a comment

halgab commented Jul 5, 2023

dreddy-work commented Jul 6, 2023

dreddy-work commented Jul 21, 2023

Epica3055 Sep 14, 2023

halgab Sep 14, 2023

Epica3055 Sep 15, 2023

halgab Sep 15, 2023

KlausLoeffelmann left a comment

KlausLoeffelmann Oct 10, 2023

KlausLoeffelmann Oct 10, 2023

KlausLoeffelmann Oct 10, 2023

RussKie commented Oct 11, 2023

halgab commented Oct 11, 2023

halgab commented Oct 11, 2023

halgab commented Oct 27, 2023

Enable nullability in DesignerUtils and replace ArrayList with List #9381

Enable nullability in DesignerUtils and replace ArrayList with List #9381

Conversation

halgab commented Jun 27, 2023 • edited Loading

Proposed changes

Microsoft Reviewers: Open in CodeFlow

elachlan commented Jun 28, 2023

halgab commented Jun 29, 2023

elachlan commented Jun 29, 2023

halgab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreddy-work left a comment

Choose a reason for hiding this comment

halgab commented Jul 5, 2023

dreddy-work commented Jul 6, 2023

dreddy-work commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlausLoeffelmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Oct 11, 2023

halgab commented Oct 11, 2023

halgab commented Oct 11, 2023

halgab commented Oct 27, 2023

halgab commented Jun 27, 2023 •

edited

Loading