[macOS] Change view ID to signed #39958

dkwingsmt · 2023-02-28T20:56:51Z

This PR makes view ID signed from unsigned int64.

Initially, I made view IDs unsigned because they were opaque anyway. As I'm working deeper into multiview, I found some issues that made me think signed is better:

Unsigned integers are worse
- Sometimes you want negative values to represent special values.
- Unsigned integers are dangerous (if compared with signed ones by mistake.)
Unsigned integers are not needed
- We're very unlikely to reach that big anyway.
- Almost all other languages support only signed integers.
- Also JavaScript only supports up to 51 bits of integer.

Therefore I think it's better to change them to signed int64, especially before these APIs are widely used by developers.

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.

loic-sharma · 2023-02-28T23:01:17Z

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

@@ -644,7 +644,7 @@ - (nonnull NSView*)createWithViewIdentifier:(int64_t)viewId arguments:(nullable
  @autoreleasepool {
    // Create FVC1.
    viewController1 = [[FlutterViewController alloc] initWithProject:project];
-    EXPECT_EQ(viewController1.id, 0ull);
+    EXPECT_EQ(viewController1.id, 0ll);


Should this be just 11? Same comment for other occurrences in this file

First, if this should be 1l, then the tests wouldn't pass. :)

Second, all tests here are for the current single-view uses, which only uses the default view. IDs starting from 1 are only for multiviews.

Oh I'm sorry, those are Ls not 1s. Please ignore this comment, I should check my eye prescription.

loic-sharma

LGTM

cbracken

Sometimes you want negative values to represent special values.

Can you give an example of such a case? Should we be using a different type to represent view IDs if so?

Unsigned integers are dangerous (if compared with signed ones by mistake.)

One could say the same for signed integers, if compared with unsigned ones by mistake ;-)

All that said, an argument that I really do like in favour of signed integers is that that's what we use for the following other identifiers:

external texture
platform view
semantics node

cbracken

Please define something like typedef int64_t FlutterViewIdentifier or similar rather than using int64_t in the public API.

dkwingsmt · 2023-03-01T08:05:26Z

@cbracken That's a good idea, but I find it hard to find a place to put it. This type (as well as kFlutterDefaultViewId, which I'll probably put into that header file as well) is used in both public headers (such as FlutterEngine.h) and the leaf classes (such as FlutterView.h), both of which requires minimal includes. The only option I can think of is a new public header. If so, what do you think I should call it? Common.h?

chinmaygarde · 2023-03-09T21:12:38Z

Are we making progress on this?

cbracken

This reverts commit 1212658.

Reverts #39958 Failing the roll to the framework starting with flutter/flutter#123893 One example: https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20dart_plugin_registry_test/21819/overview

This is a reland of #39958, but only contains the minimal changes that removes all references to viewId from public interfaces, reverting these changes from #39576. Flutter doesn't really support multi-view anyway. These methods currently only works for view 0. We better remove them until we mark multi-view stable. It was a mistake that I decided to add them to public interfaces. For the reason why #39958 failed, I'm pretty sure it's the change to [platform_dispatcher.dart](https://github.com/flutter/engine/pull/39958/files#diff-57d6953e215d0e5dd7260ee60b665c812aa730566ef0b30f7ff1e3d661143585) that mysteriously and unintentionally appeared in the change list. ## 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

dkwingsmt added 2 commits February 28, 2023 12:52

Impl

63b00fd

Merge remote-tracking branch 'origin/main' into change-view-id-to-signed

826d5ec

flutter-dashboard bot added the platform-macos label Feb 28, 2023

Format

6ec81fc

dkwingsmt requested review from cbracken, goderbauer and loic-sharma February 28, 2023 22:00

dkwingsmt marked this pull request as ready for review February 28, 2023 22:00

loic-sharma reviewed Feb 28, 2023

View reviewed changes

loic-sharma approved these changes Feb 28, 2023

View reviewed changes

cbracken reviewed Mar 1, 2023

View reviewed changes

cbracken requested changes Mar 1, 2023

View reviewed changes

WIP

4e990ab

loic-sharma added the affects: desktop label Mar 23, 2023

cbracken approved these changes Mar 30, 2023

View reviewed changes

dkwingsmt added 2 commits March 30, 2023 14:05

Merge remote-tracking branch 'origin/main' into change-view-id-to-signed

5b7d1c7

Fix web.PointerData

bea9f46

dkwingsmt merged commit 1212658 into flutter:main Mar 31, 2023
39 checks passed

dkwingsmt deleted the change-view-id-to-signed branch March 31, 2023 19:11

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to 12126583a727 (2 revisions) flutter/flutter#123893

Closed

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

02d3514

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to 69f1fa283b22 (3 revisions) flutter/flutter#123894

Closed

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

7aada1f

zanderso added a commit that referenced this pull request Mar 31, 2023

Revert "[macOS] Change view ID to signed (#39958)"

6690c96

This reverts commit 1212658.

zanderso mentioned this pull request Mar 31, 2023

Revert "[macOS] Change view ID to signed" #40829

Merged

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to a38655743250 (4 revisions) flutter/flutter#123897

Closed

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

7d7d5fa

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to c21cecd8c6cd (5 revisions) flutter/flutter#123898

Closed

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

567e672

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to b83d7dc6757c (6 revisions) flutter/flutter#123901

Closed

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

a7cf307

engine-flutter-autoroll mentioned this pull request Mar 31, 2023

Roll Flutter Engine from a10990314872 to b2855e074deb (7 revisions) flutter/flutter#123902

Merged

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

12126583a [macOS] Change view ID to signed (flutter/engine#39958)

d97c879

This was referenced May 1, 2023

[macOS] Change view ID's type to signed and a typedef #41653

Merged

[macOS] Remove view ID from public #41712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macOS] Change view ID to signed #39958

[macOS] Change view ID to signed #39958

dkwingsmt commented Feb 28, 2023 •

edited

loic-sharma Feb 28, 2023

dkwingsmt Feb 28, 2023

loic-sharma Mar 1, 2023 •

edited

loic-sharma left a comment

cbracken left a comment

cbracken left a comment

dkwingsmt commented Mar 1, 2023 •

edited

chinmaygarde commented Mar 9, 2023

cbracken left a comment

[macOS] Change view ID to signed #39958

[macOS] Change view ID to signed #39958

Conversation

dkwingsmt commented Feb 28, 2023 • edited

Pre-launch Checklist

loic-sharma Feb 28, 2023

Choose a reason for hiding this comment

dkwingsmt Feb 28, 2023

Choose a reason for hiding this comment

loic-sharma Mar 1, 2023 • edited

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

dkwingsmt commented Mar 1, 2023 • edited

chinmaygarde commented Mar 9, 2023

cbracken left a comment

Choose a reason for hiding this comment

dkwingsmt commented Feb 28, 2023 •

edited

loic-sharma Mar 1, 2023 •

edited

dkwingsmt commented Mar 1, 2023 •

edited