Provided support for iOS 13 Context Menus #1430

jeromeboursier · 2020-02-25T15:52:41Z

Changes in this pull request

Issue fixed: Provided support for Context Menus

Checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
I have reviewed the contributing guide

jeromeboursier · 2020-02-26T07:29:29Z

Not sure about what to do with Travis failing, as per the following PR Fix the podlint error #1428 it seems it's a known issue.

lorixx · 2020-02-26T20:50:39Z

hey @jjbourdev , thanks for the PR! mind rebase from master and trigger the build again? thx!

lorixx

Overall looks good, pls fix the pod lint error (you could run the command locally) and ensure CI pass.

lorixx · 2020-02-27T01:39:17Z

Source/IGListKit/IGListSectionController.h

+
+ @note The default implementation does nothing. **Calling super is not required.**
+ */
+- (nullable UIContextMenuConfiguration *)contextMenuConfigurationForItemAtIndex:(NSInteger)index point:(CGPoint)point API_AVAILABLE(ios(13.0)) API_UNAVAILABLE(tvos);


Does this actually compile or work? I saw you have some lint error here: https://travis-ci.org/Instagram/IGListKit/jobs/655541844?utm_medium=notification&utm_source=github_status which is related to this file change.

Yes it does, I don't really get why it complains here, it seems it wants the nullable to be apart from the return type.
I double checked on all samples, deintegrating / reinstalling all pods, it works pretty well, on multiple iOS versions.

Here's the local execution of the lint:

$ machine:IGListKit jbourdev$ bundle exec pod lib lint IGListSwiftKit.podspec --allow-warnings "--include-podspecs=*.podspec" -> IGListSwiftKit (4.1.0) - NOTE | xcodebuild: note: Using new build system - NOTE | xcodebuild: note: Planning build - NOTE | xcodebuild: note: Constructing build description - NOTE | xcodebuild: warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App') IGListSwiftKit passed validation.

Travis builds on Xcode 10.2.1 - 10E1001 while I locally build on Xcode 11.3.1 - 11C504. It shouldn't be an issue though.

Same result if I do my setup the same as the CI... @lorixx any idea? :/

@lorixx The error is different from a CI run to another... It compiles properly locally, on all targets, and for each example app. How could I solve the issue so this PR gets accepted?

Flatout73 · 2020-07-16T17:50:19Z

Hey, what about merge?)

jeromeboursier · 2020-07-16T18:15:29Z

I wish I can :)

AYastrebov

What about merging it to the master?
@lorixx any ETA?

AYastrebov · 2021-02-04T20:13:40Z

Source/IGListKit/Internal/IGListAdapter+UICollectionView.m

+  // forward this method to the delegate b/c this implementation will steal the message from the proxy
+  id<UICollectionViewDelegate> collectionViewDelegate = self.collectionViewDelegate;
+  if ([collectionViewDelegate respondsToSelector:@selector(collectionView:contextMenuConfigurationForItemAtIndexPath:point:)]) {
+      [collectionViewDelegate collectionView:collectionView contextMenuConfigurationForItemAtIndexPath:indexPath point:point];


I think here should be return [collectionViewDelegate collectionView:collectionView contextMenuConfigurationForItemAtIndexPath:indexPath point:point];

Nope it's a call to the delegate.
I don't see anything blocking in this PR, apart from the fact it's getting old now :)

My 2 cents: I too think this line is missing a return in the beginning. As far as I'm aware collectionView:contextMenuConfigurationForItemAtIndexPath:point:'s purpose is to return a context menu configuration, not simply to notify the delegate (see docs).

If you do return here you would give the developer option to either add context menu configuration to collectionViewDelegate or to sectionController. If you do not return here there's no reason to implement this method in collectionViewDelegate since it wouldn't do anything.

At least I would have been surprised if I had implemented collectionView:contextMenuConfigurationForItemAtIndexPath:point: in collectionViewDelegate, returned proper configuration and it wouldn't cause context menu to show up.

sjchmiela · 2021-09-02T09:00:20Z

Source/IGListKit/IGListBindingSectionControllerSelectionDelegate.h

+
+ @return An object that conforms to `UIContextMenuConfiguration`.
+ */
+- (UIContextMenuConfiguration * _Nullable)sectionController:(IGListBindingSectionController *)sectionController


I'm not super familiar with IGListKit, but this seems like an unnecessary forwarding of the call — after all it's IGListBindingSectionController**Selection**Delegate, not a "general delegate" — why would a "delegate that receives selection events from cells" be interested in context menu events?

amalenduk · 2023-04-12T15:02:12Z

Hi, Any update on this

facebook-github-bot · 2023-04-26T09:48:02Z

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

facebook-github-bot · 2023-04-27T08:28:40Z

@jeromeboursier has updated the pull request. You must reimport the pull request before landing.

TimOliver · 2023-05-02T07:14:07Z

Hi folks! I'm so sorry for the amount of time it's taken to get back to this PR. I had a play with the PR in the sample app, and I gotta say this is a pretty dope feature!

I tried importing it into the Instagram codebase, and after a bit of review, I think we need to tweak it a bit. Namely, that new method in IGListBindingSectionControllerSelectionDelegate implicitly adds UIKit as a required import for the protocol, which might have ramifications down the line.

We might need to have a think about this, but it might be more appropriate to add a new property/protocol to IGListBindingSectionController to handle context menu configurations in its own silo separate from cell configuration. This also gives us a better space to add more of the context menu protocol methods down the line.

What do you think?

amalenduk · 2023-09-15T09:39:37Z

Hi @TimOliver , Any update on this?

facebook-github-bot added the CLA Signed label Feb 25, 2020

jeromeboursier force-pushed the feature/support-for-context-menu branch from b38510c to 78a5fc0 Compare February 26, 2020 21:15

aaronbrethorst mentioned this pull request Feb 26, 2020

Add support for context menus on iOS 13 OneBusAway/onebusaway-ios#55

Closed

lorixx requested changes Feb 27, 2020

View reviewed changes

jeromeboursier force-pushed the feature/support-for-context-menu branch from 2a6dfb5 to 92e927a Compare March 3, 2020 13:17

lorixx mentioned this pull request Oct 21, 2020

Swift Package Manager support #1465

Closed

5 tasks

AYastrebov reviewed Feb 4, 2021

View reviewed changes

sjchmiela reviewed Sep 2, 2021

View reviewed changes

sjchmiela mentioned this pull request Sep 2, 2021

Provide support for context menus luma-team/IGListKit#1

Merged

jeromeboursier force-pushed the feature/support-for-context-menu branch from dc15a92 to e662826 Compare September 2, 2021 09:47

Jerome Boursier added 2 commits April 27, 2023 17:28

Provided support for iOS 13 Context Menus

5d7936b

Changed nullable keyword to _Nullable and added a missing return

95fb46b

TimOliver force-pushed the feature/support-for-context-menu branch from 3b2f186 to 95fb46b Compare April 27, 2023 08:28

Asmodeusxxx85 approved these changes Jul 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provided support for iOS 13 Context Menus #1430

Provided support for iOS 13 Context Menus #1430

jeromeboursier commented Feb 25, 2020 •

edited

Loading

jeromeboursier commented Feb 26, 2020

lorixx commented Feb 26, 2020

lorixx left a comment

lorixx Feb 27, 2020

jeromeboursier Feb 27, 2020 •

edited

Loading

jeromeboursier Feb 27, 2020

jeromeboursier Feb 27, 2020

jeromeboursier Feb 27, 2020

jeromeboursier Mar 3, 2020

Flatout73 commented Jul 16, 2020

jeromeboursier commented Jul 16, 2020

AYastrebov left a comment

AYastrebov Feb 4, 2021

jeromeboursier Feb 16, 2021

sjchmiela Sep 2, 2021

sjchmiela Sep 2, 2021 •

edited

Loading

amalenduk commented Apr 12, 2023

facebook-github-bot commented Apr 26, 2023

facebook-github-bot commented Apr 27, 2023

TimOliver commented May 2, 2023

amalenduk commented Sep 15, 2023

Provided support for iOS 13 Context Menus #1430

Are you sure you want to change the base?

Provided support for iOS 13 Context Menus #1430

Conversation

jeromeboursier commented Feb 25, 2020 • edited Loading

Changes in this pull request

Checklist

jeromeboursier commented Feb 26, 2020

lorixx commented Feb 26, 2020

lorixx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromeboursier Feb 27, 2020 • edited Loading

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

Flatout73 commented Jul 16, 2020

jeromeboursier commented Jul 16, 2020

AYastrebov 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

sjchmiela Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

amalenduk commented Apr 12, 2023

facebook-github-bot commented Apr 26, 2023

facebook-github-bot commented Apr 27, 2023

TimOliver commented May 2, 2023

amalenduk commented Sep 15, 2023

jeromeboursier commented Feb 25, 2020 •

edited

Loading

jeromeboursier Feb 27, 2020 •

edited

Loading

sjchmiela Sep 2, 2021 •

edited

Loading