Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

add docs to platformviewios (and some drive-by changes) #17593

Merged
xster merged 5 commits into from
May 7, 2020
Merged

add docs to platformviewios (and some drive-by changes) #17593

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30fa906
add docs to platformviewios (and some drive-by changes)
xster Apr 8, 2020
bebb1f0
a bit more import cleanups
xster Apr 8, 2020
5d0b151
autoformat
xster Apr 9, 2020
d10149c
add one more comment line
xster May 6, 2020
2f311fa
tweak a sentence
xster May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,6 @@ - (void)maybeSetupPlatformViewChannels {
[_textInputChannel.get() setMethodCallHandler:^(FlutterMethodCall* call, FlutterResult result) {
[_textInputPlugin.get() handleMethodCall:call result:result];
}];
self.iosPlatformView->SetTextInputPlugin(_textInputPlugin);
}
}

Expand Down
8 changes: 2 additions & 6 deletions shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,6 @@ - (void)installFirstFrameCallback {

#pragma mark - Properties

- (FlutterView*)flutterView {

@xster xster Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this wasn't exposed or used anywhere

return _flutterView;
}

- (UIView*)splashScreenView {
if (!_splashScreenView) {
return nil;
Expand Down Expand Up @@ -515,8 +511,8 @@ - (void)viewDidLoad {
_engineNeedsLaunch = NO;
}

FML_DCHECK([_engine.get() viewController] != nil)
<< "FlutterViewController::viewWillAppear:AttachView ViewController was nil";
FML_DCHECK([_engine.get() viewController] == self)
<< "FlutterViewController shown but is not attached to a FlutterEngine";

@gaaclarke gaaclarke Apr 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this message be confusing if [_engine.get() viewController] is nil?

@xster xster May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what scenario do you have in mind? A viewDidLoad on a FlutterViewController should never be called if [_engine.get() viewController] is nil right?

@gaaclarke gaaclarke May 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, viewDidLoad doesn't necessarily mean that the ViewController has been shown. It just mean's it's been lazy loaded (hopefully only when it gets shown, but that wasn't the case a couple of months ago).

Also, what about the case where [_engine.get() viewController] == self && [_engine.get() viewController] != nil? The wording is a bit odd in that case.

@xster xster May 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tweaked a few words from VC being shown to VC's view being loaded. But the spirit of the warning is still the same.

A few months ago, the bug was that the engine loaded the view on the VC earlier than when the VC would have been shown. If an engine wasn't attached altogether, the warning would be even more important.

Also, what about the case where [_engine.get() viewController] == self && [_engine.get() viewController] != nil? The wording is a bit odd in that case.

You meant [_engine.get() viewController] != self && [_engine.get() viewController] != nil right? That's still a bug (that somehow this VC's view is loaded and isn't attached to an engine).

[_engine.get() attachView];

[super viewDidLoad];
Expand Down
7 changes: 7 additions & 0 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@
namespace flutter {
class PlatformViewIOS;

/**
* An accessibility instance is bound to one `FlutterViewController` and
* `FlutterView` instance.
*
* It helps populate the UIView's accessibilityElements property from Flutter's
* semantics nodes.
*/
class AccessibilityBridge final : public AccessibilityBridgeIos {
public:
AccessibilityBridge(UIView* view,
Expand Down
9 changes: 5 additions & 4 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/accessibility_text_entry.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h"

@xster xster Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we can just change all the #includes to #imports for objc(++) files (also in go/objc-style)

@gaaclarke gaaclarke Apr 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Those rules are for objc, there is one exception for objc++. If the .h for you .mm has a c++ class, you should use "include" if the intent is to use it in c++. All .mm files can use import exclusively.

@xster xster May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah ok. Though you're still saying this file is ok right?

@gaaclarke gaaclarke May 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, just responding to just change **all** the #includes.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_text_entry.h"

#include "flutter/shell/platform/darwin/ios/platform_view_ios.h"
#import "flutter/shell/platform/darwin/ios/platform_view_ios.h"

#pragma GCC diagnostic error "-Wundeclared-selector"

Expand Down Expand Up @@ -38,7 +39,7 @@
}

UIView<UITextInput>* AccessibilityBridge::textInputView() {
return [platform_view_->GetTextInputPlugin() textInputView];
return [[platform_view_->GetOwnerViewController().get().engine textInputPlugin] textInputView];

@gaaclarke gaaclarke Apr 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't recommend this change because of the law of dimeter. I recommend placing this code inside the GetTextInputPlugin() method.

@xster xster May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I generally agree if our current components lifecycles and ownerships are cleanly designed and encapsulated. In practice, I think the platform view class shouldn't exist and I'd like to gradually hollow it out more and more. We're also already not minimizing knowledge in this class a few lines above with the binaryMessenger.

}

void AccessibilityBridge::UpdateSemantics(flutter::SemanticsNodeUpdates nodes,
Expand Down
42 changes: 38 additions & 4 deletions shell/platform/darwin/ios/platform_view_ios.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@

namespace flutter {

/**
* A bridge connecting the platform agnostic shell and the iOS embedding.
*
* The shell provides and requests for UI related data and this PlatformView subclass fulfills
* it with iOS specific capabilities. As an example, the iOS embedding (the `FlutterEngine` and the
* `FlutterViewController`) sends pointer data to the shell and receives the shell's request for a
* Skia GrContext and supplies it.
*
* Despite the name "view", this class is unrelated to UIViews on iOS and doesn't have the same
* lifecycle. It's a long lived bridge owned by the `FlutterEngine` and can be attached and
* detached sequentially to multiple `FlutterViewController`s and `FlutterView`s.
*/
class PlatformViewIOS final : public PlatformView {
public:
explicit PlatformViewIOS(PlatformView::Delegate& delegate,
Expand All @@ -33,21 +45,43 @@ class PlatformViewIOS final : public PlatformView {

~PlatformViewIOS() override;

/**
* The `PlatformMessageRouter` is the iOS bridge connecting the shell's
* platform agnostic `PlatformMessage` to iOS's channel message handler.
*/
PlatformMessageRouter& GetPlatformMessageRouter();

/**
* Returns the `FlutterViewController` currently attached to the `FlutterEngine` owning
* this PlatformViewIOS.
*/
fml::WeakPtr<FlutterViewController> GetOwnerViewController() const;

/**
* Updates the `FlutterViewController` currently attached to the `FlutterEngine` owning
* this PlatformViewIOS. This should be updated when the `FlutterEngine`
* is given a new `FlutterViewController`.
*/
void SetOwnerViewController(fml::WeakPtr<FlutterViewController> owner_controller);

/**
* Called one time per `FlutterViewController` when the `FlutterViewController`'s
* UIView is first loaded.
*
* Can be used to perform late initialization after `FlutterViewController`'s
* init.
*/
void attachView();

/**
* Called through when an external texture such as video or camera is
* given to the `FlutterEngine` or `FlutterViewController`.
Comment on lines +68 to +78

@gaaclarke gaaclarke Apr 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two docstrings talk about when a method is called. Docstrings should explain what the procedure does, not when it is called.

@xster xster May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ya, this is unfortunately same as above. It's not a "docstring" since the whole thing is private. It's code comment rather since this existing code (which was here before FlutterEngine etc existed) already have expectations on when it needs to be called. What it offers to its "consumer"'s perspective is already opaque.

*/
void RegisterExternalTexture(int64_t id, NSObject<FlutterTexture>* texture);

// |PlatformView|
PointerDataDispatcherMaker GetDispatcherMaker() override;

fml::scoped_nsprotocol<FlutterTextInputPlugin*> GetTextInputPlugin() const;

void SetTextInputPlugin(fml::scoped_nsprotocol<FlutterTextInputPlugin*> plugin);

// |PlatformView|
void SetSemanticsEnabled(bool enabled) override;

Expand Down
15 changes: 5 additions & 10 deletions shell/platform/darwin/ios/platform_view_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/fml/trace_event.h"
#include "flutter/shell/common/shell_io_manager.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h"

namespace flutter {

Expand Down Expand Up @@ -99,6 +99,9 @@

void PlatformViewIOS::attachView() {
FML_DCHECK(owner_controller_);
FML_DCHECK(owner_controller_.get().isViewLoaded)
<< "FlutterViewController's view should be loaded "
"before attaching to PlatformViewIOS.";
ios_surface_ =
[static_cast<FlutterView*>(owner_controller_.get().view) createSurface:ios_context_];
FML_DCHECK(ios_surface_ != nullptr);
Expand Down Expand Up @@ -188,14 +191,6 @@ new AccessibilityBridge(static_cast<FlutterView*>(owner_controller_.get().view),
[owner_controller_.get() platformViewsController]->Reset();
}

fml::scoped_nsprotocol<FlutterTextInputPlugin*> PlatformViewIOS::GetTextInputPlugin() const {

@xster xster Apr 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We might want to clean this a bit more, but it was strange having this in separately maintained lifecycles in various owners

return text_input_plugin_;
}

void PlatformViewIOS::SetTextInputPlugin(fml::scoped_nsprotocol<FlutterTextInputPlugin*> plugin) {
text_input_plugin_ = plugin;
}

PlatformViewIOS::ScopedObserver::ScopedObserver() : observer_(nil) {}

PlatformViewIOS::ScopedObserver::~ScopedObserver() {
Expand Down