Adding copy/paste support from MessagesViewController by Ganzolo · Pull Request #248 · MessageKit/MessageKit

Ganzolo · 2017-10-09T10:02:43Z

(Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.)

What does this implement/fix? Explain your changes.

Copy implementation

Does this close any currently open issues?

#143

Any other comments?

Better implementation

SD10 · 2017-10-09T18:27:10Z

        registerReusableViews()
        setupDelegates()

+		self.addMenuControllerObservers()


Can we remove self here?

SD10 · 2017-10-09T18:28:39Z

        addGestureRecognizer(tapGesture)
        tapGesture.delegate = self

-        let longPressGesture = UILongPressGestureRecognizer(target: self, action: #selector(handleGesture(_:)))


I still have to look into this

SD10 · 2017-10-09T18:28:58Z

+
+extension MessagesViewController: UICollectionViewDelegate {
+
+	public func collectionView(_ collectionView: UICollectionView, shouldShowMenuForItemAt indexPath: IndexPath) -> Bool {


This needs to be marked as open

SD10 · 2017-10-09T18:29:02Z

+		return false
+	}
+
+	public func collectionView(_ collectionView: UICollectionView, canPerformAction action: Selector, forItemAt indexPath: IndexPath, withSender sender: Any?) -> Bool {


This needs to be marked as open

SD10 · 2017-10-09T18:29:06Z

+		return (action == NSSelectorFromString("copy:"))
+	}
+
+	public func collectionView(_ collectionView: UICollectionView, performAction action: Selector, forItemAt indexPath: IndexPath, withSender sender: Any?) {


This needs to be marked as open

SD10 · 2017-10-09T18:29:57Z

+	}
+
+	fileprivate var targetedCell:UICollectionViewCell?
+	fileprivate var targetedFrame = CGRect.zero


Can we keep properties at the top of the file like we're doing with the rest of them?
Can we also put all of the logic related to the MenuController in an extension under Keyboard Handling?

SD10

Thanks @Ganzolo 👍 I just have to review the Long Press Gesture and any other gesture conflicts. I left you some comments regarding how I think we should organize the code

Ganzolo · 2017-10-10T09:09:38Z

@SD10 Thanks for feedbacks. I did all suggested fixes. I have nothing to say about what u suggested, except for putting all properties at the top. I usually like to put them close to methods that are using it so refactoring is easier, scope is more concise.

However, then we cannot use extensions which is also a good way to break the code. So both solution is fine with me. The best would be to be able to add properties in an extension :)

SD10 · 2017-10-17T01:36:07Z

@Ganzolo Can you add a CHANGELOG entry for this?

SD10 · 2017-10-20T21:27:36Z

Me too, I just have to make sure the gestures are behaving properly. If either of you can pull it and run it that would be great. I will try myself again soon.

I think there was some inconsistency with the gestures last time I tested it. Sometimes the copy menu would be displayed for a long press. Sometimes for a long press and lift up. Things like that.

nathantannar4

Looks like a good feature we need

SD10 · 2017-10-20T21:44:43Z

@nathantannar4 Were you able to test the gestures?Yes we definitely need it but it needs to be working properly.

_{Sent with GitHawk}

nathantannar4 · 2017-10-21T02:35:34Z

@SD10 @Ganzolo I notice that the "Copy" prompt appears even when a long press is recognized on the AvatarView or anywhere else outside of the MessageLabel. Is this behavior we want?

Only other thing I see is if you long press a link such as an address, the "Copy" prompt appears before the notifier is sent that the user has tapped an address. I wonder if this can be prevented?

SD10 · 2017-10-21T09:26:46Z

@nathantannar4 Thanks for the extra set of eyes. I definitely missed some of those. These are all valid points. Looks like we're going to have to look into this a little bit more.

It's a pretty important feature to have but easy enough for clients to implement on their own while we figure this out.

Ganzolo · 2017-10-23T08:09:59Z

@SD10 @nathantannar4
I looked today into this issue and I am afraid that we would need to use custom touch on a Cell level to prevent this behavior from happening.

I tried to make contentView transparent background or to assign a long press gesture to background of cell. But it does not work.

The other idea would be to respond false when this method is called only when the touch is not in the bubble area. However I didn't find a way to get the position of the touch from this method.

func collectionView(_ collectionView: UICollectionView, shouldShowMenuForItemAt indexPath: IndexPath) -> Bool

cwalo · 2017-10-26T16:27:17Z

Sorry, just now getting to this. For your consideration - add a long press gesture recognizer to MessageLabel and either pass the gesture type (maybe via a custom enum) along in the MessageLabelDelegate methods and/or have an additional didLongPressMessageLabel delegate method with a default implementation that displays the MenuController. @Ganzolo @SD10 @nathantannar4

SD10 · 2017-10-26T23:29:22Z

@cwalo Good suggestions. We may have to move this to the cell level as @Ganzolo mentioned. I'm going to be rewriting MessageLabel and then we can re-evaluate from there

relaxsus · 2017-11-20T10:02:38Z

Hello! I can't call the menu with long press (copy, custom action...). when will this function be in MessageKit? Or help me, please, to write the right code. It was not the problem to call this menu on JSQ.

SD10 · 2017-11-20T14:14:26Z

Hey @relaxsus,

You can remove the long press gesture from your MessageKit copy for now. Right now we require people to add copy/paste support themselves.

I agree this feature is something we should provide, but due to its ease of implementation outside of MessageKit, I’ve spent my time trying to resolve more serious issues. It’s on the roadmap though

_{Sent with GitHawk}

nathantannar4 · 2018-01-10T22:50:29Z

#422

Adding copy/paste support from MessagesViewController

4e54c47

SD10 reviewed Oct 9, 2017

View reviewed changes

SD10 requested changes Oct 9, 2017

View reviewed changes

SD10 mentioned this pull request Oct 9, 2017

[WIP] Making copy feature using partial Collection view mechanism. Could no… #155

Closed

SD10 changed the base branch from master to v0.10.0 October 10, 2017 00:58

Code formatting

db66955

SD10 assigned cwalo and nathantannar4 and unassigned cwalo and nathantannar4 Oct 20, 2017

SD10 requested review from cwalo and nathantannar4 October 20, 2017 21:17

nathantannar4 approved these changes Oct 20, 2017

View reviewed changes

SD10 changed the base branch from v0.10.0 to v0.11.0 October 30, 2017 01:58

SD10 changed the base branch from v0.11.0 to development November 2, 2017 03:57

zhongwuzw mentioned this pull request Dec 22, 2017

Added copy support for message bubble #422

Merged

nathantannar4 closed this Jan 10, 2018

SD10 deleted the feature/copySupportForMessageTextAndImage branch February 23, 2018 12:03


		extension MessagesViewController: UICollectionViewDelegate {

		public func collectionView(_ collectionView: UICollectionView, shouldShowMenuForItemAt indexPath: IndexPath) -> Bool {

Conversation

Ganzolo commented Oct 9, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Uh oh!

SD10 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

SD10 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

SD10 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

SD10 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

SD10 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

SD10 Oct 9, 2017 • 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!

SD10 left a comment

Choose a reason for hiding this comment

Uh oh!

Ganzolo commented Oct 10, 2017

Uh oh!

SD10 commented Oct 17, 2017

Uh oh!

SD10 commented Oct 20, 2017

Uh oh!

nathantannar4 left a comment

Choose a reason for hiding this comment

Uh oh!

SD10 commented Oct 20, 2017

Uh oh!

nathantannar4 commented Oct 21, 2017

Uh oh!

SD10 commented Oct 21, 2017

Uh oh!

Ganzolo commented Oct 23, 2017

Uh oh!

cwalo commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SD10 commented Oct 26, 2017

Uh oh!

relaxsus commented Nov 20, 2017

Uh oh!

SD10 commented Nov 20, 2017

Uh oh!

nathantannar4 commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SD10 Oct 9, 2017 •

edited

Loading

cwalo commented Oct 26, 2017 •

edited

Loading