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

Allow to choose Qt style #21553

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Allow to choose Qt style #21553

merged 3 commits into from
Oct 11, 2024

Conversation

glassez
Copy link
Member

@glassez glassez commented Oct 8, 2024

000

@glassez glassez added OS: Windows Issues specific to Windows Look and feel Affect UI "Look and feel" only without changing the logic GUI GUI-related issues/changes labels Oct 8, 2024
@glassez glassez requested a review from a team October 8, 2024 16:35
@glassez glassez added this to the 5.0.1 milestone Oct 8, 2024
@xavier2k6

This comment was marked as resolved.

@glassez glassez marked this pull request as draft October 8, 2024 17:07
@glassez

This comment was marked as resolved.

@glassez

This comment was marked as duplicate.

@xavier2k6

This comment was marked as resolved.

@glassez

This comment was marked as resolved.

@xavier2k6

This comment was marked as duplicate.

@glassez

This comment was marked as resolved.

@xavier2k6

This comment was marked as resolved.

@glassez

This comment was marked as duplicate.

@xavier2k6

This comment was marked as resolved.

@Pentaphon
Copy link

I think an upgrade to Qt 6.8 is needed before this can be merged.

@glassez

This comment was marked as duplicate.

@glassez
Copy link
Member Author

glassez commented Oct 9, 2024

I think an upgrade to Qt 6.8 is needed before this can be merged.

I don't see any reason for this yet...

@xavier2k6

This comment was marked as resolved.

@xavier2k6

This comment was marked as off-topic.

@xavier2k6

This comment was marked as resolved.

@glassez glassez marked this pull request as ready for review October 9, 2024 09:22
@glassez

This comment was marked as resolved.

@glassez
Copy link
Member Author

glassez commented Oct 9, 2024

Just tested the latest push with Qt 6.7.3, no blackness issue now
why it works on Qt 6.7.3....I wonder how Qt 6.7.1/6.7.2 behave...

Everything is obvious to me. Qt 6.7.0 had some kind of bug that was fixed in one of the subsequent updates. Which one, it doesn't matter to me, because on Windows there is no problem just using Qt 6.7.3, which has it fixed. If you're interested, you could find out more precisely.

If there are no more serious problems with this PR, I would prefer to end it. I don't want to spend too much time on this issue.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

Was it a consideration to have a "system default" entry? (which would allow qt to choose the style)

I initially had such an idea, but then I rejected it. Windows 11 uses the windows11 theme by default, which recently provided with the release of Qt 6.7, and hardly anyone has had enough time to test the use of qBittorrent with this theme.

In general, I would not recommend having such an option, at least if it just follows Qt default. We could have "default" option that enables qBittorrent own default option.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Oct 13, 2024

What I was thinking is slightly different (keep in mind that this setting is Windows-only anyway):
Combobox:

  • Fusion <------- our default
  • System <------- qt's default
  • <all other qtstyles available>

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

What I was thinking is slightly different (keep in mind that this setting is Windows-only anyway):

That's what I care about. Wouldn't an option like "System" or "Default" look like something preferred/recommended?

@sledgehammer999
Copy link
Member

Wouldn't an option like "System" or "Default" look like something preferred/recommended?

I think we can get around that by naming our default as Fusion (recommended).
Or having a text label after the combobox: "Hint: Darkmode works best with the Fusion style".

What do you think?

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

I think we can get around that by naming our default as Fusion (recommended). Or having a text label after the combobox: "Hint: Darkmode works best with the Fusion style".

What do you think?

I think such options set would be confusing under current circumstances.

Anyway, I don't want to pay too much of my attention to this topic. And I don't want it to slow down the release of version 5.0.1, which contains hotfixes of the most popular problems of v5.0.0.

@Pentaphon
Copy link

Pentaphon commented Oct 13, 2024

I think we can get around that by naming our default as Fusion (recommended).

Or just be straightforward and say Fusion (dark mode) and System (default) because clearly, Fusion isn't giving many people the dark mode they want. The text label should say "Fusion is the only current Qt style compatible with Windows dark mode, for now".

As long as we get users a way to get out of dark mode while being informative, this issue is over and done with.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

The text label should say "Fusion is the only current Qt style compatible with Windows dark mode, for now".

This is not true. "Windows11" style also supports it, AFAIK.

@Pentaphon
Copy link

Pentaphon commented Oct 13, 2024

The text label should say "Fusion is the only current Qt style compatible with Windows dark mode, for now".

This is not true. "Windows11" style also supports it, AFAIK.

So then we just need the Qt style dropdown to say:

  • System (default)
  • Fusion (dark mode)
  • Windows 11 (dark mode)

Along with a text label that says "Dark mode on Windows only works with dark mode compatible Qt styles".

Nothing else is necessary as long as we inform users of what each style can do.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

So then we just need the Qt style dropdown to say:

  • System (default)
  • Fusion (dark mode)
  • Windows 11 (dark mode)

Along with a text label that says "Dark mode on Windows only works with dark mode compatible Qt styles".

Nothing else is necessary as long as we inform users of what each style can do.

System (default) still confuses. Is it "dark mode" compatible or not? Is it preferred/recommended? If it is "default" why is it not used by default?

BTW, Windows style is also dark mode compatible, AFAIK.
I wouldn't pollute combo box with such auxiliary info. If you want you can add text label with all such info.

@xavier2k6
Copy link
Member

We are not changing the default style in Qt on Windows, so by default, your application will use the Windows Vista style, which will use the light system palette even on a Windows 11 system running with a dark theme. The window framing will be consistent with the application palette.

Re-read https://www.qt.io/blog/dark-mode-on-windows-11-with-qt-6.5

Styles below will follow the Windows system settings for dark "app" mode automatically as of Qt 6.5+

  • Fusion
  • Windows
  • Windows 11

The problem currently for some users is that even though they have enabled dark "app" mode they don't want qBittorrent to respect that & want it to have a light pallette "Windows Vista style" aka the way it behaved in <=4.6.x until we changed to Fusion

So, what we've enabled through Qt is an "auto" switch but some users want:

  1. An actual "manual" switch too, like: https://www.qt.io/blog/qt-6.8-released#:~:text=Qt%20GUI
  2. An option to revert to previous theme or choose another (this PR does that)
  3. Correct handling of accent color on Windows.

@Pentaphon
Copy link

Pentaphon commented Oct 13, 2024

System (default) still confuses. Is it "dark mode" compatible or not? Is it preferred/recommended? If it is "default" why is it not used by default?

  1. That is why it is important to give the users a string of information to inform them instead of just giving them a new combobox with no context. I'm fine with removing the auxiliary info as long as there is a text label that shows users which options work with dark mode.
  2. We should use system (default) by default because most people seem to want out of this lackluster dark mode while still giving people who want it the option to use it. That's the main issue people are complaining about.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Oct 13, 2024

And I don't want it to slow down the release of version 5.0.1, which contains hotfixes of the most popular problems of v5.0.0.

I don't want this backported to 5.0.1 as a potentially incomplete solution, because we are still discussing its options.

I think our best option is a text label. A revised text could be "Hint: Fusion style is recommended. It has the better compatibility with Windows dark mode."
This doesn't indicate that Fusion has the only compatibility with dark mode.

@Pentaphon If I am interpreting correctly your commetns you essentially advocate in also changing the default style from Fusion to qt's, right? I am not entirely convinced that this is a good solution.

@xavier2k6
Copy link
Member

BTW, although windowsvista style doesn't support dark "app" mode - it does support high contrast themes, like the others:

Screenshot 2024-10-13 141400

@Pentaphon
Copy link

Pentaphon commented Oct 13, 2024

I don't want this backported to 5.0.1 as a potentially incomplete solution, because we are still discussing its options.

This should go to 5.0.1 because a lot of people want to get out of using the dark mode that the Fusion theme enables. If you don't push this to 5.0.1, many people will keep complaining.

I think our best option is a text label. A revised text could be "Hint: Fusion style is recommended. It has the better compatibility with Windows dark mode."
This doesn't indicate that Fusion has the only compatibility with dark mode.

Yes but I would say: "Hint: Fusion style is recommended for best compatibility with Windows dark mode."

That's all that needs to be said.

if I am interpreting correctly your commetns you essentially advocate in also changing the default style from Fusion to qt's, right? I am not entirely convinced that this is a good solution.

It does not matter what we use as default. We can use Fusion or any other Qt style by default as long as we have:

  1. The combobox to allow users to change away from Fusion to another Qt style (already done)
  2. Some way to inform users what each combobox option does, like a simple string of text saying which one best supports dark mode. That is all that is left to do.

This should not be hard to figure out. There are more important bugs to fix as @glassez points out.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

2. simple string of text saying which one best supports dark mode.

If all styles except single one support dark mode, then it looks wasteful to tag them all. It's enough to mention in your text label that WindowsVista doesn't support dark mode, and that's it.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

it does support high contrast themes, like the others:

IIRC, when high contrast is enabled Windows style is just forced.

@glassez
Copy link
Member Author

glassez commented Oct 13, 2024

I don't want this backported to 5.0.1 as a potentially incomplete solution, because we are still discussing its options.

👎
This already contains a solution to the problem that someone can use all the time while these subjective discussions continue.

IMO, all these explanations in the combobox itself can only make it more confusing, as well as the System (default) option, which is essentially neither system nor default. A separate label explaining that WindowsVista style does not support "dark mode" may make sense, and that's it.

I expressed my opinion. And then do as you like.

@stalkerok
Copy link
Contributor

IMO, what is already in place is enough.

sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 13, 2024
@sledgehammer999
Copy link
Member

Please see/continue in #21605

sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 13, 2024
sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 13, 2024
@Pentaphon
Copy link

  1. simple string of text saying which one best supports dark mode.

If all styles except single one support dark mode, then it looks wasteful to tag them all. It's enough to mention in your text label that WindowsVista doesn't support dark mode, and that's it.

Yes, if that is the case then that is perfect.

sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 13, 2024
sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 14, 2024
sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 14, 2024
sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 14, 2024
sledgehammer999 added a commit to sledgehammer999/qBittorrent that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI GUI-related issues/changes Look and feel Affect UI "Look and feel" only without changing the logic OS: Windows Issues specific to Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants