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

Conversation

@moko256
Copy link
Contributor

@moko256 moko256 commented Oct 24, 2021

This PR adds SettingsPlugin to add theme change support in win32.

In the #28131 and flutter/flutter#88520, the top-level window message is used to notify brightness change, but I found I cannot use with the same pattern to notify text scale factor change.
In this PR, I separate sending flutter/settings as SettingsPlugin, remove the functions to notify brightness change from out of engine, and add watching brightness change in SettingsPlugin with registry when win32.

This PR will remove some public functions added in #28131, but that is only used in flutter/flutter#88520 that is not merged, so I think it does not cause large effect.

This PR will close flutter/flutter#88520.
This PR will close flutter/flutter#98355.

No changes in flutter/tests.

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. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@google-cla google-cla bot added the cla: yes label Oct 24, 2021
@moko256 moko256 marked this pull request as ready for review October 24, 2021 08:34
@moko256 moko256 force-pushed the moko256_windows_settings_plugin branch from d9a094d to 57a8f24 Compare October 25, 2021 14:50
@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:13
@godofredoc godofredoc changed the base branch from master to main November 24, 2021 07:12
@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@moko256 moko256 force-pushed the moko256_windows_settings_plugin branch from 57a8f24 to 4dbd568 Compare February 17, 2022 22:09
@moko256
Copy link
Contributor Author

moko256 commented Feb 18, 2022

I resolved the conflicts and this is ready for review.

@moko256 moko256 force-pushed the moko256_windows_settings_plugin branch 2 times, most recently from b2853ca to e9d5973 Compare March 13, 2022 21:16
@moko256 moko256 force-pushed the moko256_windows_settings_plugin branch from e9d5973 to 42b8e2a Compare March 21, 2022 18:58
@cbracken cbracken self-requested a review April 28, 2022 18:04
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Overall, this looks great. Thank you for the patch.

LGTM stamp from a Japanese personal seal

@cbracken cbracken requested a review from dkwingsmt April 28, 2022 21:14
@moko256 moko256 force-pushed the moko256_windows_settings_plugin branch from 42b8e2a to c5d1d01 Compare April 28, 2022 21:36
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM, very clean code!

There seem to be a lack of tests for SettingsPluginWin{32,Uwp} but I don't see much to be tested considering they're basically just connecting system functions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows][Desktop]: WidgetsBindingObserver is not working.

4 participants