bind missing add icon in platform_view example #132028

TJRoger · 2023-08-07T07:20:38Z

In the examples/platform_view example, which demonstrates transitioning from a Flutter view to a general iOS UIView and back, as well as using channels to communicate between the two, the Flutter view renders correctly, but in the iOS UIView, the FAB equivalent button in the lower-right corner is rendered without the '+' icon. This is because no binding as declared for the add icon in the storyboard. This adds the missing binding.

Further, this eliminates the use of the deprecated UIButtonTypeRoundedRect, falling back to the default instead.

flutter-dashboard · 2023-08-07T07:20:40Z

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.

cbracken · 2023-11-21T18:54:11Z

Thanks for the patch and apologies for the delays in getting to it. I've rebased to tip of tree and reduced the diffs to the minimum required to get the add button displaying correctly in the right colour.

I'd initially thought this might be a reasonable candidate for a test exemption on the basis that we don't have any tests for the native bits of the app and that the primary purpose of the app is really to demonstrate the interaction between Flutter and non-Flutter parts of the app, so adding a test for a cosmetic issue might reduce the signal-to-noise ratio of what we actually want to test... but thinking about it, we do have native XCTests for this, so I suspect it's possible to add one without too much difficulty.

The tests are here:
dev/integration_tests/ios_platform_view_tests/ios/PlatformViewUITests/PlatformViewUITests.m

Let me know if you run into difficulty adding the test code! Happy to answer any questions you may have.

cbracken

I've rebased and minimised the diff to just what's required. We'll want a test in order to ensure that no one re-breaks this without realising. Once that's landed, this looks good to me.

flutter-dashboard · 2023-11-21T19:49: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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

goderbauer · 2024-01-30T23:38:50Z

@TJRoger Do you still have plans to come back to this one and address the feedback given above about adding a test?

TJRoger · 2024-02-26T03:36:14Z

@TJRoger Do you still have plans to come back to this one and address the feedback given above about adding a test?

Sorry for late reply. I don't think it's necessary for adding test. I even don't know how to add such a test for icon binding.

cbracken · 2024-02-26T19:56:56Z

I even don't know how to add such a test for icon binding.

Bind the button to an outlet by adding something like this line in the storyboard:

--- i/examples/platform_view/ios/Runner/Base.lproj/Main.storyboard
+++ w/examples/platform_view/ios/Runner/Base.lproj/Main.storyboard
@@ -130,6 +130,7 @@
                     </view>
                     <connections>
                         <outlet property="incrementLabel" destination="qaB-rs-mWp" id="bFY-of-WoI"/>
+                        <outlet property="incrementButton" destination="6yL-sX-bUL" id="aQR-ap-BrT"/>
                     </connections>
                 </viewController>
                 <placeholder placeholderIdentifier="IBFirstResponder" id="7qW-nP-WZA" userLabel="First Responder" sceneMemberID="firstResponder"/>

and the outlet in the platformview header:

--- i/examples/platform_view/ios/Runner/PlatformViewController.h
+++ w/examples/platform_view/ios/Runner/PlatformViewController.h
@@ -9,6 +9,7 @@
 @end
 
 @interface PlatformViewController : UIViewController
+@property(weak, nonatomic) IBOutlet UIButton* incrementButton;
 @property(strong, nonatomic) id<PlatformViewControllerDelegate> delegate;
 @property int counter;
 @end

then in a test case check:

UIImage* image = [incrementButton imageForState:UIControlStateNormal];
XCTAssertNotNil(image);

I don't think it's necessary for adding test.

Without a test, there's no guarantee this won't regress back to how it was. Generally speaking, if it's worth fixing, it's worth having a test to ensure it stays fixed. We make occasional exceptions for this -- e.g. variable renames or minor refactoring that results in no change to the external behaviour of the code.

I've added a unit test to this patch that catches the issue; I'll send another patch that wires this into our CI.

cbracken

This reverts some changes that were unnecessary to fix the display of the "+" icon in the native iOS view.

Adds a test that verifies that an icon was loaded for the incrementButton.

auto-submit · 2024-02-27T19:16:05Z

auto label is removed for flutter/flutter/132028, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

gspencergoog

examples/platform_view/ios/Runner/Base.lproj/Main.storyboard

This applies minor cosmetic cleanups noticed while landing fix/tests in another patch. Renames `incrementLabel` to `countLabel` and `setIncrementLabel` to `updateCountLabel`, since it's not setting it to a specific value but rather updating itself based on the current state of the `count` ivar. Related: flutter#132028

This applies minor cosmetic cleanups noticed while landing fix/tests in another patch. Renames `incrementLabel` to `countLabel` and `setIncrementLabel` to `updateCountLabel`, since it's not setting it to a specific value but rather updating itself based on the current state of the `count` ivar. No tests since this patch introduces no semantic changes. Related: #132028

…er#132028)

Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) Manual roll requested by bmparr@google.com flutter/flutter@c30f998...d00bfe8 2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9381fb8ef4c to 455c814fe5de (1 revision) (flutter/flutter#144340) 2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 91898e397261 to c9381fb8ef4c (11 revisions) (flutter/flutter#144338) 2024-02-28 15619084+vashworth@users.noreply.github.com Reland "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144275) 2024-02-28 christopherfujino@gmail.com [flutter_tools] Catch rpc error in render frame with raster stats (flutter/flutter#144190) 2024-02-28 github@alexv525.com ð��¡ï¸� Guard Flutter Android app by disallow task affinity by default (flutter/flutter#144018) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 8acc96d405d0 to 91898e397261 (flutter/flutter#144316) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 64a375de9c8f to 8acc96d405d0 (flutter/flutter#144296) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine c79117b706e9 to 64a375de9c8f (flutter/flutter#144293) 2024-02-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (#144207)" (flutter/flutter#144292) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 2461280c38b7 to c79117b706e9 (flutter/flutter#144290) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 5e0d9ba35dd5 to 2461280c38b7 (flutter/flutter#144288) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine fe7ea6d9c34f to 5e0d9ba35dd5 (flutter/flutter#144285) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 0bc21ea7bc92 to fe7ea6d9c34f (flutter/flutter#144283) 2024-02-28 polinach@google.com Use const route for notAnnounced. (flutter/flutter#144050) 2024-02-28 tessertaha@gmail.com Add `tabs_utils.dart` class (flutter/flutter#143937) 2024-02-28 36861262+QuncCccccc@users.noreply.github.com Remove `bottomAppBarColor` from `ThemeData` (flutter/flutter#144080) 2024-02-27 richadlu001@gmail.com fix: unexpected chinese punctuation (flutter/flutter#143678) 2024-02-27 42216813+eliasyishak@users.noreply.github.com Clean up lint ignores (flutter/flutter#144229) 2024-02-27 jhy03261997@gmail.com Reland [a11y] Add isEnabled semantics flag to text field (flutter/flutter#143601) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove deprecated `CupertinoContextMenu.previewBuilder` (flutter/flutter#143990) 2024-02-27 chris@bracken.jp [iOS] Fix naming in platform_view example (flutter/flutter#144247) 2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (#144001)" (flutter/flutter#144262) 2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144253) 2024-02-27 kustermann@google.com [web] Make flutter web profile builds always keep wasm symbols (flutter/flutter#144130) 2024-02-27 36861262+QuncCccccc@users.noreply.github.com Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#144001) 2024-02-27 tjrogertj@gmail.com bind missing add icon in platform_view example (flutter/flutter#132028) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (flutter/flutter#144207) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove strut migration flag from `TextPainter` (flutter/flutter#144242) 2024-02-27 15619084+vashworth@users.noreply.github.com Remove force Xcode debug workflow (flutter/flutter#144185) 2024-02-27 54558023+keyonghan@users.noreply.github.com Mark two other firebase targets as bringup: true (flutter/flutter#144234) 2024-02-27 15619084+vashworth@users.noreply.github.com Add FlutterMacOS.xcframework artifact (flutter/flutter#143244) 2024-02-27 jonahwilliams@google.com Re-enable Impeller goldens blocking. (flutter/flutter#144210) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 ...

) Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) Manual roll requested by bmparr@google.com flutter/flutter@c30f998...d00bfe8 2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9381fb8ef4c to 455c814fe5de (1 revision) (flutter/flutter#144340) 2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 91898e397261 to c9381fb8ef4c (11 revisions) (flutter/flutter#144338) 2024-02-28 15619084+vashworth@users.noreply.github.com Reland "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144275) 2024-02-28 christopherfujino@gmail.com [flutter_tools] Catch rpc error in render frame with raster stats (flutter/flutter#144190) 2024-02-28 github@alexv525.com ð��¡ï¸� Guard Flutter Android app by disallow task affinity by default (flutter/flutter#144018) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 8acc96d405d0 to 91898e397261 (flutter/flutter#144316) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 64a375de9c8f to 8acc96d405d0 (flutter/flutter#144296) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine c79117b706e9 to 64a375de9c8f (flutter/flutter#144293) 2024-02-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (#144207)" (flutter/flutter#144292) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 2461280c38b7 to c79117b706e9 (flutter/flutter#144290) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 5e0d9ba35dd5 to 2461280c38b7 (flutter/flutter#144288) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine fe7ea6d9c34f to 5e0d9ba35dd5 (flutter/flutter#144285) 2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 0bc21ea7bc92 to fe7ea6d9c34f (flutter/flutter#144283) 2024-02-28 polinach@google.com Use const route for notAnnounced. (flutter/flutter#144050) 2024-02-28 tessertaha@gmail.com Add `tabs_utils.dart` class (flutter/flutter#143937) 2024-02-28 36861262+QuncCccccc@users.noreply.github.com Remove `bottomAppBarColor` from `ThemeData` (flutter/flutter#144080) 2024-02-27 richadlu001@gmail.com fix: unexpected chinese punctuation (flutter/flutter#143678) 2024-02-27 42216813+eliasyishak@users.noreply.github.com Clean up lint ignores (flutter/flutter#144229) 2024-02-27 jhy03261997@gmail.com Reland [a11y] Add isEnabled semantics flag to text field (flutter/flutter#143601) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove deprecated `CupertinoContextMenu.previewBuilder` (flutter/flutter#143990) 2024-02-27 chris@bracken.jp [iOS] Fix naming in platform_view example (flutter/flutter#144247) 2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (#144001)" (flutter/flutter#144262) 2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144253) 2024-02-27 kustermann@google.com [web] Make flutter web profile builds always keep wasm symbols (flutter/flutter#144130) 2024-02-27 36861262+QuncCccccc@users.noreply.github.com Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#144001) 2024-02-27 tjrogertj@gmail.com bind missing add icon in platform_view example (flutter/flutter#132028) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (flutter/flutter#144207) 2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove strut migration flag from `TextPainter` (flutter/flutter#144242) 2024-02-27 15619084+vashworth@users.noreply.github.com Remove force Xcode debug workflow (flutter/flutter#144185) 2024-02-27 54558023+keyonghan@users.noreply.github.com Mark two other firebase targets as bringup: true (flutter/flutter#144234) 2024-02-27 15619084+vashworth@users.noreply.github.com Add FlutterMacOS.xcframework artifact (flutter/flutter#143244) 2024-02-27 jonahwilliams@google.com Re-enable Impeller goldens blocking. (flutter/flutter#144210) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 ...

…er#132028)

github-actions bot added the d: examples Sample code and demos label Aug 7, 2023

goderbauer requested a review from cyanglaz August 8, 2023 20:56

goderbauer requested a review from cbracken October 6, 2023 23:31

cbracken force-pushed the master branch 2 times, most recently from 3e686e8 to 3a047ce Compare November 21, 2023 18:49

cbracken reviewed Nov 21, 2023

View reviewed changes

cbracken force-pushed the master branch from c02a0bf to e483c06 Compare February 26, 2024 20:08

cbracken approved these changes Feb 26, 2024

View reviewed changes

TJRoger and others added 4 commits February 26, 2024 14:33

bind missing add icon in platform_view example

002401f

Minimise the diff to essential changes

1d2bc9a

This reverts some changes that were unnecessary to fix the display of the "+" icon in the native iOS view.

A few more reverts

45875fb

Add XCTests

a8b044f

Adds a test that verifies that an icon was loaded for the incrementButton.

cbracken force-pushed the master branch from e483c06 to a8b044f Compare February 26, 2024 22:33

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2024

cbracken merged commit 609d30f into flutter:master Feb 27, 2024

gspencergoog reviewed Feb 27, 2024

View reviewed changes

cbracken reviewed Feb 27, 2024

View reviewed changes

examples/platform_view/ios/Runner/Base.lproj/Main.storyboard Show resolved Hide resolved

cbracken mentioned this pull request Feb 27, 2024

[iOS] Fix naming in platform_view example #144247

Merged

10 tasks

engine-flutter-autoroll mentioned this pull request Feb 28, 2024

Roll Flutter from c30f998eb5db to 82ffde7ddfa2 (26 revisions) flutter/packages#6215

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024

609d30f bind missing add icon in platform_view example (flutter/flutt…

d4081ab

…er#132028)

engine-flutter-autoroll mentioned this pull request Feb 28, 2024

Roll Flutter from c30f998eb5db to d67580705242 (27 revisions) flutter/packages#6217

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024

609d30f bind missing add icon in platform_view example (flutter/flutt…

e1437ab

…er#132028)

engine-flutter-autoroll mentioned this pull request Feb 28, 2024

Roll Flutter from c30f998eb5db to 47b0ef8127d9 (28 revisions) flutter/packages#6220

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024

609d30f bind missing add icon in platform_view example (flutter/flutt…

e6a38e1

…er#132028)

engine-flutter-autoroll mentioned this pull request Feb 28, 2024

Manual roll Flutter from c30f998eb5db to d00bfe820eef (32 revisions) flutter/packages#6222

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024

609d30f bind missing add icon in platform_view example (flutter/flutt…

1dbbb92

…er#132028)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

609d30f bind missing add icon in platform_view example (flutter/flutt…

e7ffea6

…er#132028)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bind missing add icon in platform_view example #132028

bind missing add icon in platform_view example #132028

Uh oh!

TJRoger commented Aug 7, 2023 •

edited by cbracken

Loading

Uh oh!

flutter-dashboard bot commented Aug 7, 2023

Uh oh!

cbracken commented Nov 21, 2023 •

edited

Loading

Uh oh!

cbracken left a comment

Uh oh!

flutter-dashboard bot commented Nov 21, 2023

Uh oh!

goderbauer commented Jan 30, 2024

Uh oh!

TJRoger commented Feb 26, 2024

Uh oh!

cbracken commented Feb 26, 2024

Uh oh!

cbracken left a comment

Uh oh!

auto-submit bot commented Feb 27, 2024

Uh oh!

gspencergoog left a comment

Uh oh!

Uh oh!

Uh oh!

bind missing add icon in platform_view example #132028

bind missing add icon in platform_view example #132028

Uh oh!

Conversation

TJRoger commented Aug 7, 2023 • edited by cbracken Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Aug 7, 2023

Uh oh!

cbracken commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 21, 2023

Uh oh!

goderbauer commented Jan 30, 2024

Uh oh!

TJRoger commented Feb 26, 2024

Uh oh!

cbracken commented Feb 26, 2024

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Feb 27, 2024

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TJRoger commented Aug 7, 2023 •

edited by cbracken

Loading

cbracken commented Nov 21, 2023 •

edited

Loading