-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Windows] Allow plugins to get views #51096
[Windows] Allow plugins to get views #51096
Conversation
01c60cb
to
870b27b
Compare
// not destroy the underlying view. | ||
std::shared_ptr<FlutterView> GetViewById(FlutterViewId view_id) const { | ||
return std::make_shared<FlutterView>( | ||
FlutterDesktopPluginRegistrarGetViewById(registrar(), view_id)); |
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.
std::shared_ptr<FlutterView>
instead of a FlutterView*
. This is a change from the original design.
The original PluginRegistrarWindows::GetViewById
proposal returned a FlutterView*
to align with PluginRegistrarWindows::GetView
. However, this would require the PluginRegistrarWindows
to own/manage FlutterView
s, similar to how it stores the implicit view today. We would need to introduce some other API to allow cleaning up these FlutterView
s. By returning a std::shared_ptr<FlutterView>
, the user can clean up the FlutterView
themselves when they no longer need the view reference.
870b27b
to
c3a423f
Compare
Gentle reminder @yaakovschectman @cbracken, this is ready for review :) |
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
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.
@yaakovschectman @cbracken FYI I pushed a new commit: f89609f. Before this fix, |
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.
re-lgtm!
…144732) flutter/engine@31bbe61...b2adf74 2024-03-06 737941+loic-sharma@users.noreply.github.com [Windows] Allow plugins to get views (flutter/engine#51096) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Allow Flutter Windows plugins to get views by their ID.
Design doc: https://flutter.dev/go/desktop-multi-view-runner-apis
Part of flutter/flutter#143767
Part of flutter/flutter#142845
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.