Properly calculate alwaysUse24HourFormat on MacOS #53795

kpsroka · 2024-07-10T10:28:27Z

Moves the implementation if isAlwaysUse24HourFormat from iOS's FlutterViewController internals to common utility, and makes use of it on MacOS in order to return correct value of alwaysUse24HourFormat.

This PR partially resolves #32006.

Note that on iOS 16+ and MacOS 13+, there is a new API for obtaining this information: https://developer.apple.com/documentation/foundation/locale/components/3952289-hourcycle. However, to keep things simpler, I wanted to not include changes to the logic.

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 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

… makes MacOs use it

flutter-dashboard · 2024-07-11T22:20:58Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

knopp

LGTM. This seems like a straightforward refactor. Not sure if this needs a test since it only moves things from iOS to darwin embedder. Also I'm not sure how to test this given that the behavior depends on system settings. @cbracken, any ideas?

cbracken · 2024-07-12T18:30:52Z

Also I'm not sure how to test this given that the behavior depends on system settings.

In the usual case, we'd say extract out a thin utility class that wraps the system setting and then write a test of the usage site against a mock. ~~In this case, this is just a refactoring that factors out the thin wrapper that wraps the system setting.~~ Since this also makes use of this in the mac embedding, we should add a test there.

You could write a test against the existing usage that just tests that the value set in the FlutterHourFormat is the value that's used. Probably not too much effort, ~~but I think a case could also be made for requesting a test exemption.~~

cbracken

Looks great! Thanks for the patch!

shell/platform/darwin/common/framework/Headers/FlutterHourFormat.h

stuartmorgan-g · 2024-07-16T22:58:00Z

In this case, this is just a refactoring that factors out the thin wrapper that wraps the system setting.

This isn't just a refactoring though; the purpose of this PR is to change the behavior of the macOS embedding. That's a behavioral change that should not regress, and thus should be tested.

(In this case I would suggest a test that onSettingsChanged: call the channel with expected values, probably using an injected formatter as cbracken suggested.)

zanderso · 2024-07-18T20:09:40Z

From PR triage: It looks like this PR is waiting for a test to be added, correct?

kpsroka · 2024-07-18T21:21:09Z

From PR triage: It looks like this PR is waiting for a test to be added, correct?

Yes, I will get to that over the weekend.

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm

shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm

cbracken

Overall looks good, just a few nits and suggestions.

kpsroka · 2024-08-01T09:50:55Z

I think that the PR is ready for merge. @cbracken please merge/add "autosubmit" [if you agree].

kpsroka · 2024-08-01T09:51:38Z

Note that flutter/flutter#32006 is best left [re-]opened, and updated to refer only to textScaleFactor

cbracken · 2024-08-01T18:51:37Z

Thanks for the fix, @kpsroka! Merged!

…engine#53795)

…152703) flutter/engine@230879c...17e3c7d 2024-08-01 [email protected] Properly calculate alwaysUse24HourFormat on MacOS (flutter/engine#53795) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#152703) flutter/engine@230879c...17e3c7d 2024-08-01 [email protected] Properly calculate alwaysUse24HourFormat on MacOS (flutter/engine#53795) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

kpsroka added 2 commits July 10, 2024 12:06

Moves implementation of isAlwaysUse24HourFormat to darwin/common, and…

951828f

… makes MacOs use it

Adding whitespace for style

6651c30

github-actions bot added platform-ios affects: desktop platform-macos labels Jul 10, 2024

kpsroka marked this pull request as ready for review July 10, 2024 11:16

kpsroka force-pushed the alwaysUse24Hour-macos-32006 branch from c347a3a to b29181f Compare July 10, 2024 11:22

Updates licenses_golden/licenses_flutter

99313d2

kpsroka force-pushed the alwaysUse24Hour-macos-32006 branch from b29181f to 99313d2 Compare July 10, 2024 11:24

CI, please do licenses_flutter magic now

f16f37e

kpsroka force-pushed the alwaysUse24Hour-macos-32006 branch from f1ccbb5 to f16f37e Compare July 10, 2024 12:29

kpsroka added 2 commits July 10, 2024 16:06

It's the final licenses_flutter update

47fb9fc

Merge branch 'main' into alwaysUse24Hour-macos-32006

52ac771

knopp self-requested a review July 12, 2024 10:10

knopp approved these changes Jul 12, 2024

View reviewed changes

cbracken approved these changes Jul 12, 2024

View reviewed changes

stuartmorgan-g reviewed Jul 16, 2024

View reviewed changes

shell/platform/darwin/common/framework/Headers/FlutterHourFormat.h Show resolved Hide resolved

kpsroka added 6 commits July 21, 2024 15:42

First attempt at testing

7ff9ed5

Second attempt at testing

b95f96b

Merge branch 'main' into alwaysUse24Hour-macos-32006

18dbf64

Is this real?

beda225

This is it?!

b43f104

Merge branch 'main' into alwaysUse24Hour-macos-32006

9749f90

cbracken reviewed Jul 30, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm Outdated Show resolved Hide resolved

cbracken reviewed Jul 30, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm Outdated Show resolved Hide resolved

cbracken reviewed Jul 30, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm Outdated Show resolved Hide resolved

cbracken reviewed Jul 30, 2024

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm Outdated Show resolved Hide resolved

cbracken approved these changes Jul 30, 2024

View reviewed changes

kpsroka added 3 commits July 31, 2024 10:28

Tests that actually test (iOS)

f82a310

Better tests for macos

7f095f0

Removing unused @Property

e53e8f0

cbracken approved these changes Aug 1, 2024

View reviewed changes

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2024

cbracken mentioned this pull request Aug 1, 2024

Share the alwaysUse24Hour and textScaleFactor heuristics between iOS and macOS flutter/flutter#32006

Closed

2 tasks

auto-submit bot merged commit 17e3c7d into flutter:main Aug 1, 2024

engine-flutter-autoroll mentioned this pull request Aug 1, 2024

Roll Flutter Engine from 230879c7936e to 17e3c7d520e7 (1 revision) flutter/flutter#152703

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 1, 2024

17e3c7d52 Properly calculate alwaysUse24HourFormat on MacOS (flutter/…

13c617f

…engine#53795)

kpsroka deleted the alwaysUse24Hour-macos-32006 branch August 2, 2024 15:49

Properly calculate alwaysUse24HourFormat on MacOS #53795

Properly calculate alwaysUse24HourFormat on MacOS #53795

Uh oh!

Conversation

kpsroka commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jul 11, 2024

Uh oh!

knopp left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented Jul 18, 2024

Uh oh!

kpsroka commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

kpsroka commented Aug 1, 2024

Uh oh!

kpsroka commented Aug 1, 2024

Uh oh!

cbracken commented Aug 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kpsroka commented Jul 10, 2024 •

edited

Loading

cbracken commented Jul 12, 2024 •

edited

Loading

stuartmorgan-g commented Jul 16, 2024 •

edited

Loading