Options for styling of NetSparkle.UI.WPF to follow Host Application

christophwille commented 1 year ago

Discussion got started in https://github.com/NetSparkleUpdater/NetSparkle/issues/466#issuecomment-1577005537

Reference sample: https://github.com/icsharpcode/ILSpy/tree/netsparkleupdater (Theme switching: View / Theme > ..., Manual update check: Help / Check for updates )

Code in question: https://github.com/NetSparkleUpdater/NetSparkle/tree/develop/src/NetSparkle.UI.WPF

Haven't yet looked into the options directly inside ILSpy, but some general techniques:

Copy and paste. That is the "nuclear" option with a very hard time of syncing with the original NuGet (maybe the ViewModels can cut this down to "only" the XAML files)
Passing and binding of style names, style dictionaries, ....?
Definitely looking around if someone has solved such an issue before (control style inheritance)

christophwille commented 1 year ago

@tom-englert as ILSpy's "resident expert" on all things WPF styling - do you know of recommendations of such "style inheritance" (NuGet picking up the style of the host application) that you could point us to? Thanks!

tom-englert commented 1 year ago

@christophwille looked at NetSparkle, and they use plain default controls only, so that should work. What exact problem do you experience? How can I reproduce it?

christophwille commented 1 year ago

https://github.com/NetSparkleUpdater/NetSparkle/issues/466#issuecomment-1576956277 here is one screenshot - I had dark mode set on the previous run (so no switching in the same instance), and then checked for updates. Only the buttons seemed to have picked up dark. (for repro - see initial issue comment with my branch https://github.com/icsharpcode/ILSpy/tree/netsparkleupdater)

tom-englert commented 1 year ago

https://github.com/NetSparkleUpdater/NetSparkle/blob/0561653b064dc19e9e042530fba5b09794142558/src/NetSparkle.UI.WPF/UpdateAvailableWindow.xaml#L19

This window has a hard-coded background

tom-englert commented 1 year ago

And the embedded web-browser will of course not follow the WPF styling

christophwille commented 1 year ago

Oh, I was expecting something way more involved than a hardcoded value... thanks! Yes, the Web browser won't follow the style, I am fully aware of that.

The only thing that is really off then is the window chrome because of our custom style there https://github.com/icsharpcode/ILSpy/blob/b6535a4d710bfc94dcc14d49fe5abc07aa7835e9/ILSpy/App.xaml#L10

Deadpikle commented 1 year ago

Ah, I forgot about that hardcoded value. See #396. It's up for removal on breaking changes.

Just pushed a new preview version with a new property on the UIFactory -- Just set UseStaticUpdateWindowBackgroundColor to false and you'll be set. (Defaults to true because legacy.) Also added this to the Avalonia UI, which already had something like this to get around the exact same issue.

For the HTML in the change log, an app I used to work on just swapped out some CSS when the update window opened. If you set the AdditionalReleaseNotesHeaderHTML property (or override the UIFactory method as below, which I did in that app for various reasons [it ran its own UI]), you can config the CSS to match your theme. Yes, if they changed the theme AFTER the update window opens, things wouldn't match unless you update the CSS/etc. again, but I figured at the time that this was pretty low chance and Generally Not A Big Deal™️ to worry about.


        public virtual IUpdateAvailable CreateUpdateAvailableWindow(SparkleUpdater sparkle, List<AppCastItem> updates, bool isUpdateAlreadyDownloaded = false)
        {
            var extraHeadAdditionForReleaseNotes = "<style>";
            if (ThemeManager.Current.ApplicationTheme == ApplicationTheme.Dark)
            {
                extraHeadAdditionForReleaseNotes +=
                    "body {background-color: #212121; } " +
                    "h1, h2, li { color: #e8e8e8; } ";
            }
            extraHeadAdditionForReleaseNotes +=
                "h1, h2 { margin-top: -8px; margin-left: 6px; } ";
            extraHeadAdditionForReleaseNotes += "</style>";
            var viewModel = new UpdateAvailableWindowViewModel();

            var window = new UpdateAvailableWindow(viewModel)
            {
                Icon = _applicationIcon,
                WindowStartupLocation = WindowStartupLocation.CenterScreen
            };
            if (HideReleaseNotes)
            {
                (window as IUpdateAvailable).HideReleaseNotes();
            }
            if (HideSkipButton)
            {
                (window as IUpdateAvailable).HideSkipButton();
            }
            if (HideRemindMeLaterButton)
            {
                (window as IUpdateAvailable).HideRemindMeLaterButton();
            }
            viewModel.Initialize(sparkle, updates, isUpdateAlreadyDownloaded, string.Empty, extraHeadAdditionForReleaseNotes);
            return window;
        }

tom-englert commented 1 year ago

And for completeness:

    var window = new UpdateAvailableWindow(viewModel)
    {
        Icon = _applicationIcon,
        WindowStartupLocation = WindowStartupLocation.CenterScreen,
        Style = (Style)Application.Current.FindResource("DialogWindow")
    };

christophwille commented 1 year ago

I went with AdditionalReleaseNotesHeaderHTML for simplicity in test (so the chrome doesn't match)

christophwille commented 1 year ago

Icon = _applicationIcon,

I just tried that - when deriving from NetSparkleUpdater.UI.WPF.UIFactory that cannot be accessed because the field is private. That should be protected.

tom-englert commented 1 year ago

So if you're going to style the HTML, also select a Grotesk font to match the applications style.

Deadpikle commented 1 year ago

I went with AdditionalReleaseNotesHeaderHTML for simplicity in test (so the chrome doesn't match)

[image omitted]

Glad that it's working better. Would it be helpful to add in Style hooks to all the applicable windows so you can get the chrome to match without having to derive from UIFactory?

Icon = _applicationIcon,
I just tried that - when deriving from NetSparkleUpdater.UI.WPF.UIFactory that cannot be accessed because the field is private. That should be protected.

Yup. I agree.

christophwille commented 1 year ago

Glad that it's working better. Would it be helpful to add in Style hooks to all the applicable windows so you can get the chrome to match without having to derive from UIFactory?

There are quite a few windows, so I'd need to override & copy implementation details of the original all over the place. A "WindowChromeStyle" property or the like that is applied to all windows that open would be most convenient I'd guess.

tom-englert commented 1 year ago

Or maybe a callback with the new window as parameter, so you are free to post process the window as you like?

christophwille commented 1 year ago

Callback would allow most customization - what aside from Style do you think would be a "common" thing to change?

Deadpikle commented 1 year ago

Try 2.3.0-preview20230606002 when it finishes publishing. Added a ProcessWindowAfterInit property to the UIFactory -- definition of delegate is: public delegate void WindowHandler(Window window, UIFactory factory);. (Naming things is hard; if you have better ideas for names, let me know.)

The view model is set before this is called, so in theory this would even let you reach into the view model. Having this sort of capability would have removed the need for things like ReleaseNotesGrabberOverride, UseStaticUpdateWindowBackgroundColor, UpdateWindowGridBackgroundBrush, etc. -- albeit make them a little less easily accessible. So the shortcuts are good to keep around for ease-of-use and then I figure the "power tools" can stick around for those who want more control.

edit: and the icon is protected now rather than public -- although I suppose you could have set this via the base constructor, I think making this protected is easier all around.

christophwille commented 1 year ago

ok, something strange is going on: the initial window of the update check (the one with the progress bar) runs fine and has the style applied, however, just before the actual dialog for updates comes up it crashes on

                    ProcessWindowAfterInit = (w, f) => {
                        w.Style = (Style)Application.Current.FindResource("DialogWindow");
                    }

with

System.InvalidOperationException
  HResult=0x80131509
  Message=The calling thread cannot access this object because a different thread owns it.
  Source=WindowsBase
  StackTrace:
   at System.Windows.Threading.Dispatcher.VerifyAccess()
   at System.Windows.Freezable.Clone()
   at TomsToolbox.Wpf.StyleBindings.<>c.<Behaviors_Changed>b__19_0(Behavior item)
   at System.Linq.Enumerable.SelectIListIterator`2.MoveNext()
   at TomsToolbox.Essentials.CollectionExtensions.AddRange[T](ICollection`1 target, IEnumerable`1 items)
   at System.Windows.DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
   at System.Windows.FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
   at System.Windows.DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args)
   at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType)
   at System.Windows.StyleHelper.ApplyStyleOrTemplateValue(FrameworkObject fo, DependencyProperty dp)
   at System.Windows.StyleHelper.InvalidateContainerDependents(DependencyObject container, FrugalStructList`1& exclusionContainerDependents, FrugalStructList`1& oldContainerDependents, FrugalStructList`1& newContainerDependents)
   at System.Windows.StyleHelper.DoStyleInvalidations(FrameworkElement fe, FrameworkContentElement fce, Style oldStyle, Style newStyle)
   at System.Windows.StyleHelper.UpdateStyleCache(FrameworkElement fe, FrameworkContentElement fce, Style oldStyle, Style newStyle, Style& styleCache)
   at System.Windows.FrameworkElement.OnStyleChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
   at System.Windows.DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
   at System.Windows.FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
   at System.Windows.DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args)
   at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType)
   at System.Windows.DependencyObject.SetValueCommon(DependencyProperty dp, Object value, PropertyMetadata metadata, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType, Boolean isInternal)
   at System.Windows.DependencyObject.SetValue(DependencyProperty dp, Object value)
   at System.Windows.FrameworkElement.set_Style(Style value)
   at ICSharpCode.ILSpy.MainWindow.<>c.<EnableSparkleUpdateChecking>b__59_0(Window w, UIFactory f) in D:\GitWorkspace\ILSpy\ILSpy\MainWindow.xaml.cs:line 957
   at NetSparkleUpdater.UI.WPF.UIFactory.CreateUpdateAvailableWindow(SparkleUpdater sparkle, List`1 updates, Boolean isUpdateAlreadyDownloaded)
   at NetSparkleUpdater.SparkleUpdater.<>c__DisplayClass130_0.<ShowUpdateAvailableWindow>b__2(Object state)
   at NetSparkleUpdater.SparkleUpdater.<>c__DisplayClass130_0.<ShowUpdateAvailableWindow>b__1()

I saw that happen on my custom UI Factory too. Could it be that somehow in the chain we end up not on the UI thread? (my changes are checked in to the branch for testing)

christophwille commented 1 year ago

ok, if I use ShowsUIOnMainThread = true then this error goes away. Is finding a resource really thread-dependent? Seems so.

christophwille commented 1 year ago

The looks if anyone stumbles across this issue later Animation

tom-englert commented 1 year ago

Callback would allow most customization - what aside from Style do you think would be a "common" thing to change?

I don't have any special thing in mind, just to adhere to open/close principle.

tom-englert commented 1 year ago

Having ShowsUIOnMainThread default to false for WPF applications seems to be a fatal design flaw.

Also this condition looks to be wrong, shouldn't it rather marshal to the UI thread if ShowsUIOnMainThread is true?: https://github.com/NetSparkleUpdater/NetSparkle/blob/0c51fa54040ac330671cf768eadc14ddccf8f420/src/NetSparkle/SparkleUpdater.cs#L1802-L1812

Deadpikle commented 1 year ago

Could it be that somehow in the chain we end up not on the UI thread?

Yes. As you noticed, ShowsUIOnMainThread is a thing and will cause the update work on another thread. (In the 0.x and 1.x days, when this lib was WinForms only, this property was called UseSyncronizedForms and made a lot more sense for the WinForms world. It makes no sense for Avalonia at all and must be true, but for WPF it can roll either way.) I should update the docs that the ProcessWindowAfterInit func may not be called on the UI thread.

Can you Freeze() your style and anything else that needs it there? That should fix the immediate issue. (I need to pull out my Windows laptop and test things myself. Life is very busy right now, sorry.) Edit: nvm on this you can't freeze a style.

In hindsight, the main lib should probably be asking the UIFactory for the default value.

Also this condition looks to be wrong, shouldn't it rather marshal to the UI thread if ShowsUIOnMainThread is true?:

No, this is correct:

if we're showing UI on the main thread, then when this func is called, we are already on the main thread, so we can just invoke the action
If we're NOT showing UI on the main thread (else), then when this func is called, we are probably on a background thread, so we need to call to the UI thread

Having ShowsUIOnMainThread default to false for WPF applications seems to be a fatal design flaw.

Can you please explain for my (and everyone in the future's) understanding? This is the first time someone has had an issue with the way this works since 2.x was released 2 years ago. (Edit: Text is hard and misses voice inflection -- I want to make sure that this doesn't come off as ornery or defensive. Genuinely trying to learn, here. Apologies if this came off the wrong way.)

Deadpikle commented 1 year ago

Threading issues moved to https://github.com/NetSparkleUpdater/NetSparkle/issues/470 since I now am questioning my own understanding of the code and need to collate these problems in one place

tom-englert commented 1 year ago

Can you please explain for my (and everyone in the future's) understanding? This is the first time someone has had an issue with the way this works since 2.x was released 2 years ago. (Edit: Text is hard and misses voice inflection -- I want to make sure that this doesn't come off as ornery or defensive. Genuinely trying to learn, here. Apologies if this came off the wrong way.)

Yes, you can create WPF windows on another thread, but that's a very fragile approach with a lot of pit falls. It may have worked the last two years because you did not expose any UI internals, but now you do and the first thing you encounter is that someone traps into the pit. There is absolutely no benefit in interacting with the UI in a background thread

you have the same number of context switches, just on another place
you already have the working solution to interact with the UI only on the main thread
the behavior of both approaches from a consumers POV is exactly the same

So why do you want to default to a fragile design with many pitfalls if you have the robust approach already in place? ShowsUIOnMainThread will work great for any UI, is robust and does not contain any pitfalls. And why do you want to offer to switch between both modes, it only clutters your code and makes is hard to understand. (e.g. see my confusion about CallFuncConsideringUIThreads, I guess none beside you will understand what the current thread is in which method)

Deadpikle commented 1 year ago

@tom-englert Thanks for your input. I welcome yours and @christophwille 's input on https://github.com/NetSparkleUpdater/NetSparkle/issues/470#issuecomment-1585298415 if you feel like it. (tl;dr: I'm seriously considering ripping out that option, as you've noted would be best.)

@christophwille I think the original purpose for this Issue is fixed...?

christophwille commented 1 year ago

In my case, yes, ShowsUIOnMainThread set to true fixed the issue.

Deadpikle commented 1 year ago

I believe the original purpose for this issue (the styling problems/solutions) has been fixed, so I'm going to close this issue since the other things discussed are in other threads. Thanks! :smile:

Deadpikle commented 3 weeks ago

Just a note here for future searchers: ShowsUIOnMainThread is now gone entirely with all the mess it created/made/brought with it, so all the discussion surrounding that has been resolved. The only calling from background to "UI thread" NetSparkleUpdater explicitly does now is as follows: it has a background loop that starts on a background thread. When this background thread calls an event or shows the update UI, it posts that call to the SyncronizationContext for the thread that the SparkleUpdater instance was created on as a convenience to users. That's it. CallFuncConsideringUIThreads is gone; ShowsUIOnMainThread is gone, etc.

Appreciate the input that was given on it above. Thank you.

NetSparkleUpdater / NetSparkle

Options for styling of NetSparkle.UI.WPF to follow Host Application #467