xamarin / Xamarin.PropertyEditing

MIT License
24 stars 16 forks source link

Fixed NSColor dispose side effect. #787

Closed mkrueger closed 2 years ago

mkrueger commented 2 years ago

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1455310 Bug 1455310: [Feedback] Editor theme is not correctly updated when the theme changes (VS for Mac 2022 Preview 4)

mkrueger commented 2 years ago

Seems that NSColor got disposed for the IDE. First exception shown when switching the editor theme:

 System.ObjectDisposedException: Cannot access a disposed object.
  Object name: 'AppKit.NSColor'.
     at ObjCRuntime.ThrowHelper.ThrowObjectDisposedException(Object o) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/ObjCRuntime/ThrowHelper.cs:line 34
     at ObjCRuntime.NativeObjectExtensions.GetNonNullHandle(INativeObject self, String argumentName) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/ObjCRuntime/INativeObject.cs:line 29
     at AppKit.NSBox.set_FillColor(NSColor value) in /Users/builder/azdo/_work/1/s/xamarin-macios/src/build/dotnet/macos/generated-sources/AppKit/NSBox.g.cs:line 580
     at Xamarin.PropertyEditing.Mac.DynamicBox.AppearanceChanged() in /Users/mkrueger/work/vsmac/main/external/Xamarin.PropertyEditing/Xamarin.PropertyEditing.Mac/Controls/Custom/DynamicBox.cs:line 82
     at Xamarin.PropertyEditing.Mac.DynamicBox.set_HostResourceProvider(IHostResourceProvider value) in /Users/mkrueger/work/vsmac/main/external/Xamarin.PropertyEditing/Xamarin.PropertyEditing.Mac/Controls/Custom/DynamicBox.cs:line 35
     at Xamarin.PropertyEditing.Mac.PropertyEditorPanel.UpdateResourceProvider() in /Users/mkrueger/work/vsmac/main/external/Xamarin.PropertyEditing/Xamarin.PropertyEditing.Mac/PropertyEditorPanel.cs:line 221
     at Xamarin.PropertyEditing.Mac.PropertyEditorPanel.ViewDidChangeEffectiveAppearance() in /Users/mkrueger/work/vsmac/main/external/Xamarin.PropertyEditing/Xamarin.PropertyEditing.Mac/PropertyEditorPanel.cs:line 213

That error shows up in other areas of the IDE after that & preventing theme switches in some scenarios. Decoupling the DynamicBox from using IDE NSColors fixes the issue.

BretJohnson commented 2 years ago

Approved. @mkrueger - Thanks for this. When you are comfortable with the fix feel free to merge it.

We also just branched PropertyEditing, creating the d17-0-vsmac branch which is what should be used for VSMac 17.0. After this is merged to main, we can do the backport and VSMac bump.

sevoku commented 2 years ago

@BretJohnson we'd like to take the fix asap, but the build is red here. Please verify and merge if you think it's good to go. Also I don't know how the instertion process goes nowedays, please help if you can.

BretJohnson commented 2 years ago

Here's the build error:

  /Users/runner/work/1/s/Xamarin.PropertyEditing.Mac/Controls/Custom/DynamicBox.cs(46,35): error CS0029: Cannot implicitly convert type 'AppKit.NSColor' to 'CoreGraphics.CGColor' [/Users/runner/work/1/s/Xamarin.PropertyEditing.Mac/Xamarin.PropertyEditing.Mac.csproj]

@mkrueger - were you able to test this change locally?

As for updating what's bundled in VSMac, vsmac includes the PropertyEditor as a submodule, under main/external/Xamarin.PropertyEditing. There's no insertion required.

BretJohnson commented 2 years ago

@mkrueger - Can you test this locally, if you haven't already, to confirm it fixes it. I think you probably did test it, but with the build errors I wasn't sure. After confirmation it works, one of us can merge / backport to d17-0-vsmac branch / bump submodule ref in vsmac.

mkrueger commented 2 years ago

@BretJohnson y fixes the issue for me locally. Saw the nscolor.clear issue in the PR - didn't compile it :/

sevoku commented 2 years ago

@BretJohnson yeah Mike tested this, but committed the suggestion here on GH afterwards. That should be fixed now and it builds and work locally, however the CI is still red.

sevoku commented 2 years ago

And you're right, we can just bump the submodule.

mkrueger commented 2 years ago

The difference is that I prevent the colors from disposing by using the CGColors. There is no difference in the display. It would be possible to copy the NSColors as well but I don't really intend to get into the NSColor disposal business.

BretJohnson commented 2 years ago

@gitbot backport d17-0-vsmac

vs-mobiletools-engineering-service2 commented 2 years ago

Backport Job to branch d17-0-vsmac Created! The magic is happening here

vs-mobiletools-engineering-service2 commented 2 years ago

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5707934 for more details.

Therzok commented 2 years ago

The problem is actaully in Xamarin.Mac https://github.com/xamarin/xamarin-macios/issues/13921