-
Notifications
You must be signed in to change notification settings - Fork 6k
[MacOS] Add support for creating and presenting platform views. #22905
Conversation
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
99a48ad to
9ee9d45
Compare
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are also missing tests in this PR.
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterPlatformViews.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
6e91de1 to
b1c6f57
Compare
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits, assuming @iskakaushik LGTMed as well.
Thanks for adding tests!
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterPlatformViews.h
Outdated
Show resolved
Hide resolved
444f71e to
6874a3e
Compare
|
Please don't merge this until i've taken a look :-) I think the public API here needs a little more scrutiny. |
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterGLCompositor.mm
Outdated
Show resolved
Hide resolved
87de964 to
8fbb288
Compare
shell/platform/darwin/macos/framework/Source/FlutterPlatformViewController.mm
Outdated
Show resolved
Hide resolved
Thanks for the review! I'll update this on Friday. |
Add platform view support in FlutterGLCompositor and FlutterViewController
Renaming variables, created PresentPlatformView method and updated documentation
|
@stuartmorgan @iskakaushik First ran into after rebasing this branch ontop of master but it seems like I'm running into on master as well with a local engine. Tried following the suggestions in this issue flutter/flutter#72608, ie upgrading and doing a new project but none seem to work with a local engine.
Anyone have any ideas whats wrong here? |
|
I wouldn't have expected a local engine to have any effect on App.framework copying. I'll see if I can reproduce locally and follow up in flutter/flutter#72608 |
612f034 to
b8f5837
Compare
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
I'm getting the same failures as https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8857625998621949056/+/steps/build_host_debug_unopt/0/stdout locally. Can you fix those up and see if you are still reproducing flutter/flutter#72608? |
Never mind, I was able to solve the problem by recloning the repos and setting up the environment again. |
|
Hey @RichardJCai , thanks for continuing to work and make progress on this PR. Given the inactivity over the last week, I'm closing this. Feel free to open a new PR once you get a chance to address the failing pre-submit checks and tag me. Thanks! |
Sorry tried to address the PR comments but I still can't seem to get my engine environment set up for some reason. |
|
@RichardJCai did you have a chance to fix your environment? I know you probably already checked this but I once got this exact same error message when the underlying issue was in fact a build error in my dart code. Can you maybe post a verbose output of your build with the local engine? I'd be happy to help you debug your environment as good as I can. Here on or on Discord (cben#2483). |
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable.
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668 * Example is at: https://github.com/iskakaushik/flutter_macos_platform_view_example
* The plan is to only support platform views when using Metal rendering backend. * This PR is based on the work done in: flutter#22905 * There are some threading issues to be addressed still to make this feature usable: flutter/flutter#96668 * Example is at: https://github.com/iskakaushik/flutter_macos_platform_view_example
Add support for:
Will add follow up PR for static thread merging, there may be visual bugs since raster/ui threads are not merged.