Update "unregistered_view_type" error message #33450

dacianf · 2022-05-18T10:09:32Z

Updating the error message for unregistered_view_type to include a hint about a very common cause.

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 · 2022-05-18T10:09:35Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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 · 2022-05-18T18:42:18Z

test-exempt: improves error message

This is an area where you could test it, e.g. by triggering the error and seeing if the message has the detail you want, but it's fine if you don't want to do that.

lib/web_ui/lib/src/engine/platform_views/message_handler.dart

jmagman · 2022-05-19T17:25:02Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -140,7 +140,8 @@
  NSObject<FlutterPlatformViewFactory>* factory = factories_[viewType].get();
  if (factory == nil) {
    result([FlutterError errorWithCode:@"unregistered_view_type"
-                               message:@"trying to create a view with an unregistered type"
+                               message:@"trying to create a view with an unregistered type. Have "
+                                       @"you called GeneratedPluginRegistrant.register?"


@cyanglaz in the wild we saw this error fixed by calling GeneratedPluginRegistrant.register flutter/flutter#100241 (comment) but perhaps there's also some -registerViewFactory: to be done in the darwin case?

It is already available: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Headers/FlutterPlugin.h#L309

Sorry, I know it exists, the question is when this error happens is it reasonable to say "you need to call GeneratedPluginRegistrant.register" because it's missing (add-to-app) or do they also need to call -registerViewFactory: somewhere? We want this suggestion to actually be actionable.

Ah ok.

The registerViewFactory: is only meant to be called by the author of the PlatformView. If the user only uses a PlatformView plugin, they wouldn't be able to call registerViewFactory: as they don't own the factory, so showing this to the app developer who uses the PlatformView plugin might cause confusion.
I think only include GeneratedPluginRegistrant.register is fine for app developers.

We might be able to add some checks for the plugin/PlatformView authors to remind them to call registerViewFactory in registerWithRegistrar.

In addition to that, I think this part is also confusing: "trying to create a view with an unregistered type".
What view?(A view means lots of things, especially in add-to-app) Who or what tried to create a view? Maybe something like:

A UIKitView widget is trying to create a PlatformView with an unregistered type: <some type>. If you are the author of the PlatformView, make sure `registerViewFactory` is invoked. See: https://docs.flutter.dev/development/platform-integration/platform-views?tab=ios-platform-views-objective-c-tab#on-the-platform-side-1 for more details. If you are not the author of the PlatformView, make sure to call `GeneratedPluginRegistrant.register`.

jmagman

Some nits about whitespace and the URL

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

lib/web_ui/lib/src/engine/platform_views/message_handler.dart

shell/platform/darwin/macos/framework/Source/FlutterPlatformViewController.mm

jmagman · 2022-05-20T18:24:55Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -140,9 +140,9 @@
  NSObject<FlutterPlatformViewFactory>* factory = factories_[viewType].get();
  if (factory == nil) {
    result([FlutterError errorWithCode:@"unregistered_view_type"
-                               message:[NSString stringWithFormat:@"A UIKitView widget is trying to create a PlatformView with an unregistered type: < %@ >", viewType]
+                               message:[NSString stringWithFormat:@"A UIKitView widget is trying to create a PlatformView with an unregistered type: < %@ >", args[@"viewType"]]


Can you pull args[@"viewType"] into a local above std::string viewType([args[@"viewType"] UTF8String]); and use it here to reduce the complexity of this line?

@jmagman std::string viewType([args[@"viewType"] UTF8String]); is already defined on line 132. The problem that I noticed by using only viewType as variable was that the build failed with "error: cannot pass non-trivial object of type 'std::string'". So I guess it needs an NSString and not an std::String

Actually I think it should work if I use %s instead of %@

Don't use a %s c string like that, there are encoding implications https://stackoverflow.com/a/60217425

I was suggesting

NSString* viewTypeString = args[@"viewType"]; std::string viewType(viewTypeString.UTF8String); ... message:[NSString stringWithFormat:@"A UIKitView widget is trying to create a " @"PlatformView with an unregistered type: < %@ >", viewTypeString]

@jmagman I added the NSString variable there. I was thinking that maybe I have to make it work using the existing variable.

ditman

LGTM once @jmagman and @cyanglaz are happy!

jmagman

One more nit, then LGTM

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

jmagman · 2022-05-23T18:44:53Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

+              message:[NSString stringWithFormat:@"A UIKitView widget is trying to create a "
+                                                 @"PlatformView with an unregistered type: < %@ >",
+                                                 viewTypeString]
+              details:@"If you are the author of the PlatformView, make sure `registerViewFactory`"


Can you double check the spacing on these?

Suggested change

details:@"If you are the author of the PlatformView, make sure `registerViewFactory`"

details:@"If you are the author of the PlatformView, make sure `registerViewFactory` "

jmagman · 2022-05-23T18:45:20Z

shell/platform/darwin/macos/framework/Source/FlutterPlatformViewController.mm

+              message:[NSString stringWithFormat:@"A UIKitView widget is trying to create a "
+                                                 @"PlatformView with an unregistered type: < %@ >",
+                                                 viewType]
+              details:@"If you are the author of the PlatformView, make sure `registerViewFactory`"


Suggested change

details:@"If you are the author of the PlatformView, make sure `registerViewFactory`"

details:@"If you are the author of the PlatformView, make sure `registerViewFactory` "

…#33450)

flutter-dashboard bot added platform-ios platform-macos platform-web Code specifically for the web engine labels May 18, 2022

flutter-dashboard bot added the needs tests label May 18, 2022

dacianf force-pushed the patch branch from 7208da6 to db68a6e Compare May 18, 2022 10:21

jmagman reviewed May 18, 2022

View reviewed changes

lib/web_ui/lib/src/engine/platform_views/message_handler.dart Outdated Show resolved Hide resolved

jmagman reviewed May 19, 2022

View reviewed changes

dacianf force-pushed the patch branch from 3961259 to 17c89b6 Compare May 20, 2022 17:54

jmagman reviewed May 20, 2022

View reviewed changes

ditman approved these changes May 20, 2022

View reviewed changes

ditman removed the needs tests label May 20, 2022

dacianf force-pushed the patch branch from 6cbc1be to 29f6f70 Compare May 20, 2022 21:32

dacianf requested review from cyanglaz and jmagman May 20, 2022 21:39

dacianf force-pushed the patch branch from 29f6f70 to d9487e2 Compare May 20, 2022 21:45

jmagman reviewed May 20, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Outdated Show resolved Hide resolved

dacianf requested a review from jmagman May 21, 2022 06:22

jmagman reviewed May 23, 2022

View reviewed changes

Update "unregistered_view_type" error message

0a56618

dacianf force-pushed the patch branch from 7c6a314 to 0a56618 Compare May 23, 2022 19:05

jmagman approved these changes May 23, 2022

View reviewed changes

jmagman 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 May 23, 2022

fluttergithubbot merged commit c12332b into flutter:main May 23, 2022

engine-flutter-autoroll mentioned this pull request May 23, 2022

Roll Flutter Engine from c2cb62b54be4 to 13c80332a192 (7 revisions) flutter/flutter#104459

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2022

c12332b Update "unregistered_view_type" error message (flutter/engine…

ac51d7a

…#33450)

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

Update "unregistered_view_type" error message (flutter#33450)

28fca7b

	details:@"If you are the author of the PlatformView, make sure `registerViewFactory`"
	details:@"If you are the author of the PlatformView, make sure `registerViewFactory` "

Update "unregistered_view_type" error message #33450

Update "unregistered_view_type" error message #33450

Uh oh!

Conversation

dacianf commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 18, 2022

Uh oh!

Hixie commented May 18, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dacianf commented May 18, 2022 •

edited

Loading

jmagman May 20, 2022 •

edited

Loading