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

System theme preference #1800

Merged
merged 30 commits into from
May 11, 2022

Conversation

vallode
Copy link
Contributor

@vallode vallode commented Oct 8, 2021


System theme preference

Pull Request Type

  • Feature Implementation

Related issue
Closes #691
Closes #2106
Closes #2092

Description
Adds a new theme "system" that will match your OS's preferred color scheme (light/dark) dynamically** (see additional notes)

Screenshots (if appropriate)
Please add before and after screenshots if there is a visible change.

Testing (for code that is not small enough to be easily understandable)
Open the app and switch your OS theme from light to dark and vice-versa to test the change.

Desktop (please complete the following information):

  • OS: [e.g. iOS] Debian
  • OS Version: [e.g. 22] 10
  • FreeTube version: [e.g. 0.8] 0.15.0

Additional context

**The dynamic nature of this currently does not work at all on my system (GTK 3 Gnome) but I am debugging it with some electron folks over at their discord server. Hoping this will be fixed soon but I am opening the PR to have it ready.

If you could test this out on Windows/Mac to see if the behaviour is already working there that'd be great :)

I'm also aware I need to add the fields to all translations, I just left it out until I can confirm things work

@PrestonN PrestonN enabled auto-merge (squash) October 8, 2021 06:42
auto-merge was automatically disabled October 8, 2021 06:44

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 8, 2021 06:44
@ChunkyProgrammer
Copy link
Member

Haven't looked at your PR yet but this might be useful to look at #1318 (comment)

auto-merge was automatically disabled October 9, 2021 08:49

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 9, 2021 08:49
auto-merge was automatically disabled October 9, 2021 08:50

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 9, 2021 08:51
@vallode
Copy link
Contributor Author

vallode commented Oct 9, 2021

This PR is ready, I need this to be tested on several platforms in order to confirm 100% but after looking through vscode source code and discussing with Electron folk it seems this solution is good enough for coverage of all platforms.

Needs to be checked on:

  • Linux (Tested on Debian 10 and Ubuntu 20.04)
  • MacOS
  • Windows

Ideally we'd check it on a few different OS versions. @ChunkyProgrammer @efb4f5ff-1298-471a-8973-3d47447115dc could you test on your respective platforms and let me know if this works?

Go to settings -> Change to "system" theme -> Use your operating system settings to change back/forth from dark mode -> app should flip between light/dark theme respectively

There is one known bug to me: If the system theme changes while the app is starting up there is a window where the app will not update the theme. This is due to the electron checks before registering the ipcRenderer listener. Not sure if there is a clean way of fixing it right now.

@efb4f5ff-1298-471a-8973-3d47447115dc

Will start reviewing within a few hours

@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: WIP labels Oct 9, 2021
Copy link
Contributor

@peepo5 peepo5 left a comment

Choose a reason for hiding this comment

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

Code looks good, but on my current PC setup my system theme preference is broken so I cannot test :<

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 9, 2021

Looks quite good to me. Tested on ubuntu 20.04.

One thing i would like to see changed tho is in this PR #1502

we introduced a black screen before the theme loads. Before that it was a white screen and when u had black theme enabled and started the app u would get flashbanged by the white flash so we opted to go for a black background so it wouldn't impact users that much. So it would be nice if the white background persisted if the light theme in system is enabled so the user doesnt see that weird black background for a sec. Im not sure if we can only change it tho.

I would also go for moving the system setting to the top in the dropdown. Sound more logical to me.

To keep the name uniform through the app please rename system to System Default

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 9, 2021
auto-merge was automatically disabled October 10, 2021 20:50

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 10, 2021 20:50
@peepo5
Copy link
Contributor

peepo5 commented Oct 10, 2021

use lint

auto-merge was automatically disabled October 11, 2021 06:42

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 11, 2021 06:43
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Oct 12, 2021
auto-merge was automatically disabled October 14, 2021 06:49

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) October 14, 2021 06:49
@vallode
Copy link
Contributor Author

vallode commented Oct 14, 2021

Coming back to this now, something weird is going on with localStorage persistence on my machine. I am unable to really test anything as the theme settings are being reset on every launch (tested on development and it happens to me there too). Any idea what might be causing this? Otherwise this PR is ready for another round of reviews.

@PrestonN PrestonN enabled auto-merge (squash) March 6, 2022 13:35
@vallode
Copy link
Contributor Author

vallode commented Mar 6, 2022

This is fixed now, took me a while to get it right but here is the gist of what was happening:

index.ejs had some hardcoded default classes on the <body> element, since in this PR that class was set to system there would be a momentary flash of system-styled background color (light or dark) before the checkThemeSettings() functions fired.

This was also combined with the fact that checkThemeSettings() fired multiple times during the initial mounting of App.js where the Vuex store was not loaded yet (getUserSettings).

Taking out the hardcoded values in the body element makes it transparent, so it takes whatever electron is setting as the background color. I also added a v-if statement to the actual app element itself for it to wait for the user settings to be loaded to avoid any future un-styled rendering.

That's about it, @efb4f5ff-1298-471a-8973-3d47447115dc cheers for finding that I think I was staring at the windows too much and it never occurred to me that it was happening! Always thankful for your testing!

Copy link
Member

Choose a reason for hiding this comment

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

LGTM! Apologies for the wait!

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested briefly locally, reviewed code
Only got 1 concern (see line comment)

src/renderer/store/modules/settings.js Show resolved Hide resolved
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

I guess we can always change it back if people complain...

@julianfairfax
Copy link

Seems like this is close to being done. I'm excited!

@vallode
Copy link
Contributor Author

vallode commented Apr 23, 2022

@PrestonN would you be able to take a look at this one, it's either ready to go or very close to. Just need some more eyes on it I guess. Cheers.

@efb4f5ff-1298-471a-8973-3d47447115dc

Hi @vallode apologies for the wait. Preston is unavailable this week, will be back next week. Our plan is to do a release with allot of fixes. After that release we will merge over all the PRs that are ready to go including this one. If u want we could maybe include this pr is our next release.

@vallode
Copy link
Contributor Author

vallode commented Apr 24, 2022

If u want we could maybe include this pr is our next release.

I'm biased to say this would be a great addition to a release, haha! Whatever you decide, I'm around to make sure I can do some fixes if/when they come up for the release.

No need to apologise at all! Looking forward to the release.

Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

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

Lgtm

@PrestonN PrestonN merged commit 3dcea52 into FreeTubeApp:development May 11, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 11, 2022
@vallode vallode deleted the system-theme-preference branch May 11, 2022 17:45
@PrestonN
Copy link
Member

Hi there @vallode.

I was looking through the project today and noticed that there was a small error occurring where it was trying to look for a setBodyTheme function. Looking into it more brought me over to this PR. Searching through the project in our Dev branch I didn't see any other instance of this function, so I swapped it out with our existing function for setting the theme. If the setBodyTheme function isn't needed any more then we're all good, but if it was needed for something related to this PR, then I ask that you please submit a new PR with the intended functionality. Everything still seems to be functional regardless so it isn't urgent, but I wanted to mention it here just in case.

Thanks!

@vallode
Copy link
Contributor Author

vallode commented May 18, 2022

We're all good @PrestonN, I had a look but I remember using setBodyTheme for an earlier implementation of this feature. Not needed and thanks for fixing my mistake!

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