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

Fix: support to set layout direction for hybrid composition platform view #30611

Merged
blasten merged 8 commits into from
Jan 26, 2022
Merged

Fix: support to set layout direction for hybrid composition platform view #30611

blasten merged 8 commits into from
Jan 26, 2022

Conversation

@wangying3426
Copy link
Contributor

@wangying3426 wangying3426 commented Dec 31, 2021 •
edited
Loading

On android platform, virtual display platform view can set direction layout correctly, but hybrid composition will fail and throw an NullPointerException as flutter/flutter#96003 shown, this PR fixes it.

Besides, if a virtual display platform view is not found with specified viewId, vdControllers.get(viewId) will be null, so an NPE will also occur, this PR also fixes it.

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.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Dec 31, 2021

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 Hixie on the #hackers channel in Chat.

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.

@wangying3426
Copy link
Contributor Author

wangying3426 commented Jan 4, 2022

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 Hixie on the #hackers channel in Chat.

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.

@Hixie Could you please review this RP? thanks.

@eggfly eggfly requested review from blasten and chinmaygarde January 4, 2022 03:00
@wangying3426 wangying3426 mentioned this pull request Jan 4, 2022
Closed
@eggfly eggfly removed the needs tests label Jan 6, 2022
@wangying3426 wangying3426 force-pushed the fix_set_layout_direction branch from 71c5ece to 342682e Compare January 17, 2022 06:31
@wangying3426
Copy link
Contributor Author

wangying3426 commented Jan 18, 2022 •
edited
Loading

@blasten @chinmaygarde hello, I have fixed it, Could you have any other suggestions？😌

@wangying3426 wangying3426 changed the title Support hybrid composition platform view to set layout direction Fix: support to set layout direction for hybrid composition platform view Jan 19, 2022
@eggfly eggfly requested a review from blasten January 21, 2022 09:44
blasten
blasten approved these changes Jan 25, 2022
View reviewed changes
Copy link

@blasten blasten 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 added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 25, 2022
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jan 25, 2022

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 25, 2022
@blasten blasten merged commit e2addab into flutter:main Jan 26, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 26, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
@engine-flutter-autoroll
e2addab Fix: support to set layout direction for hybrid composition p…
8846381 
…latform view (flutter/engine#30611)
@wangying3426 wangying3426 deleted the fix_set_layout_direction branch January 27, 2022 02:31
@zanderso zanderso mentioned this pull request Jan 27, 2022
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 27, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2022
@engine-flutter-autoroll
e2addab Fix: support to set layout direction for hybrid composition p…
5f50772 
…latform view (flutter/engine#30611)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

1 more reviewer

@blasten blasten blasten approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

platform-android

Projects

Contributions from Bytedance
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wangying3426 @fluttergithubbot @blasten @eggfly