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

Fix #316507: Application now supports using the system theme option i… #7366

Closed

Conversation

sidharth-anand
Copy link
Contributor

Resolves: https://musescore.org/en/node/316507

This patch allows Windows users to choose the system theme option and changes the theme accordingly. (Works on versions later than Windows 10 build 1809, where dark theme was first introduced. On older builds, defaults to light theme which is the system default)

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

return data == 0 ? MuseScoreEffectiveStyleType::DARK_FUSION : MuseScoreEffectiveStyleType::LIGHT_FUSION;
else
return MuseScoreEffectiveStyleType::LIGHT_FUSION;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this uses the new indent style from master, not the old (banner, 6 spaces) 3.x one.

@@ -82,6 +82,9 @@ PreferenceDialog::PreferenceDialog(QWidget* parent)
if (CocoaBridge::isSystemDarkModeSupported())
styleName->addItem(tr("System"));
#endif
#ifdef Q_OS_WIN
styleName->addItem(tr("System"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this looks like an added string, but due to same context should reuse the existing translation, so really is not something that needs to get translated again

@@ -654,6 +657,9 @@ void PreferenceDialog::updateValues(bool useDefaultValues, bool setup)
if (CocoaBridge::isSystemDarkModeSupported())
styleName->addItem(tr("System"));
#endif
#ifdef Q_OS_WIN
styleName->addItem(tr("System"));
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

@cbjeukendrup
Copy link
Contributor

This is very nice! However, will it also automatically update when MuseScore is already running and then the user switches the theme in their Windows settings?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 29, 2021

I don't think so (and I don't think it does on Mac either), It should on next start though?

@cbjeukendrup
Copy link
Contributor

https://stackoverflow.com/a/41763234/11432719 might be helpful for implementing this.

@RobFog
Copy link

RobFog commented Jan 29, 2021

Is it realistic this will go into 3.x?

@sidharth-anand
Copy link
Contributor Author

sidharth-anand commented Jan 29, 2021

Nope it doesn't automatically update when MuseScore is running. It updates on the next start or if you switch to dark/light then switch back. Windows doesn't really provide a mechanism for listening to settings changes in c++ without using the native HWND window. It looks like it might be possible only for C# or UWP apps, and the link @cbjeukendrup sent suggests this too.

@cbjeukendrup
Copy link
Contributor

Is it realistic this will go into 3.x?

In a way, this is pretty safe. It only affects the case that the user sets the theme explicitly to Follow System, which is on Windows not the default (which I think is good). So I think it could well go into 3.6.2. And I can very easily port this to 4.x.

By the way, on Mac, it does automatically switch (see CocoaBridge.mm).

I still have hope that it is possible to implement automatic switching on Windows, but it may be tricky to figure it out.

Another question: isn't it a good idea to show the "Follow System" option only on versions of Windows that actually support dark theme? Something like this:

#ifdef Q_OS_WIN
      if (windows build >= 1809)
            styleName->addItem(tr("System"));

(in two places in prefsdialog.cpp)

@sidharth-anand
Copy link
Contributor Author

isn't it a good idea to show the "Follow System" option only on versions of Windows that actually support dark theme?

Getting the build number seems to be simpler than i thought it would be on Windows. I'll submit another patch doing that soon

@Jojo-Schmitz
Copy link
Contributor

Add it to this PR here

@sidharth-anand
Copy link
Contributor Author

Sure I'll update this PR then

@vpereverzev
Copy link
Member

I don't like the idea of fixing this for 3.x, but I admit it might be useful to port it to master. It is not known what pitfalls are hidden here.

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Feb 3, 2021

FWIW, in the meantime, I already created the master version, but am still puzzling about the automatically switching, so haven't created a PR yet.

And I'm also looking into a Linux/Ubuntu version. That seems also possible, but also a bit of a puzzle.

@vpereverzev
Copy link
Member

FWIW, in the meantime, I already created the master version, but am still puzzling about the automatically switching, so haven't created a PR yet.

And I'm also looking into a Linux/Ubuntu version. That seems also possible, but also a bit of a puzzle.

OK, cool. Then I can freely close this PR

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 this pull request may close these issues.

None yet

5 participants