jeremytammik / RevitLookup

Interactive Revit RFA and RVT project database exploration tool to view and navigate BIM element parameters, properties and relationships.
http://thebuildingcoder.typepad.com
MIT License
1.03k stars 294 forks source link

Change to Dark Theme not updating background. #194

Closed ricaun closed 6 months ago

ricaun commented 7 months ago

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.

https://github.com/jeremytammik/RevitLookup/assets/12437519/bc55d358-6f35-4397-8a48-e82a6417b3ae

Nice3point commented 7 months ago

What version of OS and Revit ?

https://github.com/jeremytammik/RevitLookup/assets/20504884/f408e37a-4cbe-40ff-b390-f582a2c8991f

ricaun commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 ? https://github.com/jeremytammik/RevitLookup/blob/b65685f5088b730f52d560e5bf064699979c3d37/RevitLookup.UI/Controls/NavigationView/NavigationView.Navigation.cs#L300-L308

ricaun commented 7 months ago

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 ?

https://github.com/jeremytammik/RevitLookup/blob/b65685f5088b730f52d560e5bf064699979c3d37/RevitLookup.UI/Controls/NavigationView/NavigationView.Navigation.cs#L300-L308

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 commented 7 months ago

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

Nice3point commented 7 months ago

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

ricaun commented 7 months ago

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.

https://github.com/jeremytammik/RevitLookup/assets/12437519/3815ad06-564c-4f29-8885-57e7f6585c7d

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 commented 7 months ago

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 https://github.com/jeremytammik/RevitLookup/blob/b65685f5088b730f52d560e5bf064699979c3d37/RevitLookup.UI/Controls/NavigationView/NavigationView.Navigation.cs#L300-L308 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 commented 7 months ago

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 commented 7 months ago

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. https://github.com/lepoco/wpfui/pull/851

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

Nice3point commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

@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 https://github.com/jeremytammik/RevitLookup/blob/5301493d00ebd01c527fe388109f3ebe1541a2f3/RevitLookup/ViewModels/Pages/SettingsViewModel.cs#L64-L70

ricaun commented 6 months ago

Yes, it works! The theme changes in Runtime.

https://github.com/jeremytammik/RevitLookup/assets/12437519/922b4e17-d769-4c61-aa82-b7e879938678

Now you already released, so the next version 😀

Merry Christmas 🎅🎄

ricaun commented 6 months ago

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

https://github.com/jeremytammik/RevitLookup/assets/12437519/cf67def4-a8c6-4768-a768-3a12718035d6

Nice3point commented 6 months ago

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 commented 6 months ago

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

https://github.com/jeremytammik/RevitLookup/assets/12437519/55e82df0-7d68-4adc-b856-1cd451ba04a2

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

and why display them if they are not supported?

I can keep it, it doesn't matter

ricaun commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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...

https://github.com/jeremytammik/RevitLookup/assets/12437519/79c09af6-1ea4-4eee-b41e-6c06315e5cb9

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

You can do it if you want to)

ricaun commented 6 months ago

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 commented 6 months ago

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

Nice3point commented 6 months ago

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

Nice3point commented 6 months ago

изображение

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 commented 6 months ago

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

https://github.com/jeremytammik/RevitLookup/assets/20504884/28a67b64-d7b1-4119-bc16-0d215265b532

ricaun commented 6 months ago

изображение

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 😃