Changes to fix issues in `DemoConsole` by elachlan · Pull Request #9419 · dotnet/winforms

elachlan · 2023-07-04T07:07:46Z

Fixes #9412
Fixes #9416
Fixes #9420
Fixes #9389

Microsoft Reviewers: Open in CodeFlow

…, and DemoConsole

elachlan · 2023-07-04T07:08:14Z

CC: @Tanya-Solyanik

elachlan · 2023-07-04T07:10:09Z

@MelonWang1 can you please test this?

MelonWang1 · 2023-07-04T07:51:18Z

@elachlan The two issues have been fixed, PictureBox can resize and add image successfully, and the most controls delete, undo, redo function use well.

fix.mp4

Except SplitContainer control cannot be deleted.

splitcontainer.mp4

elachlan · 2023-07-04T08:19:42Z

Could you please create a separate issue for the SplitContainer not being able to be deleted?

elachlan · 2023-07-04T08:38:34Z

This is the exception when you try and delete the splitcontainer. I'll look into it.

MelonWang1 · 2023-07-04T09:00:12Z

@elachlan Verified new commit, the SplitContainer control can delete, but an exception occurs when delete SplitContainer.Panel.

splitpanel.mp4

MelonWang1 · 2023-07-04T09:42:59Z

@elachlan I'm sorry I did not clearly description this. The SplitContainer.Panel should not be removed when click delete keyboard. In winforms .NET framework application, the behavior is the SplitContainer.Panel cannot be removed.

framework.mp4

elachlan · 2023-07-04T09:46:24Z

I imagine we just need to not process it when a split panel is selected.

dmitrii-drobotov · 2023-07-04T14:35:00Z

Great job @elachlan! Regarding SplitterPanel delete — I think it's a bug on application side. In VS Designer delete command is disabled:

But in DemoConsole app commands are invoked without status check:

winforms/src/System.Windows.Forms/tests/IntegrationTests/DesignSurface/DesignSurfaceExt/DesignSurfaceExt.cs

Lines 376 to 393 in 918e346

    
           IMenuCommandService ims = GetService(typeof(IMenuCommandService)) as IMenuCommandService; 
        
           try 
        
           { 
        
               switch (command.ToUpper()) 
        
               { 
        
                   case "CUT": 
        
                       ims.GlobalInvoke(StandardCommands.Cut); 
        
                       break; 
        
                   case "COPY": 
        
                       ims.GlobalInvoke(StandardCommands.Copy); 
        
                       break; 
        
                   case "PASTE": 
        
                       ims.GlobalInvoke(StandardCommands.Paste); 
        
                       break; 
        
                   case "DELETE": 
        
                       ims.GlobalInvoke(StandardCommands.Delete); 
        
                       break;

We can update this part to include a condition for MenuCommand.Enabled, then the delete command won't do anything for splitter panel because it would have Enabled as false:

CommandID? commandId = command.ToUpper() switch
{
    "CUT" => StandardCommands.Cut,
    "COPY" => StandardCommands.Copy,
    "PASTE" => StandardCommands.Paste,
    "DELETE" => StandardCommands.Delete,
    _ => null,
};

if (commandId is not null)
{
    MenuCommand? menuCommand = ims?.FindCommand(commandId);
    if (menuCommand?.Enabled is true)
    {
        menuCommand.Invoke();
    }
}

elachlan · 2023-07-05T03:13:18Z

@Nora-Zhou01 can you also verify #9389 is fixed please?

Nora-Zhou01 · 2023-07-05T06:43:35Z

@elachlan 9389 This issue is fixed and the item is displayed again after adding an item to ComboBox in DemoConsole application.

9419.mp4

elachlan · 2023-07-05T06:51:02Z

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

    public void Remove(Glyph value)
    {
-        List.Remove(value);
+        if (List.Contains(value))


This was because remove was throwing exceptions. But the exceptions were caught further up. Up to the team if they are happy with the exception.

@elachlan, Did you find a scenario where we are trying to call remove for a glyph that wasn't added to collection?

This change should be good.

Yes, it would have been when deleting and then performing an "undo" (I think). Do you want a debug.assert for when the value isn't in the list?

If it is a valid scenario, i would put a comment about it. Debug assert will be annoying for a valid scenario.

Added comment.

dreddy-work

thanks @elachlan for taking a look at these. Added comments and questions.

dreddy-work · 2023-07-05T16:43:23Z

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

            }

-            return _mergedMetadata.GetEnumerator();
+            return _mergedMetadata?.GetEnumerator();


can you enable nullability on this file or is it a big change?

Its a big change I think. I'd rather have it done in another PR.

@halgab / @gpetrou did either of you want to tackle null annotating ResourceCodeDomSerializer.SerializationResourceManager.cs?

I was waiting for #9099 to be merged to tackle CodeDomSerializer and then derived classes, but I can take care of this one file after this PR merges for sure

Here it is: #9457

dreddy-work · 2023-07-05T16:46:12Z

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

    public void Remove(Glyph value)
    {
-        List.Remove(value);
+        if (List.Contains(value))


@elachlan, Did you find a scenario where we are trying to call remove for a glyph that wasn't added to collection?

This change should be good.

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

....Forms.Design/src/System/Windows/Forms/Design/StringCollectionEditor.StringCollectionForm.cs

RussKie · 2023-07-06T02:25:17Z

src/System.Windows.Forms/src/System/Windows/Forms/ComboBox.ObjectCollection.Entry.cs

+
+            public override string? ToString()
+            {
+                return Item.ToString();


❗ This can NRE

Should it be Item.ToString()!?

wait, Item.ToString() ?? string.Empty;

This has nullable types enabled and object is marked as non-nullable. So it shouldn't NRE?

This has nullable types enabled and object is marked as non-nullable. So it shouldn't NRE?

Item = null! 🤔

If we know for sure Item can't be a null then I retract my comment :)

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

....Forms.Design/src/System/Windows/Forms/Design/StringCollectionEditor.StringCollectionForm.cs

…n/Behavior/GlyphCollection.cs Co-authored-by: Devendar Reddy Adulla <dreddy@microsoft.com>

…ix-DemoConsole

elachlan · 2023-07-06T21:27:15Z

I've done some refactoring based on the feedback. Hopefully that covers what people were worried about.

Changes to fix GetMetadataEnumerator, GetResourceSet, GlyphCollection…

9e87828

…, and DemoConsole

elachlan requested a review from a team as a code owner July 4, 2023 07:07

ghost assigned elachlan Jul 4, 2023

Fix deleting SplitContainer

54577c1

Fix SplitContainer.Panel delete

974cc44

elachlan added 2 commits July 5, 2023 08:54

Fix SplitterPanel delete

aa4750f

Fix Error adding Items of ComboBox in DemoConsole

795876d

elachlan mentioned this pull request Jul 5, 2023

Error adding Items of ComboBox in DemoConsole application #9420

Closed

Fix StringCollectionEditor text not being set

901eb7b

elachlan commented Jul 5, 2023

View reviewed changes

dmitrii-drobotov previously approved these changes Jul 5, 2023

View reviewed changes

dreddy-work reviewed Jul 5, 2023

View reviewed changes

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

Tanya-Solyanik previously approved these changes Jul 5, 2023

View reviewed changes

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

RussKie reviewed Jul 6, 2023

View reviewed changes

Null annotate StringCollectionForm and changes from review

2999908

elachlan dismissed stale reviews from Tanya-Solyanik and dmitrii-drobotov via 2999908 July 6, 2023 03:05

Change from review

88b2dbe

dreddy-work reviewed Jul 6, 2023

View reviewed changes

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

....Forms.Design/src/System/Windows/Forms/Design/StringCollectionEditor.StringCollectionForm.cs Outdated Show resolved Hide resolved

elachlan and others added 4 commits July 7, 2023 06:35

Update src/System.Windows.Forms.Design/src/System/Windows/Forms/Desig…

8c949a9

…n/Behavior/GlyphCollection.cs Co-authored-by: Devendar Reddy Adulla <dreddy@microsoft.com>

Use string.join when loading _textEntry.Text

a972eee

Merge branch 'Fix-DemoConsole' of github.com:elachlan/winforms into F…

92c3e73

…ix-DemoConsole

refactor StringCollectionForm OKButton_click to improve readability

007f35f

dreddy-work approved these changes Jul 6, 2023

View reviewed changes

dreddy-work added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jul 6, 2023

dmitrii-drobotov approved these changes Jul 7, 2023

View reviewed changes

dreddy-work merged commit 8a55875 into dotnet:main Jul 7, 2023

ghost added this to the 8.0 Preview7 milestone Jul 7, 2023

ghost removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jul 7, 2023

elachlan deleted the Fix-DemoConsole branch July 21, 2023 23:40

ghost locked as resolved and limited conversation to collaborators Aug 21, 2023

Conversation

elachlan commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

elachlan commented Jul 4, 2023

Uh oh!

elachlan commented Jul 4, 2023

Uh oh!

MelonWang1 commented Jul 4, 2023

Uh oh!

elachlan commented Jul 4, 2023

Uh oh!

elachlan commented Jul 4, 2023

Uh oh!

MelonWang1 commented Jul 4, 2023

Uh oh!

MelonWang1 commented Jul 4, 2023

Uh oh!

elachlan commented Jul 4, 2023

Uh oh!

dmitrii-drobotov commented Jul 4, 2023

Uh oh!

elachlan commented Jul 5, 2023

Uh oh!

Nora-Zhou01 commented Jul 5, 2023

Uh oh!

elachlan Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dreddy-work 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elachlan Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

elachlan commented Jul 6, 2023

Uh oh!

Reviewers

Assignees

elachlan commented Jul 4, 2023 •

edited

Loading

elachlan Jul 5, 2023 •

edited

Loading

RussKie Jul 6, 2023 •

edited

Loading

elachlan Jul 6, 2023 •

edited

Loading