Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AndroidView offset and resize #99888

Merged
merged 17 commits into from
Mar 14, 2022
Merged

Conversation

blasten
Copy link

@blasten blasten commented Mar 10, 2022

Fixes AndroidView by correcting how the texture is resized and setting the right offset, so a11y highlights
are drawn.

This PR replaces #97628

Engine PR: flutter/engine#31198
Related Issue: #96679

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 10, 2022
@flutter flutter deleted a comment from flutter-dashboard bot Mar 10, 2022

@override
Future<void> setSize(Size size) async {
Future<Size> setSize(Size size) async {
Copy link
Contributor

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. Can we just add a new method to get the updated size?

Copy link
Author

Choose a reason for hiding this comment

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

This is necessary. When someone overrides setSize, they do it for testing purposes.
e.g. to provide a fake implementation of this controller. If there's a new method that makes calls over the channel, then failures are expected as a result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would it be necessary, can we have an ivar to save it after calling void setSize, and a getter to get the value of that ivar?

It is also a little confusing to get a new Size object when you setSize.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we query all flutter projects to make sure there's no subclasses of AndroidViewController that overrides setSize?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't see any subclasses other than FakeAndroidViewController already defined within the framework (google3 and public repos). In general, this controller is an internal implementation of the framework; and it's very tightly coupled to the platform implementation.

I'm also not aware of any use case (other than maybe a fake?) that requires overriding or extending functionality.

Copy link
Author

Choose a reason for hiding this comment

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

At a high level, this API seems like it should have been private. I don't know why it was made public.


@override
Future<void> setOffset(Offset off) async {
if (off == _off || _state != _AndroidViewState.created)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the significance of _state != _AndroidViewState.created here? By looking at the code I don'
t understand why we don't need to set a new off if the state is created. Could you add a comment to explain? Or maybe find a better condition.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Added a comment to clarify.


@override
Future<void> setOffset(Offset off) async {
if (off == _off || _state != _AndroidViewState.created)

This comment was marked as duplicate.

@@ -1043,13 +1092,13 @@ class TextureAndroidViewController extends AndroidViewController {

@override
Future<void> _sendCreateMessage() async {
assert(!_size.isEmpty, 'trying to create $TextureAndroidViewController without setting a valid size.');
assert(!_initialSize.isEmpty, 'trying to create $TextureAndroidViewController without setting a valid size.');

This comment was marked as duplicate.

// native view on the screen
void _setOffset() {
SchedulerBinding.instance.addPostFrameCallback((_) async {
if (!_isDisposed) {

This comment was marked as duplicate.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@blasten blasten merged commit f58e8f5 into flutter:master Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants