Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Linux] fix and test light vs. dark theme detection #32618

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

jpnurmi
Copy link
Member

@jpnurmi jpnurmi commented Apr 12, 2022

This PR replaces the unreliable (and untestable) window text color
-based theme detection with the simpler alternative proposed in
#25535 to check for a "-dark" suffix in the theme name,
which is used by convention in dark GTK theme names (Yaru vs. Yaru-dark,
Adwaita vs. Adwaita-dark, Pop vs. Pop-dark, ...).

Partial fix to: flutter/flutter#101438

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

Before

Personally, I was able to reproduce the issue in a Pop!_OS 20.04 VM

  • Detects correct theme on startup
  • First theme change does nothing
  • Consequent theme changes detect the opposite theme
popos-before.mp4

After

Same VM with the fix applied

popos-after.mp4

@jpnurmi
Copy link
Member Author

jpnurmi commented Apr 12, 2022

@robert-ancell and @cbracken, this is a spin-off of #32456 that I had trouble adding tests for. I have done some refactoring around FlSettingsPlugin to make it possible to test these fixes against the different GSettings desktop schemas in Ubuntu 20.04 vs. Ubuntu 22.04. The latter has GNOME 42 that includes the new dark style preference.

P.S. I have included it all here for now for feedback but if you think the changes look good in principle, I can start splitting this into smaller PRs.

@jpnurmi
Copy link
Member Author

jpnurmi commented Apr 12, 2022

These changes would also lay the foundations for implementing accessibility features for Linux, to make Flutter respect the high-contrast and disabled animations accessibility settings in GNOME.

@robert-ancell
Copy link
Contributor

Changes make sense to me - let's split into smaller PRs.

jpnurmi added a commit to jpnurmi/engine that referenced this pull request Apr 13, 2022
This follows the common naming convention for fake implementations,
and makes room for a GMock-based mock implementation that registers
calls and allows settings expectations.

Split off from flutter#32618.
jpnurmi added a commit to jpnurmi/engine that referenced this pull request Apr 13, 2022
This follows the common naming convention for fake implementations,
and makes room for a GMock-based mock implementation that registers
calls and allows settings expectations.

This is a preparation step split off from flutter#32618 that
will eventually fix flutter/flutter#101438.
@jpnurmi
Copy link
Member Author

jpnurmi commented Apr 13, 2022

I started splitting off some changes but I didn't realize the bot would get mad at me for not including tests when adding code to testing/.

Some users have reported that toggling between light and dark system
theme results in Flutter detecting the opposite theme. This is because
the window text color, which is used to calculate the brightness, may
not have been yet refreshed when the theme change signal is emitted.

This commit replaces the unreliable (and untestable) window text color
-based theme detection with the simpler alternative proposed in
flutter#25535 to check for a "-dark" suffix in the theme name,
which is used by convention in dark GTK theme names (Yaru vs. Yaru-dark,
Adwaita vs. Adwaita-dark, Pop vs. Pop-dark, ...).

Ref: flutter/flutter#101438
@jpnurmi
Copy link
Member Author

jpnurmi commented Apr 19, 2022

I've rebased the remaining commits and left out the GNOME 42 color-scheme changes from this. Let's fix and test the "legacy" theme detection first.

FlSettings is an interface that provides desktop settings and notifies
changes, whereas FlSettingsPlugin merely communicates the settings to
the framework. This makes it straightforward to test both separately.
As a bonus, it will be easy to add FlSettings implementations for other
DEs too.
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

LGTM!

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 26, 2022
@fluttergithubbot fluttergithubbot merged commit 0b08b62 into flutter:main Apr 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 26, 2022
@jpnurmi jpnurmi deleted the settings branch April 26, 2022 21:15
jpnurmi added a commit to jpnurmi/engine that referenced this pull request Jun 14, 2022
Let MockBinaryMessenger internally manage the FlBinaryMessenger wrapper
instance and provide it via operator FlBinaryMessenger*() to avoid
having to create two objects in every test. The same technique was used
for MockSettings in flutter#32618.
jpnurmi added a commit to jpnurmi/engine that referenced this pull request Jun 14, 2022
Let MockBinaryMessenger internally manage the FlBinaryMessenger wrapper
instance and provide it via operator FlBinaryMessenger*() to avoid
having to create two objects in every test. The same technique was used
for MockSettings in flutter#32618.
jpnurmi added a commit to jpnurmi/engine that referenced this pull request Jun 14, 2022
Let MockBinaryMessenger internally manage the FlBinaryMessenger wrapper
instance and provide it via operator FlBinaryMessenger*() to avoid
having to create two objects in every test. The same technique was used
for MockSettings in flutter#32618.
robert-ancell pushed a commit that referenced this pull request Jun 20, 2022
Let MockBinaryMessenger internally manage the FlBinaryMessenger wrapper
instance and provide it via operator FlBinaryMessenger*() to avoid
having to create two objects in every test. The same technique was used
for MockSettings in #32618.
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
…ter#34029)

Let MockBinaryMessenger internally manage the FlBinaryMessenger wrapper
instance and provide it via operator FlBinaryMessenger*() to avoid
having to create two objects in every test. The same technique was used
for MockSettings in flutter#32618.
bryan-hoang added a commit to bryan-hoang/dotfiles that referenced this pull request Jan 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform-linux waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants