Allow Theme manager to preload themes from different assemblies, after initial load of sample defaults #117
Conversation
|
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. |
|
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. |
|
Yes, I would like to see the fix. I know the ThemeManager implementation is not ideal, but it served its purpose so far. |
This reverts commit 0122335.
…sent in App.xaml, also allow for Preload of Themes from Application with different assembly
|
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. |
| // as Theme resources in samples placed explicitly in App.xaml for xaml preview purposes | ||
| // (however ThemeManager doesn't find those as already preloaded on startup, | ||
| // because MainWindow not initialized yet) | ||
| var duplicates = Application.Current.Resources.MergedDictionaries.Where(r => r.Source == res.Source).ToList(); |
There was a problem hiding this comment.
Can we detect duplicates in the PreloadThemes function? This may be the reason why changing themes is now slower than before (verified with ~400 nodes in Playground)
There was a problem hiding this comment.
its possible, but i guess the order is more important, not necessarily the duplication. The performance issue is likely related to Output Warning messages.
| > Tip: Right-click and drag the screen around to move the view and use the mouse wheel to zoom in and out. |
There was a problem hiding this comment.
The wiki contains documentation only about Nodify because that's the published package on NuGet. The Nodify.Shared project was developed as a utility project for quickly developing example apps and was not meant to be used in production. However, the best place for this type of documentation would be a new README file in the Examples folder.
| ``` | ||
|
|
||
| Redefine all colors you dislike by copying and them to your theme from: |
There was a problem hiding this comment.
I think you meant: by copying them
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
changed the title
|
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) |
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