search

miroiu / nodify

Highly performant and modular controls for node-based editors designed for data-binding and MVVM.
https://miroiu.github.io/nodify
MIT License
1.39k stars 224 forks source link

Allow Theme manager to preload themes from different assemblies, after initial load of sample defaults #117

Open hendrikp opened 4 months ago

hendrikp commented 4 months ago

originally related to #111

📝 Description of the Change

Allows preload of different themes through theme manager, if users opt to use Nodify.Shared (without having to modify Nodify.Shared)

🐛 Possible Drawbacks

-

miroiu commented 4 months ago

The problem with dynamic resources is that the app will fail at runtime if they are not found. With this PR it becomes necessary to include one of the themes in App.xaml.

hendrikp commented 4 months ago

hm there is an general issue with the samples, however im unsure if its related to running on NET8. In the preload routine there is a check to query preexisting resources however the ones from the App.xaml are never found as theyre not initalized yet. So what happens is the default "Nodify" theme is always there as duplicate resource (but causing issues with the other themes during switching/espacially for themes not in that Nodify assembly).

I have a fix for that, and other stuff in the ThemeManager to allow for preload of other themes and external themes (from other assemblies). Let me know if you'd like to have that then i can put it in this MR. Likely it will fix also the root cause for the original problem with the border switch.

The theme switch for border doesnt work so its a bug, indepent of static/dynamic it should be fixed and those two resource keys are part of any theme.

miroiu commented 4 months ago

Yes, I would like to see the fix. I know the ThemeManager implementation is not ideal, but it served its purpose so far.

hendrikp commented 4 months ago

Please note, now the MR doesn't actually fix the border theme switch anymore instead it extends the ThemeManager to allow loading of external custom themes, also adding small doc for it.

For the border issue i retried after reverting it and the issue still occurs, it must be related to the border being an inherited DependencyProperty and not a new one on the ItemContainer, or some other strange reason (maybe its copied somewhere). Its unexplainable otherwise to me why that border is the only color that doesn't work for theme switching. However i can't spend more time on it as it works for me as DynamicResource.

hendrikp commented 4 months ago

For some reason changing themes now it is noticeably slower on my PC.

I think thats related to the unload+load order (as now temporary the resources aren't present, if ui gets scheduled by chance inbetween unload/load of new theme then you see some output warnings). However switching the order to having a duplicate in the bg would likely break again the cleanup, would have to reverify.

hendrikp commented 3 months ago

I did some tests today and think its now up to previous performance for those cases, i also tested if the order has effect in regard to the functionality - it seems i can restore the previous therefore avoiding the resource not found exceptions that happen temporarily after unload before.

Ill try to commit the changes tomorrow it should be usable then. (also adjusted title+description of PR)

miroiu commented 4 days ago

In the meantime, I fixed the binding failures for the ComboBox and I found more information about the Border not reacting to color changes in this answer from stack overflow: https://stackoverflow.com/a/17791735

And I was actually wrong in this comment:

The problem with dynamic resources is that the app will fail at runtime if they are not found. With this PR it becomes necessary to include one of the themes in App.xaml.

The changes were done in the Themes/Controls.xaml file that is referenced only by the theme files and is not loaded by apps that are not using a theme. Would you like to create another PR?