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

Use Breeze style in Flatpak #681

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Use Breeze style in Flatpak #681

merged 1 commit into from
Jul 13, 2020

Conversation

ilya-fedin
Copy link
Contributor

Before
Снимок экрана в 2020-07-03 05-20-53
After
Снимок экрана в 2020-07-03 05-21-08

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Many thanks for the contribution!

To be honest, it looks quite hardcodey (and ugliness is a matter of taste - call me old-fashioned but I don't find Fusion too ugly - especially after Windows :-/). I'm fine to override the default theme to be Breeze but what if someone actually uses Fusion on his, say, LXQt-based desktop? And what if Quaternion is packed with Snap another day? Your code seems to narrow the target case unnecessarily.

client/main.cpp Show resolved Hide resolved
@KitsuneRal KitsuneRal added the enhancement A feature or change request for Quaternion label Jul 3, 2020
@ilya-fedin
Copy link
Contributor Author

ilya-fedin commented Jul 3, 2020

I'm fine to override the default theme to be Breeze but what if someone actually uses Fusion on his, say, LXQt-based desktop?

I also thought about that, but Qt doesn't have any API to get the current style name nor to check if it is was by the user or is default fallback. All I can do is check the values of the variables QT_QPA_PLATFORMTHEME and QT_STYLE_OVERRIDE, but, probably, that would be even more ugly (that's why I didn't do it first).

And what if Quaternion is packed with Snap another day?

Currently, the recommended way for snapping is kde-neon extension. Their wrapper already sets a theme.

@KitsuneRal
Copy link
Member

Qt doesn't have any API to get the current style name nor to check if it is was by the user or is default fallback. All I can do is check the values of the variables QT_QPA_PLATFORMTHEME and QT_STYLE_OVERRIDE, but, probably, that would be even more ugly (that's why I didn't do it first).

Right, relying on environment variables for hints is even more fragile. However, for now this looks like we impose another default over the Qt's default with no means to revert to the Qt's default - and if I got it right then sorry it's not an option :) If we can't give users the flexibility to bring the theme back to Fusion, we can merely provide an easy and unobtrusive way to switch to Breeze from inside Quaternion (via a setting, or a button somewhere in the settings dialog (to be merged)). Or, given that Quaternion is for technical audience anyway, just document how to override the theme with generic means (-style and QT_STYLE_OVERRIDE).

And what if Quaternion is packed with Snap another day?

Currently, the recommended way for snapping is kde-neon extension. Their wrapper already sets a theme.

Good to know, thanks.

@ilya-fedin
Copy link
Contributor Author

with no means to revert to the Qt's default

There are a check if additional themes is installed (e.g. org.kde.KStyle.Adwaita). And if some theme is installed, then theme won't be set.

we can merely provide an easy and unobtrusive way to switch to Breeze from inside Quaternion

Hmm, just like Kdenlive:
image

@KitsuneRal
Copy link
Member

with no means to revert to the Qt's default

There are a check if additional themes is installed (e.g. org.kde.KStyle.Adwaita). And if some theme is installed, then theme won't be set.

What if there are no additional themes but the user just wants to use Fusion? This can be a case at Windows, e.g.

we can merely provide an easy and unobtrusive way to switch to Breeze from inside Quaternion

Hmm, just like Kdenlive:
image

Yeah, looks good to me.

@ilya-fedin
Copy link
Contributor Author

ilya-fedin commented Jul 3, 2020

This can be a case at Windows, e.g.

Windows doesn't have Breeze anyway...

@KitsuneRal
Copy link
Member

This can be a case at Windows, e.g.

Windows doesn't have Breeze anyway...

Ok, this is actually irrelevant if we talk about Flatpak. But Adwaita (any extra theme actually) may easily be missing exactly in cases you mentioned above - LXQt, WM, ion etc.

@ilya-fedin
Copy link
Contributor Author

Created a setting, didn't relocate inFlatpak since it is in #682 and I think it is better to rebase this PR when #682 will be merged

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

One nitpick, and yes let's put your other PR in first.

client/main.cpp Outdated
Comment on lines 103 to 104
using Quotient::SettingsGroup;
const auto useBreezeStyle = SettingsGroup("UI").value("use_breeze_style", inFlatpak()).toBool();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Quotient::SettingsGroup;
const auto useBreezeStyle = SettingsGroup("UI").value("use_breeze_style", inFlatpak()).toBool();
const auto useBreezeStyle = Quotient::Settings().get("UI/use_breeze_style", inFlatpak());

SettingsGroup is fine too but is only worth it when the same group is used multiple times (FWIW, it's been made as a generic base for AccountSettings). For one-off uses Settings comes slightly more concise and optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ilya-fedin
Copy link
Contributor Author

Rebased on the current master

@KitsuneRal KitsuneRal merged commit 67c6223 into quotient-im:master Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for Quaternion
Projects
Status: Version 0.0.95 - Done
Development

Successfully merging this pull request may close these issues.

2 participants