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

[Android] Convert int in Dart to long instead of int in java. #39476

Closed
wants to merge 2 commits into from
Closed

[Android] Convert int in Dart to long instead of int in java. #39476

wants to merge 2 commits into from

Conversation

0xZOne
Copy link
Member

@0xZOne 0xZOne commented Feb 8, 2023 •
edited
Loading

We should use the 64-bit to represent int in Dart.

As an example of an extreme case, if |viewId| is greater than 0x80000000 on the Dart side, the native code will throw the following exception:

image

See the below codes for more details:
https://github.com/flutter/flutter/blob/19dfde6989b14bd7dedbc50d7edceef4e59879a4/packages/flutter/lib/src/services/message_codecs.dart#L399-L407

Related issue: flutter/flutter#120256

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.

@0xZOne 0xZOne marked this pull request as draft February 8, 2023 04:00
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@0xZOne 0xZOne force-pushed the task/type_mismatch branch 10 times, most recently from e0fcb50 to 01bdf81 Compare February 9, 2023 06:56
@0xZOne 0xZOne marked this pull request as ready for review February 9, 2023 07:48
@0xZOne 0xZOne requested a review from stuartmorgan February 9, 2023 07:48
@chinmaygarde
Copy link
Member

xref the iOS version of this patch #39331

@0xZOne 0xZOne mentioned this pull request Feb 15, 2023
Open
4 tasks
@camsim99 camsim99 requested a review from reidbaker February 16, 2023 19:42
This was referenced Feb 16, 2023
Merged
Open
@0xZOne 0xZOne force-pushed the task/type_mismatch branch from c324b51 to 78b4ad9 Compare February 20, 2023 03:05
0xZOne
0xZOne commented Feb 20, 2023
View reviewed changes
shell/platform/android/io/flutter/plugin/platform/PlatformViewFactory.java
@@ -28,7 +28,7 @@ public PlatformViewFactory(@Nullable MessageCodec<Object> createArgsCodec) {
* null, or no arguments were sent from the Flutter app.
*/
@NonNull
public abstract PlatformView create(Context context, int viewId, @Nullable Object args);
public abstract PlatformView create(Context context, long viewId, @Nullable Object args);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a breaking change. Any advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hard-breaking every plugin that uses platform views is definitely a non-starter.

Is there some real problem that we need this change to solve? From what I see in the PR description and the issue, it's entirely a theoretical concern, rather than something that has actually caused problems. Presumably there are no Flutter apps that need more than several billion platform views over the life of the app, and I'm having a hard time imagining a scenario where some kind of non-sequential random mapping over more than the range of int would be required.

If this is all just theoretical, the easy solution would seem to be to throw if the value is too large, and document+assert on the Dart side that the value must be within the range of int.

Copy link
Contributor

Choose a reason for hiding this comment

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

(E.g., it's also the case that on the Dart side setSize on a platform view takes a Size, whose width and height are doubles, but I'm willing to bet that passing double.maxFinite for both would not end well. It's common for APIs to not accept every value that the types could allow for.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, This is not a problem in practice, and given the breaking change, I am also more inclined to the solution you proposed.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, exposing the |viewId| to the user doesn't seem to make much sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realized this was breaking. Stuart's suggestion seems like it is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realized this was breaking. Stuart's suggestion seems like it is a good idea.

Sorry, that's my mistake. The breaking change is not obvious, and I forgot to mark it at the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have submitted a new PR. Please review it when you have time. Thank you.

This comment was marked as spam.

Sign in to view

@0xZOne 0xZOne requested a review from reidbaker February 20, 2023 04:15
@0xZOne 0xZOne force-pushed the task/type_mismatch branch from 78b4ad9 to ccfedc8 Compare February 20, 2023 04:32
@0xZOne
Copy link
Member Author

0xZOne commented Feb 21, 2023

@reidbaker Please take another look when you have a moment. Thanks.

@0xZOne 0xZOne mentioned this pull request Feb 22, 2023
Merged
8 tasks
@chinmaygarde
Copy link
Member

Can we land this? @0xZOne is a part of the Flutter org so the presub requirements are satisfied.

stuartmorgan
stuartmorgan suggested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

The current version still has a breaking change to PlatformViewFactory, so can't be landed as is. Setting review state to capture that.

@0xZOne 0xZOne closed this Feb 27, 2023
@0xZOne
Copy link
Member Author

0xZOne commented Feb 27, 2023

Closing it as I have filed a new PR based on the review results.

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

@azooz12300099 azooz12300099 azooz12300099 left review comments

@stuartmorgan stuartmorgan stuartmorgan requested changes

@reidbaker reidbaker reidbaker approved these changes

Assignees
No one assigned
Labels
platform-android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0xZOne @chinmaygarde @stuartmorgan @reidbaker @azooz12300099