Port IconEditor; add test

[zsd4yr]

Port Icon Editor for 3.1

Closes #2020

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

Merging #2089 into release/3.1 will decrease coverage by 0.00262%.
The diff coverage is 8.43373%.

@@                  Coverage Diff                  @@
##           release/3.1       #2089         +/-   ##
=====================================================
- Coverage     26.51476%   26.51213%   -0.00263%     
=====================================================
  Files              804         806          +2     
  Lines           268062      268164        +102     
  Branches         38066       38079         +13     
=====================================================
+ Hits             71076       71096         +20     
- Misses          191907      191991         +84     
+ Partials          5079        5077          -2

Flag	Coverage Δ
#Debug	26.51213% <8.43373%> (-0.00263%)	⬇️
#production	26.51213% <8.43373%> (-0.00263%)	⬇️
#test	?

[RussKie]

We need to add a type forwarding to System.Drawing.Design

[weltkante]

Anyone started coordinating adding TypeDescriptor annotations to System.Drawing classes? As noted in #1119, #1120, #1121 ported editors for System.Drawing won't work otherwise, you'll have to discuss whether the correct place is to add the missing Editor attributes in CoreFX similar to what is already done for TypeConverter attributes, or if WinForms needs to have its own TypeDescriptor extension logic. CoreFX already reserved a hashtable for editor attributes but it currently is empty. I'm not sure if the infrastructure allows two extensions on the same type though, this needs extra care to handle correctly so your extension doesn't accidently revert CoreFX extensions!

[RussKie]

This is getting addresses via type forwarding - https://github.com/dotnet/winforms/blob/release/3.1/src/System.Design/src/System.Design.Forwards.cs

[weltkante]

@RussKie no, type forwarding just affects type resolution not association of the Editor to actual editable classes/properties, if you don't know which type to look for the type forwarding doesn't help. CoreFX removed [Editor] attributes from classes and properties, these need to be restored, either on the class or in some TypeDescriptor extension logic (see links in my previous comment). The question is where it is done, as I understood System.Drawing doesn't want a dependency on WinForms.

For example the Editor attribute on the Icon class in Desktop Framework is not present in CoreFX - it just has a TypeConverter but no Editor. This means TypeDescriptor.GetEditor will return null.

Like said elsewhere it would be useful to add unit tests checking e.g. TypeDescriptor.GetEditor(typeof(Icon)) is IconEditor to verify the class is linked properly. Appropriate tests could be added whenever a missing editor is ported, would detect future regressions.

Without this the ported editor may work in the VS Designer (if VS has its own TypeDescriptor overrides) but it won't work for e.g. PropertyGrid placed in user applications like in the three linked issues. IMHO if the ported editor is only for usage in VS it should be defined there and not in the WinForms repository, if its in WinForms I'd expect it to be usable in standalone PropertyGrid.

[zsd4yr]

@weltkante there are really two mechanisms to solve the issue of resolving an editor for a given type

1. The editor attribute; and
2. The intrinsic table given to TypeDescriptor

The goal in ultimately to have TypeDescriptor.GetEditor(type) return the desired editor.

So, with both of these mechanisms, there are really 2 equivalence classes of scenarios we care about:

1. If the type is owned by Windows Forms, then an editor attribute can safely be added to that type associating it with an editor. Examples:

   • winforms/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripStatusLabelBorderSides.cs

     Lines 12 to 14 in 0924da5

     	[Editor("System.Windows.Forms.Design.BorderSidesEditor, " + AssemblyRef.SystemDesign, typeof(UITypeEditor))]
     	[Flags]
     	public enum ToolStripStatusLabelBorderSides

   • winforms/src/System.Windows.Forms/src/System/Windows/Forms/ListControl.cs

     Lines 203 to 205 in d478d67

     	[Editor("System.Windows.Forms.Design.FormatStringEditor, " + AssemblyRef.SystemDesign, typeof(UITypeEditor))]
     	[MergableProperty(false)]
     	public string FormatString

   • winforms/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripItem.cs

     Lines 2421 to 2423 in d478d67

     	[Editor("System.ComponentModel.Design.MultilineStringEditor, " + AssemblyRef.SystemDesign, typeof(UITypeEditor))]
     	[Localizable(true)]
     	public string ToolTipText

2. If the type is owned by both CoreFX (or anywhere downstack) then we need to add to the TypeDescriptor's Editor Table; we do that here:

   winforms/src/System.Windows.Forms/src/System/Drawing/Design/UITypeEditor.cs

   Lines 19 to 33 in ab6730e

   	Hashtable intrinsicEditors = new Hashtable
   	{
   	// Our set of intrinsic editors.
   	[typeof(DateTime)] = "System.ComponentModel.Design.DateTimeEditor, " + AssemblyRef.SystemDesign,
   	[typeof(Array)] = "System.ComponentModel.Design.ArrayEditor, " + AssemblyRef.SystemDesign,
   	[typeof(IList)] = "System.ComponentModel.Design.CollectionEditor, " + AssemblyRef.SystemDesign,
   	[typeof(ICollection)] = "System.ComponentModel.Design.CollectionEditor, " + AssemblyRef.SystemDesign,
   	[typeof(byte[])] = "System.ComponentModel.Design.BinaryEditor, " + AssemblyRef.SystemDesign,
   	[typeof(Stream)] = "System.ComponentModel.Design.BinaryEditor, " + AssemblyRef.SystemDesign,
   	[typeof(string[])] = "System.Windows.Forms.Design.StringArrayEditor, " + AssemblyRef.SystemDesign,
   	[typeof(Collection<string>)] = "System.Windows.Forms.Design.StringCollectionEditor, " + AssemblyRef.SystemDesign
   	};
   	// Add our intrinsic editors to TypeDescriptor.
   	TypeDescriptor.AddEditorTable(typeof(UITypeEditor), intrinsicEditors);

All of that said....
you are right that we need to add a line for this editor ala the 2nd mechanism

[weltkante]

Yeah thats what I was looking for, thanks for linking the file, wasn't aware that WinForms already had one. The issues I linked above (FontEditor, ColorEditor, ImageEditor) probably need to be added there as well, I'll drop a note in the issues.

[zsd4yr]

Yeah thats what I was looking for, thanks for linking the file, wasn't aware that WinForms already had one. The issues I linked above (FontEditor, ColorEditor, ImageEditor) probably need to be added there as well, I'll drop a note in the issues.

Happy to demystify! I have created an issue #2101 to create tests for these.

I am having an issue right now where the following test fails but I would not expect it to based on the intrinsic table above...

[Fact]
public void CollectionEditor_ICollection_HasEditor()
{
    var editor = TypeDescriptor.GetEditor(typeof(ICollection), typeof(UITypeEditor));
    Assert.NotNull(editor);
}

Oddly it is failing in our tests project but when I do something like it in a dotnet new winforms project, the editor returns non-null

[zsd4yr]

^^ Thanks to @ericstj for helping me figure out the issue with the tests. We need a reference to the fascades

[zsd4yr]

https://github.com/dotnet/winforms/blob/release/3.1/src/System.Design/src/System.Design.Forwards.cs

@RussKie With respect to this, I think you should remove that file as it does not do anything. The Facades are generated automatically by the GenFacades target during the build. You should be able to test this yourself by running Build -pack, opening the resulting Microsoft.Private.WinForms transport package in winforms\artifacts\packages\Debug\NonShipping, and then opening the System.Design ref in something like ILSpy to compare the type forwards before and after the presence of your file.

If this is not the case, then something is very wrong and you should talk to @dreddy-work and @Shyam-Gupta who did the work on generating these facades.

[Tanya-Solyanik]

nit: group line 32 and 35 under the common header instead of repeating that header, and consider a more descriptive header.. editors that are used in ...

[RussKie]

I have verified the behaviour without explicit type forwarding and it appears to be working as expected.
More details here - #2078 (comment)