Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change to Dark Theme not updating background. #194

Closed
ricaun opened this issue Dec 1, 2023 · 45 comments
Closed

Change to Dark Theme not updating background. #194

ricaun opened this issue Dec 1, 2023 · 45 comments
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior

Comments

@ricaun
Copy link
Contributor

ricaun commented Dec 1, 2023

I updated the latest version and tested it in Revit 2024 and noticed that when changed to theme Dark the background color is not changing.

If I close and open works fine.

Here is a video.

RevitLookUp.2024.0.10.-.2023-12-01.16-32-56.mp4
@Nice3point
Copy link
Collaborator

Nice3point commented Dec 1, 2023

What version of OS and Revit ?

Autodesk.Revit.2023_2023.12.01-23.28_2.mp4

@ricaun
Copy link
Contributor Author

ricaun commented Dec 1, 2023

Windows 10 - 10.0.19045 Build 19045
Revit 24.0.4.427 - 20230308_1635(x64)

I guess the last version I tested It's needed to close the dialog to apply the theme.

@Nice3point
Copy link
Collaborator

Most likely the problem is related to windows 10. Unfortunately, both pomianowski (wpfui owner) and I have windows 11. I don't know if this bug will be fixed or not, but I won't be able to reproduce it, you can try to fix it if you have time

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 1, 2023

Previously the theme and applied when opening a new window. But now I have added resource transfer to NavigationService when navigating in Frame. On Win 11 it works, on Win 10 it doesn't for some reason

Can u debug it ?

private void UpdateDictionary(object? content)
{
if (content is FrameworkElement frameworkViewContent)
{
var window = Window.GetWindow(this);
if (window is null) return;
frameworkViewContent.Resources = window.Resources;
}
}

@Nice3point Nice3point added the bug 🐛 An unexpected issue that highlights incorrect behavior label Dec 1, 2023
@ricaun
Copy link
Contributor Author

ricaun commented Dec 1, 2023

Previously the theme and applied when opening a new window. But now I have added resource transfer to NavigationService when navigating in Frame. On Win 11 it works, on Win 10 it doesn't for some reason

Can u debug it ?

private void UpdateDictionary(object? content)
{
if (content is FrameworkElement frameworkViewContent)
{
var window = Window.GetWindow(this);
if (window is null) return;
frameworkViewContent.Resources = window.Resources;
}
}

That's strange to have some relation with the Windows version.

Yes, I can debug that, I think that is missing to update some resources. I having some similar issue when importing the Wpf.Ui to work with Revit this week, I used the release version 2.2.0.

Wpf Ui - 2023-12-01 15-56-34

@ricaun
Copy link
Contributor Author

ricaun commented Dec 1, 2023

The RevitLookup.UI.Demo fails as well, gonna be easier to fix.

@Nice3point
Copy link
Collaborator

2.2 is very different from the current one, I use 3.0 preview. Thanks for the help in debugging

@ricaun
Copy link
Contributor Author

ricaun commented Dec 2, 2023

I tried a lot of things, but in the end, looks like the resource is not refreshing the windows correctly for some reason.

The only way I was able to work was by using this terrible approach in the Changed event.

https://github.com/ricaun/RevitLookup/blob/3917b7b5f2d88a076e03b7daf854352e8e240b85/RevitLookup/Views/RevitLookupView.xaml.cs#L79C1-L104C10

This force re-add the resource and the windows refresh to the dark mode, you could use that to refresh the other windows.

RevitLookUp.-.ThemeChanged.-.2023-12-02.10-24-52.mp4

I don't know what other change you made in the RevitLookup.UI compared to the Wpf.Ui, I suppose you removed the Application.Current and added some stuff like the Application class.

@Nice3point
Copy link
Collaborator

I think something needs to be done in NavigationView which inherits from Frame. All in dll applications there is such a problem that Page does not inherit resources from Window. Wpf ui was designed for exe applications where this is not observed. So as you show in the video the theme does not apply specifically to Page, Window is fine. So in this code

private void UpdateDictionary(object? content)
{
if (content is FrameworkElement frameworkViewContent)
{
var window = Window.GetWindow(this);
if (window is null) return;
frameworkViewContent.Resources = window.Resources;
}
}
when navigating I directly assign the resource dictionary from the window to the Page. I didn't think it wouldn't work on WIn 10, because there are no WinAPI calls. Maybe IF checks are not executed, or something else

@Nice3point
Copy link
Collaborator

events is certainly not the most efficient approach. maybe try to debug Apply https://github.com/ricaun/RevitLookup/blob/3917b7b5f2d88a076e03b7daf854352e8e240b85/RevitLookup/ViewModels/Pages/SettingsViewModel.cs#L54 method ? it contains a lot of OS dependent code
изображение

@ricaun
Copy link
Contributor Author

ricaun commented Dec 2, 2023

I really don't know the reason in my machine the windows Ui does not update probably. Maybe is missing some trigger in the OS that force the window to update the resource.

I'm testing the preview 3.0 and was able to make work inside Revit, in my implementation I'm using the Changed event to update the resource in each Window/Page. Without changing to much in the main Wpf.Ui that's the way to do it, I guess.

Here is my PR. lepoco/wpfui#851

I gonna build a package and try swap the RevitLookUp.UI to see if works.

@Nice3point
Copy link
Collaborator

I don't really like the crutch of subscribing to events. I'm sure there is a more elegant solution. For now, for Win10 we can revert to the old code that changed the theme when new windows were opened

    partial void OnThemeChanged(ApplicationTheme value)
    {
        settingsService.Theme = value;
        if (Wpf.Ui.Win32.Utilities.IsOSWindows11OrNewer)
        {
            ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background);
            return;
        }

        notificationService.ShowSuccess("Theme changed", "Changes will take effect for new windows");
    }

@Nice3point
Copy link
Collaborator

This is not a primary problem, as very few people are still using WIn10, and losing performance to Win11 is not desirable for this purpose

@ricaun
Copy link
Contributor Author

ricaun commented Dec 2, 2023

This is not a primary problem, as very few people are still using WIn10, and losing performance to Win11 is not desirable for this purpose

I tested on another machine with Windows 10 and had the same problem (tested in the Hosted Revit Preview Machine).

I don't really like the crutch of subscribing to events. I'm sure there is a more elegant solution. For now, for Win10 we can revert to the old code that changed the theme when new windows were opened

    partial void OnThemeChanged(ApplicationTheme value)
    {
        settingsService.Theme = value;
        if (Wpf.Ui.Win32.Utilities.IsOSWindows11OrNewer)
        {
            ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background);
            return;
        }

        notificationService.ShowSuccess("Theme changed", "Changes will take effect for new windows");
    }

Yes, this approach is much better.

@Nice3point
Copy link
Collaborator

@ricaun Hi Luiz, I've moved all the changes from the wpf.ui repository, there were some windows10 related fixes there recently, if you have time can you check the latest build from the dev branch? Specifically try commenting out the lines and leave only ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background); I wonder if this helped with changing the theme in Runtime

if (IsOSWindows11OrNewer)
{
ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background);
return;
}
notificationService.ShowSuccess("Theme changed", "Changes will take effect for new windows");

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

Yes, it works! The theme changes in Runtime.

RevitLookup.2023-12-25.21-32-15.mp4

Now you already released, so the next version 😀

Merry Christmas 🎅🎄

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

Or not... I was messing with the Acrylic and Blur... Another issue maybe.

RevitLookUp.-.2023-12-25.21-41-32.mp4

@Nice3point
Copy link
Collaborator

Oh great, that problem solved 🙃 What if in the OnBackgroundChangedwindow method instead of WindowBackdropType = value; write ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background); ? Ideally this should give the same result in the end, but how does it behave on your OS? In the right way, nothing should happen when you switch. All these backgrounds are not available on win10, maybe disable them altogether 🙈

Merry Christmas 🎅

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

In the last Release, the Acrylic breaks the Dark UI.

RevitLookUp.-.2023-12-26.09-06-20.mp4

And I found this after installing the last Release. An extra ru folder inside 2024, there is the same ru folder inside the RevitLookUp.

image

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

Oh great, that problem solved 🙃 What if in the OnBackgroundChangedwindow method instead of WindowBackdropType = value; write ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background); ? Ideally this should give the same result in the end, but how does it behave on your OS? In the right way, nothing should happen when you switch. All these backgrounds are not available on win10, maybe disable them altogether 🙈

The only one that breaks the Dark UI is the WindowBackdropType.Acrylic the rest does nothing, same as disable I guess.

If I disable the window.WindowBackdropType = value in the OnBackgroundChanged does not update in the current window, if I open a new one, the effect is applied, and the Dark UI breaks.

If I disable this, the Acrylic does break as well:
https://github.com/jeremytammik/RevitLookup/blob/1bea5226852349f5a9b215e8dd3df4c22f2661f8/source/RevitLookup.UI/Interop/UnsafeNativeMethods.cs#L105C4-L123C6

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

The Acrylic should work in IsOSWindows7OrNewer, in the Light version, I don't see any change. Change to IsOSWindows11OrNewer, same as the Mica.

https://github.com/jeremytammik/RevitLookup/blob/1bea5226852349f5a9b215e8dd3df4c22f2661f8/source/RevitLookup.UI/Controls/Window/WindowBackdrop.cs#L29C1-L29C79

@Nice3point
Copy link
Collaborator

And I found this after installing the last Release. An extra ru folder inside 2024, there is the same ru folder inside the RevitLookUp.

I need to check where it came from

@Nice3point
Copy link
Collaborator

I think it's worth disabling all background effects in Windows 10 for now as they don't work there,maybe they will be fixed in wpf.ui in time. And unfortunately I can't test this, so I'm using 11

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 26, 2023

By the end of the week I think to publish Release with fixes, but in the meantime it's worth to look for any more bugs

@ricaun In general, keeps the Runtime theme switching, but turn off all background effects?

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

And I found this after installing the last Release. An extra ru folder inside 2024, there is the same ru folder inside the RevitLookUp.

I need to check where it came from

Probably in the installation, the ru is not moved or deleted.

I don't like to use these resources for multiple languages, the extra folder/dll is annoying. Most of the time I create a json/object and swap depending on the CurrentUICulture.

The ColorRepresentationUtils already have a KnownColors Dictionary, and swapping should be easy.

I could create a PR if you think that makes sense, with a JSON at least gonna be easier to translate to other languages 😀

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

By the end of the week I think to publish Release with fixes, but in the meantime it's worth to look for any more bugs

@ricaun In general, keeps the Runtime theme switching, but turn off all background effects?

Yes, turn off all the background effects in Windows 10.

I gonna test the UI.Demo it in the Autodesk machine to see if works fine.

@Nice3point
Copy link
Collaborator

Probably in the installation, the ru is not moved or deleted.

There was a problem with the installer, I wrote code that handled subfolders badly and duplicated them to the root directory. this has now been fixed. this was not observed during debugging or compiling with Nuke

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 26, 2023

Most of the time I create a json/object and swap depending on the CurrentUICulture.

I wonder what it looks like, never seen anything like it before 🙈😊. Just resx files are basic for localisation of WPF applications, they are supported by Vs and Rider

I can also import them from csv
изображение

I can add comments to mark where this resource is used, it's very handy.
изображение

I think CSV format is preferable as it has a table representation

Colors.csv

изображение

How convenient and efficient is it to use JSON?

@Nice3point
Copy link
Collaborator

Yes, turn off all the background effects in Windows 10.

Turned it off in the last commit, you should have it like in the screenshot
изображение

@ricaun
Copy link
Contributor Author

ricaun commented Dec 26, 2023

Yes, turn off all the background effects in Windows 10.

Turned it off in the last commit, you should have it like in the screenshot.

Oh, you remove the background effects options, neat!
I thought you gonna change WindowBackdropType.Acrylic => Win32.Utilities.IsOSWindows7OrNewer, to IsOSWindows11OrNewer. 😀

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 26, 2023

and why display them if they are not supported?

I can keep it, it doesn't matter

@ricaun
Copy link
Contributor Author

ricaun commented Dec 27, 2023

I wonder what it looks like, never seen anything like it before 🙈😊. Just resx files are basic for localisation of WPF applications, they are supported by Vs and Rider

I can also import them from csv

I'm not sure if Visual Studio has this csv import, never needed something like that.

How convenient and efficient is it to use JSON?

The structure would be something like this: KnownColors.json

{
  "000000": "Black",
  "000080": "Navy blue",
  "0000FF": "Blue",
  "FFFFCC": "Conditioner",
  "FFFFFF": "White"
}

Just deserialize to a Dictionary<string,string>, and is good to go! That only makes sense because the code has the KnownColors static Dictionary, is easy to swap a KnownColors.json for each language.

You could store the file KnownColors.json in the Resources, better than each key separately.

The Localization.Colors resources in not Encoding.UTF8 there are some strange characters.

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 27, 2023

Just deserialize to a Dictionary<string,string>, and is good to go!

I would still prefer to use Resx files, it is still a native way of localisation, and especially if we need to localise Xaml files or parts of it, Json is not suitable for such tasks, and it is bad practice to have several ways of localisation 🙈

For JSON you will also have to duplicate keys in all languages as I understand, which is not convenient. I follow the principle of using the format most applicable to the task, not using only 1 way because it is popular, Json is good for web, yes, but here it is bad, csv is suitable

especially you will never use ru localisation and you will not need to load these resources into the application

@ricaun
Copy link
Contributor Author

ricaun commented Dec 27, 2023

The only reason to use JSON is that is convenient to convert to c# object.

Instead of using:

private static readonly Dictionary<string, string> KnownColors = new()
{
    {"000000", Resources.Localization.Colors._000000},
    {"000080", Resources.Localization.Colors._000080},
    ...
    {"FFFFFF", Resources.Localization.Colors.FFFFFF},
};

Would be something like:

private static readonly Dictionary<string, string> KnownColors =
    Encoding.UTF8.GetString(Resources.Localization.Colors.KnownColors)
        .ToJson<Dictionary<string, string>>();

Still gonna have the Colors resources but instead of having each color key separated, you gonna have one file.

@Nice3point
Copy link
Collaborator

@ricaun I'll talk to the Microsoft guys about it. In the meantime we need to see if what we discussed above is fixed. If you have time, you can check out the latest dev build, and we'll roll out the Release on Friday

@ricaun
Copy link
Contributor Author

ricaun commented Dec 27, 2023

@ricaun I'll talk to the Microsoft guys about it. In the meantime we need to see if what we discussed above is fixed. If you have time, you can check out the latest dev build, and we'll roll out the Release on Friday

The background effect is working.

And dark background is not updating the other windows, I don't remember if was working before...

RevitLookUp.-.2023-12-27.16-56-51.mp4

Maybe is better to just add the if (Wpf.Ui.Win32.Utilities.IsOSWindows11OrNewer) like the last version.

 partial void OnThemeChanged(ApplicationTheme value)
{
    settingsService.Theme = value;
    if (Wpf.Ui.Win32.Utilities.IsOSWindows11OrNewer)
    {
        ApplicationThemeManager.Apply(settingsService.Theme, settingsService.Background);
        return;
    }

    notificationService.ShowSuccess("Theme changed", "Changes will take effect for new windows");
}

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 27, 2023

previous windows will not be updated, only new windows and the current window 🙃I didn't write the logic for updating old instances. I don't need to fix it for now, but rather focus on new tools, plans are in the works for BIP checker as many people have asked for, Modules and more. But for now I have a lot of work to do, so Lookup will be in the background

@ricaun
Copy link
Contributor Author

ricaun commented Dec 28, 2023

I assume that was already done.

You could use the Theme event to trigger the other windows to swap the resources. Is visually pleasing when you change the theme and all windows themes change as well. 😀

@Nice3point
Copy link
Collaborator

You can do it if you want to)

@ricaun
Copy link
Contributor Author

ricaun commented Dec 28, 2023

You can do it if you want to)

I did a PR #200.

I update the ApplicationThemeManager.Apply to update each Application.Windows in there.

@Nice3point
Copy link
Collaborator

Then I reckon we're ready to make the last release of this year)

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 28, 2023

Only I noticed one bug after your comit, now the windows open backwards 🙈
изображение

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 28, 2023

изображение

You can just put this loop in the ViewModel, this ensures that the theme is applied to all windows only once and not every time a new instance of RevitLookup is opened

@Nice3point
Copy link
Collaborator

Nice3point commented Dec 28, 2023

The result is the same, only we don't touch the UI library) which makes it easier to port patches

unknown_2023.12.29-01.54.mp4

@ricaun
Copy link
Contributor Author

ricaun commented Dec 29, 2023

изображение

You can just put this loop in the ViewModel, this ensures that the theme is applied to all windows only once and not every time a new instance of RevitLookup is opened

Yes, that's looks great 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior
Projects
None yet
Development

No branches or pull requests

2 participants