Skip to content

Add a viewController getter to the ios FlutterPluginRegistrar protocol #174168

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add a viewController getter to the ios FlutterPluginRegistrar protocol #174168

wants to merge 5 commits into from
+32 −1

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Aug 20, 2025 •
edited
Loading

#104117

Adds a viewController (per #104117 (comment), presumably because iOS plugins want to presenting new view controllers) getter that makes the single-view assumption. This is a method instead of a weak + readonly property like the macOS registrar protocol because existing getters are also methods, e.g.,

- (NSObject<FlutterBinaryMessenger>*)messenger;
- (NSObject<FlutterTextureRegistry>*)textures;

Unfortunately swift callers will have to use a preprocess macro and add different code paths for macOS and iOS.

Single view assumption

I've considered adding a - (nullable UIViewController*)viewControllerForViewID: ViewIDType; method but that will force plugins to make breaking changes in order to migrate off of the deprecated keyWindow property. Take the text input plugin as an example, it has to add a viewID argument to the setTextInputClient call when establishing a new text input connection, so the text input plugin knows which view hierarchy to add the text input view to. It is unclear to me whether more FlutterPluginRegistrar changes are needed to completely get rid of the single-view assumption in the protocol (the views FlutterPlatformViewFactory creates are always added as subviews to flutterView so I guess that interface may have to change in the future? According to according to Flutter’s multi-window status, only a viewForIdentifier method will be added to registrar), so IMO it would be a better experience to not introduce the viewID concept right now and force plugins to make break changes, since they may have to make more breaking changes in the future.

Alternatively, we could also ignore the viewID args for now and always return the view controller of the implicit view in the implementation, so that plugin authors can use made-up view IDs to get the implicit view controller. But that's going to make future multiview migrations difficult since the compiler won't be able to catch unmigrated code if the plugin is using a bogus view ID.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@github-actions github-actions bot added platform-ios iOS applications specifically engine flutter/engine repository. See also e: labels. team-ios Owned by iOS platform team labels Aug 20, 2025
@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Aug 21, 2025 •
edited
Loading

Hi @loic-sharma according to Multi-view runner APIs the only plugin registrar change required to add multiview support to the macOS plugin registrar is to add a get view by id method (which is pretty similar to what this PR does but for macOS). But adding that method now will require some plugins to add a viewID parameter to their API (in order to remove the deprecated keyWindow usages from their codebase), are you OK with adding the single view version first and deprecate this in favor of the multi view version in the future?

LongCatIsLooong
LongCatIsLooong commented Aug 21, 2025
View reviewed changes
engine/src/flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h
@@ -34,7 +34,7 @@ FLUTTER_DARWIN_EXPORT
* @param frame The rectangle for the newly created `UIView` measured in points.
* @param viewId A unique identifier for this `UIView`.
* @param args Parameters for creating the `UIView` sent from the Dart side of the Flutter app.
* If `createArgsCodec` is not implemented, or if no creation arguments were sent from the Dart
* If `createArgsCodec` is not implemented, or if no creation arguments were sent from the Dart
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(unrelated change, removed an invisible character)

@loic-sharma
Copy link
Member

... are you OK with adding the single view version first and deprecate this in favor of the multi view version in the future?

For iOS that seems reasonable, yes. Plumbing view IDs around is a significant/painful migration, so it seems reasonable to do separate migrations for keyWindow and single view assumptions.

@LongCatIsLooong
Copy link
Contributor Author

To answer my own question regarding whether the platform view factory registration methods on the registrar interface would need a view ID parameter: looks like each layer (including those representing platform views) knows which view it is supposed to render on, so it probably doesn't need a view ID.

flutter/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm

Lines 71 to 132 in 5c23f4c

bool FlutterCompositor::Present(FlutterViewIdentifier view_id,
const FlutterLayer** layers,
size_t layers_count) {
FlutterView* view = [view_provider_ viewForIdentifier:view_id];
if (!view) {
return false;
}
NSMutableArray* surfaces = [NSMutableArray array];
for (size_t i = 0; i < layers_count; i++) {
const FlutterLayer* layer = layers[i];
if (layer->type == kFlutterLayerContentTypeBackingStore) {
FlutterSurface* surface =
[FlutterSurface fromFlutterMetalTexture:&layer->backing_store->metal.texture];
if (surface) {
FlutterSurfacePresentInfo* info = [[FlutterSurfacePresentInfo alloc] init];
info.surface = surface;
info.offset = CGPointMake(layer->offset.x, layer->offset.y);
info.zIndex = i;
FlutterBackingStorePresentInfo* present_info = layer->backing_store_present_info;
if (present_info != nullptr && present_info->paint_region != nullptr) {
auto paint_region = present_info->paint_region;
// Safe because the size of FlutterRect is not expected to change.
info.paintRegion = std::vector<FlutterRect>(
paint_region->rects, paint_region->rects + paint_region->rects_count);
}
[surfaces addObject:info];
}
}
}
CFTimeInterval presentation_time = 0;
if (layers_count > 0 && layers[0]->presentation_time != 0) {
presentation_time = [time_converter_ engineTimeToCAMediaTime:layers[0]->presentation_time];
}
// Notify block below may be called asynchronously, hence the need to copy
// the layer information instead of passing the original pointers from embedder.
auto layers_copy = std::make_shared<std::vector<LayerVariant>>(CopyLayers(layers, layers_count));
[view.surfaceManager presentSurfaces:surfaces
atTime:presentation_time
notify:^{
// Accessing presenters_ here does not need a
// lock to avoid race condition against
// AddView and RemoveView, since all three
// take place on the platform thread. (The
// macOS API requires platform view presenting
// to take place on the platform thread,
// enforced by `FlutterThreadSynchronizer`.)
dispatch_assert_queue(dispatch_get_main_queue());
auto found_presenter = presenters_.find(view_id);
if (found_presenter != presenters_.end()) {
found_presenter->second.PresentPlatformViews(
view, *layers_copy, platform_view_controller_);
}
}];
return true;
}

@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review August 22, 2025 21:14
@LongCatIsLooong LongCatIsLooong requested a review from a team as a code owner August 22, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loic-sharma loic-sharma Awaiting requested review from loic-sharma

@vashworth vashworth Awaiting requested review from vashworth

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
engine flutter/engine repository. See also e: labels. platform-ios iOS applications specifically team-ios Owned by iOS platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LongCatIsLooong @loic-sharma