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

Add a command line option for switching themes #918

Closed
4 tasks done
sahinakkaya opened this issue Oct 31, 2022 · 8 comments
Closed
4 tasks done

Add a command line option for switching themes #918

sahinakkaya opened this issue Oct 31, 2022 · 8 comments

Comments

@sahinakkaya
Copy link

sahinakkaya commented Oct 31, 2022

Checklist

Description

Setting the gui theme to "System settings" doesn't work for me (Qtile on Arch). So I wonder if it is possible to add a new command line option to control the theme outside of the program so that I can automate it via scripting

$ streamlink-twitch-gui --theme Light
$ streamlink-twitch-gui --theme Dark
@sahinakkaya sahinakkaya changed the title Add a runtime command line option for switching themes Add a command line option for switching themes Oct 31, 2022
@bastimeyer
Copy link
Member

If you have selected the system settings color theme, then NW.js/Chromium chooses the color scheme automatically for you via the prefers-color-scheme CSS media query, whose supported values are no-preference, light and dark. Streamlink Twitch GUI's ThemeService checks whether any of those values match the media query, and the matching value gets a change event listener attached to the MediaQueryList object that reacts to changes made to your system settings, as long as it's supported by Chromium which needs to have support implemented for various OSes, DEs and WMs.

Adding a command line parameter for choosing/changing the theme is fairly simple. The main issue I have with this though is that it'd be a feature just for the lack of support in Chromium. But considering that
https://bugs.chromium.org/p/chromium/issues/detail?id=998903
is still not resolved by the Chromium devs, adding a --theme=value parameter sounds reasonable.

@sahinakkaya
Copy link
Author

sahinakkaya commented Nov 1, 2022

@bastimeyer Thank you for the detailed response! You can't imagine how much your comment helped me. Because I do understand now why some of my apps does not follow system theme. I hope Chromium devs fix this so everybody can be happy. It is not Streamlink Twitch GUI's problem, so I would understand even if you don't add the flag. It would be great if you do though.

@bastimeyer
Copy link
Member

I've just noticed after locally implementing this feature that there's a regression in NW.js which prevents this from being merged into master.

Apparently, recent NW.js versions don't fire the app's open event more than once anymore, which means that the theme could only be changed once during runtime.

Another big issue is that NW.js messes with the argument order when passing the argument string to the running process, and this breaks the argument parser. I had already applied some custom fixes for that a long time ago, but it looks like this workaround needs another update. I would much more prefer a real bugfix in NW.js.

So until at least the first issue gets fixed in NW.js, it doesn't make sense merging the --theme parameter feature. Let's leave this issue open, though.

I've opened a bug report on the NW.js repo here:
nwjs/nw.js#7991

@sahinakkaya
Copy link
Author

Another big issue is that NW.js messes with the argument order when passing the argument string to the running process, and this breaks the argument parser. I had already applied some custom fixes for that a long time ago

@bastimeyer I actually examined those lines yesterday to find out if I can do something myself. I am aware that the argument order is not preserved. And the solution was to accept only one parameter at a time. Doesn't this work now?

I don't know much JS, so don't get me wrong. I am asking this just out of curiousity 🙂

@bastimeyer
Copy link
Member

Doesn't this work now?

The workaround just needs a small update. As said, I would much rather prefer a real fix in NW.js instead of having to deal with this mess.

You can build from the feature/theme-parameter branch if you want, but as said, it won't be useful because of the open event which only triggers once.

@sahinakkaya
Copy link
Author

But considering that https://bugs.chromium.org/p/chromium/issues/detail?id=998903 is still not resolved by the Chromium devs, adding a --theme=value parameter sounds reasonable.

Looks like it is fixed now. 🥳

When the patch comes, will theme switch work properly?

@bastimeyer
Copy link
Member

bastimeyer commented Apr 10, 2023

Yes, I'm already aware. Finally fixed in Chromium and all Chromium-based applications... 🎉

CRBUG#998903 will be fixed in Chromium 114 though, which hasn't been released yet. Its stable release day is 2023-05-30.
https://chromiumdash.appspot.com/schedule

After that, NW.js needs to be updated and a new version be released. Once that's out, I'll have to test that release before I'm able to bump the version here.
https://nwjs.io/blog/

And once that's done, I can merge the branch and close this issue.

I was thinking about publishing a new release in the coming days, since it's been a while, despite not many new features and bugfixes, but with this annoying issue finally being fixed in Chromium in the coming weeks, I will probably delay the release until that's done.

@bastimeyer
Copy link
Member

master now uses NW.js 0.77.0 (Chromium 114) with support for proper prefers-color-scheme CSS media query results on Linux.

I've also updated what I had pushed on the feature/theme-parameter branch. The command line string which gets passed to the running NW.js instance is still a total mess though, but I was able to apply a new workaround, so the theme can be overridden via the --theme value or --theme=value CLI argument. The branch is now merged into master and has been deleted.
d7619e7...984b8f5

Closing...

New Streamlink Twitch GUI release once the recent Streamlink issues around the integrity token have been resolved, which I've been working on the past couple of days.

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

No branches or pull requests

2 participants