feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction #41125

okwasniewski · 2023-10-20T15:10:57Z

Summary:

The goal of this PR is to migrate deprecated UIMenuController to UIEditMenuInteraction. UIMenuController has been deprecated in iOS 16 and for that reason it's not available for VisionOS.

Recording

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-10-20.at.17.10.28.mp4

Changelog:

[IOS] [CHANGED] - Migrate RCTTextView to UIEditMenuInteraction

Test Plan:

Launch RNTester and check for "Selectable Text" example and check that it works for iOS 16/17.

okwasniewski · 2023-10-20T15:27:26Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

@@ -16,6 +16,14 @@

 #import <QuartzCore/QuartzCore.h>

+#if __IPHONE_16_0


This needs to be changed as it will only work for iOS 16.

Do you know any better way to conditionally conform to delegate?

Take a look at the types of checks in this change, and the ones linked in the notes: #39549

the availability header has some good macros to use and patterns. I deleted some examples in an earlier commit since we bumped minimum OS, hence the links to commits :P

Found that #ifdef __IPHONE_16_0 works best as its also true for iOS 17

analysis-bot · 2023-10-20T15:40:53Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,653,257	+17
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,035,028	+11
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 3f621df
Branch: main

cipolleschi

I left a nit for readability of the code, but it looks good to me.

cipolleschi · 2023-10-23T10:20:16Z

packages/react-native/Libraries/Text/Text/RCTTextView.mm

+      if (_editMenuInteraction) {
+          [_editMenuInteraction presentEditMenuWithConfiguration:config];
+      }
+  } else {


nit: can we do something like

if (@available(iOS 16.0, *)) { // your code return } // old code

a part from me, preferring early returns when reading code, it will also make the diff more readable as the body of the now non-existing else will not be indented and github will not highlight it as changed code.

Thanks for the review! I've applied your suggestion. I saw that linter step failed - I'm curious what do you use to format Objective-C code in Xcode?

It's... a complicated question.
We use the same tool we have in OSS, but:

the version we use internally have been modified by some other Meta engineer

the set of rules we use internally are different and spread in different places, so they are very hard to retrieve.

We already tried several times to align the internal and external signals, but it is a harder problem than it look... :(

Ah okay I get it, thanks for the explanation

facebook-github-bot · 2023-10-23T10:21:53Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-10-23T11:54:55Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/react-native/Libraries/Text/Text/RCTTextView.mm

facebook-github-bot · 2023-10-25T16:32:17Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

okwasniewski · 2023-10-27T08:17:07Z

Hey @cipolleschi is there any further action needed to merge this PR? I've rebased the PR (I saw that you reverted the asset removal PR that made windows build fail)

cipolleschi · 2023-10-27T13:49:51Z

@okwasniewski no, I just created the PR to see if it was fixing but then closed without merging because it wasn't the cause.
So that commit is still in main.

No other things on your side, I'll reimport it and try to land it.

facebook-github-bot · 2023-10-27T13:50:27Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-10-27T18:15:42Z

@cipolleschi merged this pull request in e08a197.

…on (facebook#41125) Summary: The goal of this PR is to migrate deprecated `UIMenuController` to `UIEditMenuInteraction`. `UIMenuController` has been deprecated in iOS 16 and for that reason it's not available for VisionOS. ## Recording https://github.com/facebook/react-native/assets/52801365/fed994be-d444-462a-9ed0-39b50531425d bypass-github-export-checks ## Changelog:  [IOS] [CHANGED] - Migrate RCTTextView to UIEditMenuInteraction Pull Request resolved: facebook#41125 Test Plan: Launch RNTester and check for "Selectable Text" example and check that it works for iOS 16/17. Reviewed By: javache Differential Revision: D50551016 Pulled By: cipolleschi fbshipit-source-id: 558ecc5a04a5daa9c4360fabddcab28fba72a323

…on (facebook#41125) Summary: The goal of this PR is to migrate deprecated `UIMenuController` to `UIEditMenuInteraction`. `UIMenuController` has been deprecated in iOS 16 and for that reason it's not available for VisionOS. https://github.com/facebook/react-native/assets/52801365/fed994be-d444-462a-9ed0-39b50531425d bypass-github-export-checks  [IOS] [CHANGED] - Migrate RCTTextView to UIEditMenuInteraction Pull Request resolved: facebook#41125 Test Plan: Launch RNTester and check for "Selectable Text" example and check that it works for iOS 16/17. Reviewed By: javache Differential Revision: D50551016 Pulled By: cipolleschi fbshipit-source-id: 558ecc5a04a5daa9c4360fabddcab28fba72a323

…on (facebook#41125) Summary: The goal of this PR is to migrate deprecated `UIMenuController` to `UIEditMenuInteraction`. `UIMenuController` has been deprecated in iOS 16 and for that reason it's not available for VisionOS. ## Recording https://github.com/facebook/react-native/assets/52801365/fed994be-d444-462a-9ed0-39b50531425d bypass-github-export-checks ## Changelog:  [IOS] [CHANGED] - Migrate RCTTextView to UIEditMenuInteraction Pull Request resolved: facebook#41125 Test Plan: Launch RNTester and check for "Selectable Text" example and check that it works for iOS 16/17. Reviewed By: javache Differential Revision: D50551016 Pulled By: cipolleschi fbshipit-source-id: 558ecc5a04a5daa9c4360fabddcab28fba72a323

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 20, 2023

okwasniewski commented Oct 20, 2023

View reviewed changes

okwasniewski force-pushed the okwasniewski/RCTTextView branch from 52976f1 to c12cd0d Compare October 22, 2023 08:35

cipolleschi approved these changes Oct 23, 2023

View reviewed changes

okwasniewski mentioned this pull request Oct 24, 2023

Migrate UIMenuController to UIEditMenuInteraction callstack/react-native-visionos#19

Closed

javache reviewed Oct 25, 2023

View reviewed changes

packages/react-native/Libraries/Text/Text/RCTTextView.mm Outdated Show resolved Hide resolved

okwasniewski added 3 commits October 27, 2023 10:16

feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction

81d6afd

fix: change if check to early returns

8afffa0

fix: remove unnecessary ifdef

adabfb9

okwasniewski force-pushed the okwasniewski/RCTTextView branch from ff86246 to adabfb9 Compare October 27, 2023 08:16

facebook-github-bot closed this in e08a197 Oct 27, 2023

facebook-github-bot added the Merged This PR has been merged. label Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction #41125

feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction #41125

okwasniewski commented Oct 20, 2023

okwasniewski Oct 20, 2023

Saadnajmi Oct 20, 2023

Saadnajmi Oct 20, 2023

okwasniewski Oct 22, 2023

analysis-bot commented Oct 20, 2023 •

edited

Loading

cipolleschi left a comment

cipolleschi Oct 23, 2023

okwasniewski Oct 23, 2023

cipolleschi Oct 23, 2023

okwasniewski Oct 23, 2023

facebook-github-bot commented Oct 23, 2023

facebook-github-bot commented Oct 23, 2023

facebook-github-bot commented Oct 25, 2023

okwasniewski commented Oct 27, 2023

cipolleschi commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

		@@ -16,6 +16,14 @@

		#import <QuartzCore/QuartzCore.h>

		#if __IPHONE_16_0

feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction #41125

feat(iOS): migrate deprecated UIMenuController to UIEditMenuInteraction #41125

Conversation

okwasniewski commented Oct 20, 2023

Summary:

Recording

Changelog:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot commented Oct 20, 2023 • edited Loading

cipolleschi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 23, 2023

facebook-github-bot commented Oct 23, 2023

facebook-github-bot commented Oct 25, 2023

okwasniewski commented Oct 27, 2023

cipolleschi commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

analysis-bot commented Oct 20, 2023 •

edited

Loading