Rename default views to implicit views #43364

dkwingsmt · 2023-06-30T20:05:38Z

This PR renames a number of internal variables from "default view ID" to "implicit view ID", as we have pretty much settled on naming this mechanism. Some docs are also fixed accordingly.

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.

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

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h

dkwingsmt · 2023-07-06T05:39:01Z

I rewrote the doc entirely. Let me know what you think :)

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h

dkwingsmt · 2023-07-06T19:56:14Z

Also cc @goderbauer to take a look at the docs :)

shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h

goderbauer · 2023-07-07T16:51:55Z

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h

 *
- * The default view is a special view operated by single-view APIs.
+ * Flutter plans to support multiple views in the future. Although single-view


I feel a bit uncomfortable about making sooo many concrete promises about how this will work in the future without having an actual implementation to back this up. I would keep this a little lighter and stick to what this does today, maybe with just a one-sentence hint that at some point flutter will support multiple views and at some point after that people will eventually have to migrate away from this API after the proper deprecation period. As we implement more stuff, the doc can evolve then.

Spending 90% of the doc on describing what this may do at some unknown point in the future seems odd.

Yeah, that's why my initial PR included nothing but the current status of this property.

/** * The view displaying Flutter content. * * This method may return |nil|, for instance in a headless environment. */

What do you think of this one?

I like that more than making promises about the future 😝

I still think we should nudge users away from this property though. If we want to give minimal explanation, perhaps we could reconsider this suggestion: #43364 (comment)

The implicit view that displays Flutter content. This property is provided for backwards compatibility for apps that assume a single view. This will eventually be replaced by a multi-view API variant. This method may return |nil|, for instance in a headless environment or for apps that do not assume a single view.

@loic-sharma Currently there's nowhere to nudge users to. It's not about being minimal, but that everything it talks about is not there yet. Imagine if you're a user, you see this concerning comment that announces an upcoming deprecation, what can you do? You can not migrate, you just keep worrying until the official support is here?

The idea is that we should only document the current state, not predictions or promises.

Loïc's suggestion in #43364 (comment) also seems fine. It doesn't make any promises about HOW this may work in the future.

Ok, I'll use that one.

…licit

…nto rename-default-to-implicit

dkwingsmt · 2023-07-07T21:39:50Z

@loic-sharma @goderbauer Is this PR good to go?

loic-sharma

Looks good! 🎉

goderbauer

LGTM

…130195) flutter/engine@4ca6191...9006633 2023-07-08 [email protected] Make updating window metrics multi-view (flutter/engine#43366) 2023-07-08 [email protected] Rename default views to implicit views (flutter/engine#43364) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR renames a number of internal variables from "default view ID" to "implicit view ID", as we have pretty much settled on naming this mechanism. Some docs are also fixed accordingly. ## 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. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

Impl

3630458

github-actions bot added the platform-macos label Jun 30, 2023

loic-sharma reviewed Jun 30, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h Show resolved Hide resolved

Fix doc

3d86303

dkwingsmt requested a review from loic-sharma July 5, 2023 15:53

loic-sharma reviewed Jul 5, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h Outdated Show resolved Hide resolved

loic-sharma reviewed Jul 5, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h Outdated Show resolved Hide resolved

dkwingsmt added 2 commits July 5, 2023 22:18

Fix doc

a293b8e

Better doc

7699ee8

dkwingsmt requested a review from loic-sharma July 6, 2023 05:40

loic-sharma reviewed Jul 6, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Headers/FlutterPluginRegistrarMacOS.h Outdated Show resolved Hide resolved

goderbauer reviewed Jul 7, 2023

View reviewed changes

dkwingsmt and others added 4 commits July 7, 2023 10:41

Back to current

57e705e

Update FlutterPluginRegistrarMacOS.h

a71e5b5

Merge remote-tracking branch 'origin/main' into rename-default-to-imp…

bc67089

…licit

Merge remote-tracking branch 'dkwingsmt/rename-default-to-implicit' i…

2aaabb8

…nto rename-default-to-implicit

loic-sharma approved these changes Jul 7, 2023

View reviewed changes

goderbauer approved these changes Jul 7, 2023

View reviewed changes

dkwingsmt merged commit 68b5eeb into flutter:main Jul 8, 2023

engine-flutter-autoroll mentioned this pull request Jul 8, 2023

Roll Flutter Engine from 4ca619166c4a to 68b5eeb1ddaf (1 revision) flutter/flutter#130193

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2023

68b5eeb1d Rename default views to implicit views (flutter/engine#43364)

b36ffc5

engine-flutter-autoroll mentioned this pull request Jul 8, 2023

Roll Flutter Engine from 4ca619166c4a to 9006633571bb (2 revisions) flutter/flutter#130195

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2023

68b5eeb1d Rename default views to implicit views (flutter/engine#43364)

ced8ea2

Rename default views to implicit views #43364

Rename default views to implicit views #43364

Uh oh!

Conversation

dkwingsmt commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkwingsmt commented Jul 6, 2023

Uh oh!

Uh oh!

dkwingsmt commented Jul 6, 2023

Uh oh!

Uh oh!

Uh oh!

goderbauer Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jul 7, 2023

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkwingsmt commented Jun 30, 2023 •

edited

Loading

dkwingsmt Jul 7, 2023 •

edited

Loading

dkwingsmt Jul 7, 2023 •

edited

Loading