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

feature: darkmode ,darkreader,custom style refinement #339

Closed
xiaoyifang opened this issue Feb 9, 2023 · 15 comments · Fixed by #345
Closed

feature: darkmode ,darkreader,custom style refinement #339

xiaoyifang opened this issue Feb 9, 2023 · 15 comments · Fixed by #345

Comments

@xiaoyifang
Copy link
Owner

xiaoyifang commented Feb 9, 2023

when user choose a custom style ,the background is different in the following image
image

the above image is caused by removing the custom qt-* style.css.

This has caused some confusions. I think the combination of the darkmode ,darkreader,custom style should be considered more closely.

Such as :
1, put darkmode as a custom style in the dropdownlist(Windows only).
image
by this way,the darkmode will not conflict with custom style.
2, when user choose darkmode,

@xiaoyifang
Copy link
Owner Author

@shenlebantongying can you check on this

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 10, 2023

On Linux, we shouldn't override UI's background & text colour. It looks insanely good if users choose to use a global theme that is comfortable for reading.

image


On windows, I have a better idea: let the user choose an arbitrary background colour rather than yellow only. It will override both article's background and the side panels' background colour.

image

The article display style will only change the style of the widgets.

image

Pro: Satisfy all users
Con: One extra setting option 😅

If the dark mode is enabled, then the side panel's background colour overriding will be disabled.

If the dark reader mode is enabled, then the article view's background colour overriding will be disabled.

Some extra thoughts from serious Windows users maybe help.

@xiaoyifang
Copy link
Owner Author

xiaoyifang commented Feb 10, 2023

I think we should not grant users too much options.

On windows, I have a better idea: let the user choose an arbitrary background colour rather than yellow only. It will override both article's background and the side panels' background colour.

currently we have about 6 custom styles , if implemented the above feature ,we should have too much styles .
this will make the situation worse.

If the dark mode is enabled, then the side panel's background colour overriding will be disabled.

If the dark reader mode is enabled, then the article view's background colour overriding will be disabled.

on Windows ,this is much better. and dark reader mode can be remove . it should be enabled together with darkmode.

on Linux/Macos, darkreaer can be enabled automaticly when the system has enabled the dark mode.
which means ,the two options(darkmode ,darkreader) should not appear on Linux.

// Enable when the system color scheme is dark.
DarkReader.auto({
    brightness: 100,
    contrast: 90,
    sepia: 10
});

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 10, 2023

We shouldn't use the auto dark reader, because "light text on dark background" is unusable for some people with low vision.

The white text on the black background will become blurry for them. Using dark UI and light text in article view is a possible use case.

https://www.google.com/search?q=accessibility+light+text+on+dark+background

@xiaoyifang
Copy link
Owner Author

xiaoyifang commented Feb 10, 2023

The white text on the black background will become blurry for them. Using dark UI and light text in article view is a possible use case.

then ,on linux, just keep the dark reader mode. Let's users to choose .
and same behavior as this

If the dark reader mode is enabled, then the article view's background colour overriding will be disabled.

If the dark reader mode is not enabled ,the application's custom style should be enabled.

@shenlebantongying
Copy link
Collaborator

To prevent dark reader mode from messing with the yellow background, We can simply append <style> body .gdarticle { background: white; } </style> when the dark reader is enabled.

https://github.com/xiaoyifang/goldendict/blob/bd736d7f37a3993f92c3f3557be5a52e7386f92c/article_maker.cc#L152-L165


However, the "correct" solution is just one step away from using arbitrary background colour:

We will remove body { background: yellow } from the CSS files. When the dark reader is not used, body .gdarticle {background: #{custom_color}} will be appended to the CSS file.

https://github.com/xiaoyifang/goldendict/blob/bd736d7f37a3993f92c3f3557be5a52e7386f92c/article-style-st-classic.css#L7-L12

@xiaoyifang
Copy link
Owner Author

in screenshot ,the darkmode has not been enabled , a custom style has been choosen.
the custom style has stop to work.

I think 587aad1 can be reverted and make some minor changes to satisfy the custom style.

if custom style+darkmode ,the custom style will not applied. only apply dark mode.
if custom style has been choosen , custom style will take effect.

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 11, 2023

If we revert that, there will be glitches on linux & mac because the old custom .css isn't complete (only some styles are changed. If the user chooses another global qt theme, then some weird visual glitch will happen.).

In other words: the struggles of those issues:

goldendict/goldendict#719
goldendict/goldendict#679
#271 (comment)
#188 (comment)

They cannot figure out a nice solution because the old .css files only change some of the styles.

To avoid those glitches:

One path is to avoid theming UI elements as 587aad1 did.

Another path is fully theming everything, but this approach's maintenance cost is too high.

However, we can bring back those qt style sheets only for Windows.

@xiaoyifang
Copy link
Owner Author

xiaoyifang commented Feb 11, 2023

the glitches only occured when enabled dark mode.
when darkmode is enabled , do not apply the custom style can avoid these glitch.

when dark mode is not enabled ,they should also work.

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 11, 2023

There is no concept of "dark theme" for Linux. The theme can be any colour, like purple or green or orange.

I think the only reasonable path is to bring them back to Windows only.

Sample glitch of reverting the commit and using the Lingoes-blue (there is no "Dark mode" involved at all):

image

@xiaoyifang
Copy link
Owner Author

xiaoyifang commented Feb 12, 2023

There is no concept of "dark theme" for Linux.

I kind of confused.

Sample glitch of reverting the commit and using the Lingoes-blue (there is no "Dark mode" involved at all):

is the screenshort taken when both darkmode and lignoes-blue been enabled?
in such a case , I think only darkmode should be applied and do not apply lingoes-blue scheme.

@xiaoyifang
Copy link
Owner Author

maybe the goal can be split into several steps:
1, make darkmode and custom style can work on Windows.
2, keep the linux ,macos unchanged for now before a new solution come up.

@shenlebantongying
Copy link
Collaborator

There is no concept of "dark theme" for Linux.
I kind of confused.

Linux users may use any color. The black background is only one of the possibilities. Check the screen below: I can install Green/Blue/Brown backgrounds, and they are not "dark".

In fact, the Qt's style engine on Linux is replaceable,

Such as Kvantum which uses SVG to theme Qt widgets, a theme is just a SVG picture.

There is also QtCurve which is built on top of GTK so that Qt and GTK apps look exactly the same. The theme will be determined by the user's GTK theme.

Thus, there is no concept of "dark theme" on Linux.

So, I think the the best solution for Linux users is don't hard code any colours. Just don't touch the defaults unless necessary.

image

@xiaoyifang
Copy link
Owner Author

So, I think the the best solution for Linux users is don't hard code any colours. Just don't touch the defaults unless necessary

Does this mean on linux the custom style can be removed?

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Feb 12, 2023

Yes, and we already removed them. We just bring them back to Windows Only. #272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants