Add a viewController property to the ios/macOS FlutterPluginRegistrar protocol

[LongCatIsLooong]

#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

• 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, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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.

[LongCatIsLooong]

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]

(unrelated change, removed an invisible character)

[loic-sharma]

... 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]

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;
	}

[vashworth]

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

@stuartmorgan-g @LongCatIsLooong What do you think of adding a viewController property to the macOS version as well to keep them uniform?

[stuartmorgan-g]

This is a method instead of a weak + readonly property like the macOS registrar protocol because existing getters are also methods

AFAIK that's only because the API predates properties, and nobody thought to change it before the introduction of Swift made it a breaking change to do so. I would recommend making it a property; IMO new code should follow current best practices even when older code doesn't. We already have an open issue for trying to migrate the old methods to properties in a non-breaking way.

[stuartmorgan-g]

What do you think of adding a viewController property to the macOS version as well to keep them uniform?

The obvious argument against is that it adds a new API surface that will be quasi-deprecated from the start. That said, it doesn't add anything conceptually new over view, so as long as we also add viewControllerForViewIdentifier: on macOS at the same time as viewController, I think the consistency argument is stronger than the deprecation counter-argument.

[LongCatIsLooong]

What do you think of adding a viewController property to the macOS version as well to keep them uniform?

The obvious argument against is that it adds a new API surface that will be quasi-deprecated from the start. That said, it doesn't add anything conceptually new over view, so as long as we also add viewControllerForViewIdentifier: on macOS at the same time as viewController, I think the consistency argument is stronger than the deprecation counter-argument.

Ah adding viewController and viewControllerForViewID: together to both platforms so people can decide which one to use / when to migrate? Sounds good.

[stuartmorgan-g]

Ah adding viewController and viewControllerForViewID: together to both platforms so people can decide which one to use / when to migrate? Sounds good.

I thought the plumbing didn't exist on the iOS side to properly implement the ID-based version, in which case we shouldn't add it there yet.

Actually, looking at macOS headers, do we not have the view-by-identifier method yet? I thought it had been added, but if not, then we don't need to add it here. I just didn't want to get into a situation where the view controller version was less capable than the existing API.

[LongCatIsLooong]

Ah adding viewController and viewControllerForViewID: together to both platforms so people can decide which one to use / when to migrate? Sounds good.

I thought the plumbing didn't exist on the iOS side to properly implement the ID-based version, in which case we shouldn't add it there yet.

Actually, looking at macOS headers, do we not have the view-by-identifier method yet? I thought it had been added, but if not, then we don't need to add it here. I just didn't want to get into a situation where the view controller version was less capable than the existing API.

IIRC the macos multi view change was reverted. I'll remove the multi view stuff then.

[vashworth]

This test should be removed since viewControllerForViewIdentifier was removed

[auto-submit]

autosubmit label was removed for flutter/flutter/174168, because - The status or check suite Mac tool_integration_tests_5_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.