Skip to content

Conversation

@nachmore
Copy link
Contributor

Fixes #625 & #637

This will mean that changing some settings will be a bit slower than before, especially when enabling blur which will reload resources twice but this is negligible and only hits in settings.

#637)

This will mean that changing some settings will be a bit slower than before, especially when enabling blur which will reload resources twice but this is negligible and only hits in settings.
@jjw24 jjw24 added the bug Something isn't working label Jul 20, 2022
@jjw24 jjw24 added this to the 1.9.4 milestone Jul 20, 2022
@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label Jul 20, 2022
Copy link
Member

@jjw24 jjw24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, thank you for taking the time to look into this issue and making a fix. The current code is not written with enough flexibility to allow directly calling the required changes to fonts without going through the whole ChangeTheme call, which is confusing and probably why you needed to move UpdateResourceDictionary method out of the if condition.

I think a better design is to move the query font update code in this UpdateResourceDictionary method into its own method so you can call it directly from the view model:

  • if (dict["QueryBoxStyle"] is Style queryBoxStyle &&
    dict["QuerySuggestionBoxStyle"] is Style querySuggestionBoxStyle)
    {
    var fontFamily = new FontFamily(Settings.QueryBoxFont);
    var fontStyle = FontHelper.GetFontStyleFromInvariantStringOrNormal(Settings.QueryBoxFontStyle);
    var fontWeight = FontHelper.GetFontWeightFromInvariantStringOrNormal(Settings.QueryBoxFontWeight);
    var fontStretch = FontHelper.GetFontStretchFromInvariantStringOrNormal(Settings.QueryBoxFontStretch);
    queryBoxStyle.Setters.Add(new Setter(TextBox.FontFamilyProperty, fontFamily));
    queryBoxStyle.Setters.Add(new Setter(TextBox.FontStyleProperty, fontStyle));
    queryBoxStyle.Setters.Add(new Setter(TextBox.FontWeightProperty, fontWeight));
    queryBoxStyle.Setters.Add(new Setter(TextBox.FontStretchProperty, fontStretch));
  • // Query suggestion box's font style is aligned with query box
    querySuggestionBoxStyle.Setters.Add(new Setter(TextBox.FontFamilyProperty, fontFamily));
    querySuggestionBoxStyle.Setters.Add(new Setter(TextBox.FontStyleProperty, fontStyle));
    querySuggestionBoxStyle.Setters.Add(new Setter(TextBox.FontWeightProperty, fontWeight));
    querySuggestionBoxStyle.Setters.Add(new Setter(TextBox.FontStretchProperty, fontStretch));

Also another method for updating the result fonts:

if (dict["ItemTitleStyle"] is Style resultItemStyle &&
dict["ItemSubTitleStyle"] is Style resultSubItemStyle &&
dict["ItemSubTitleSelectedStyle"] is Style resultSubItemSelectedStyle &&
dict["ItemTitleSelectedStyle"] is Style resultItemSelectedStyle &&
dict["ItemHotkeyStyle"] is Style resultHotkeyItemStyle &&
dict["ItemHotkeySelectedStyle"] is Style resultHotkeyItemSelectedStyle)
{
Setter fontFamily = new Setter(TextBlock.FontFamilyProperty, new FontFamily(Settings.ResultFont));
Setter fontStyle = new Setter(TextBlock.FontStyleProperty, FontHelper.GetFontStyleFromInvariantStringOrNormal(Settings.ResultFontStyle));
Setter fontWeight = new Setter(TextBlock.FontWeightProperty, FontHelper.GetFontWeightFromInvariantStringOrNormal(Settings.ResultFontWeight));
Setter fontStretch = new Setter(TextBlock.FontStretchProperty, FontHelper.GetFontStretchFromInvariantStringOrNormal(Settings.ResultFontStretch));
Setter[] setters = { fontFamily, fontStyle, fontWeight, fontStretch };
Array.ForEach(
new[] { resultItemStyle, resultSubItemStyle, resultItemSelectedStyle, resultSubItemSelectedStyle, resultHotkeyItemStyle, resultHotkeyItemSelectedStyle }, o
=> Array.ForEach(setters, p => o.Setters.Add(p)));
}

So then you can call them directly from the view model here:

Let me know if you can make these changes, happy to help/make them also :)

@nachmore
Copy link
Contributor Author

Thanks for the detailed response! I looked around for ways to do what you're suggesting, and avoided it because of how invasive it was :)

I'll see if I can spend some time this weekend refactoring some of the code as you suggest and then resubmit!

@jjw24
Copy link
Member

jjw24 commented Jul 22, 2022

Fantastic. You can just push the new changes to this PR :)

@jjw24 jjw24 modified the milestones: 1.9.4, 1.10.0 Jul 24, 2022
@nachmore
Copy link
Contributor Author

I spent a bit of time looking at this today and it's not clear that a refactor actually achieves much. GetResourceDictionary is mostly about font updating (other than a small bit updating CaretBrush and Foreground).

Additionally, to change this you either need to edit the live resource dictionaries or reapply them anyway using UpdateResourceDictionary which does a very coarse Remove and Add.

Reloading the full monte on font change is relatively cheap, since it only happens in Settings, and is safe as it just reloads everything.

A different option could be to call the resource dictionary updates directly (i.e. UpdateResourceDictionary(GetResourceDictionary()); instead of going through ChangeTheme) but that's not a particularly large change and you lose the safety of having a single (mostly) path to call when theme resources change.

@taooceros
Copy link
Member

why is this one closed? I think you are right that it is quite hard to refactor?

@nachmore
Copy link
Contributor Author

I was futzing with branches and github decided to be helpful and close the PR. I still want to fix this one...

If there is consensus on the original fix (or a different one) then I will bring it back as a new PR.

@taooceros
Copy link
Member

I was futzing with branches and github decided to be helpful and close the PR. I still want to fix this one...

If there is consensus on the original fix (or a different one) then I will bring it back as a new PR.

Sure, the original code sounds good for me though (as a fix). I think what @jjw24 would like to do is to rewrite the whole theme loading system, which may take a bit more time.
Currently we can merge the small fix first. I don't think it will affect performance in a large scale.

@jjw24 jjw24 reopened this Jul 25, 2022
@jjw24
Copy link
Member

jjw24 commented Jul 25, 2022

Yeah, I will do a small rewrite and push up soon.

@nachmore
Copy link
Contributor Author

Yeah, I will do a small rewrite and push up soon.

Let me know if you want me to re-PR the original fix, or a different fix if the overall refactor is going to take a while 😀

@jjw24
Copy link
Member

jjw24 commented Jul 26, 2022

nah should be fine, let's keep in this PR, not a big refactor and you were right, it's not going to bring much efficiency but at least the code will be more flexible and readable. Will find some time today or tomorrow.

@jjw24
Copy link
Member

jjw24 commented Jul 28, 2022

I don't think I will have time to do a proper rewrite, will merge in and tackle it later when needed.

@jjw24 jjw24 enabled auto-merge July 28, 2022 10:25
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Jul 28, 2022
@jjw24 jjw24 merged commit 85061c4 into Flow-Launcher:dev Jul 28, 2022
@jjw24 jjw24 modified the milestones: 1.10.0, 1.9.5 Sep 16, 2022
jjw24 added a commit that referenced this pull request Sep 16, 2022
Force reload theme resources when changing fonts etc in settings
@jjw24 jjw24 mentioned this pull request Sep 18, 2022
jjw24 added a commit that referenced this pull request Sep 27, 2022
* Merge pull request #1061 from Flow-Launcher/remove_winget_ci

* Merge pull request #991 from Flow-Launcher/context_menu_plugin_site

* Merge pull request #1080 from gissehel/caret-position-fix

* Caret position fix : Include PR #1074

* Merge pull request #1283 from nachmore/dev

* Merge pull request #1296 from nachmore/bug_1284

* Merge pull request #1294 from Flow-Launcher/pluginInfoMultipleActionKeyword

* Merge pull request #1299 from nachmore/bug_1269

* Plugin Exception Draft (#1147)

* Merge pull request #1355 from onesounds/LimitWidth

* Merge pull request #1088 from Flow-Launcher/add_spanish_latin_america

* Merge pull request #1387 from Flow-Launcher/fix_exception_duplicate_url_opening

* Merge pull request #1390 from Flow-Launcher/issue_1371

* Merge pull request #1391 from Flow-Launcher/issue_1366
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants