Messagekit: collectionView's didSelectItemAt: not call when touching Cells

[jdriselvato]

General Information

• MessageKit Version: v0.11.0 (Pods)

• iOS Version(s): 11.1

• Swift Version: 4.0

• Devices/Simulators: Both

• Reproducible in ChatExample? (Yes/No): YES

What happened?

• I tap a bubble, avatar, date, title and the respective MessageCellDelegate are called.
• I tap a bubble, avatar, date, title and collectionView's didSelectItemAt: does not get called
• I tap outside of any of those items (the white space around it) didSelectItemAt: gets called.

What did you expect to happen?

• I tap any part of a cell and collectionView's didSelectItemAt: should be called

Why?

I need to identify the MessageType for selected cell.

I did find this: https://github.com/MessageKit/MessageKit/issues/154, which works if you do this:

func didTapMessage<T>(in cell: MessageCollectionViewCell<T>) {
        let indexPath = self.messagesCollectionView.indexPath(for: cell)
}

Is this the only expected way to get the index?

[SD10]

I'm not sure about this one. @nathantannar4 @zhongwuzw do you think we should block the UICollectionViewDelegate method didSelectItemAt: or allow them to be recognized simultaneously? Have to look into this one

[nathantannar4]

I'll look at this when I have a moment. Issue could be because we have our other tap gestures which don't allow for simultaneous recognition with the cell tap gestures. I think it would be ok to block the method as we already have more detailed tap delegate methods. I also don't think a user should be able to tap outside of the message bubble and get the same action as tapping the bubble (which would be the case if a developer placed the login in didSelectItemAt:)

_{Sent with GitHawk}

[zhongwuzw]

@SD10 , I think we needn't support didSelecItemAt:, we have added a tap gesture on cell's contentView, maybe we can add a delegate method in MessageCellDelegate, like didTapMessageCell(in cell: MessageCollectionViewCell, at position: CGPoint), and always call it in handleTapGesture: method, like:

    @objc
    open func handleTapGesture(_ gesture: UIGestureRecognizer) {
        guard gesture.state == .ended else { return }

        let touchLocation = gesture.location(in: self)

        switch true {
        case messageContainerView.frame.contains(touchLocation) && !cellContentView(canHandle: convert(touchLocation, to: messageContainerView)):
            delegate?.didTapMessage(in: self)
        case avatarView.frame.contains(touchLocation):
            delegate?.didTapAvatar(in: self)
        case cellTopLabel.frame.contains(touchLocation):
            delegate?.didTapTopLabel(in: self)
        case cellBottomLabel.frame.contains(touchLocation):
            delegate?.didTapBottomLabel(in: self)
        default:
            break
        }
        delegate?.didTapMessageCell(in: self, at: touchLocation)
    }

And also we need to change the name didTapMessage(in: self) to didTapMessageBubble(in: self).

[SD10]

I'll mark this as a feature request to add a new method to MessageCellDelegate that handles a touch in the underlying container.

[SD10]

This is probably a good addition to MessageKit 2.0

[SD10]

We definitely need to add a method for when you just tap the cell in general and none of the other delegates handle it

[SD10]

@zhongwuzw Do you think we should call didSelectItem(at: IndexPath) manually when all of our gesture recognizers fail? 🤔

[zhongwuzw]

@SD10 , In my opinion, we shouldn't call collectionView(_:didSelectItemsAt:) manually, because this delegate is a selected state. That is say, cell would be in selected state, then system would call collectionView(_:didSelectItemsAt:) to notify user the cell status .
I think we need to provide a custom delegate like didTapMessage which is be called when all of our gesture recognizers fail. 🤔

[funct7]

Not sure if anyone else is interested, but I made it so that the tap gesture recognizer begins only if the cell at touch location is MessageContentCell.

Please have a look at the forked branch and let me know what you guys think.

Not sure about how pull requests work here.

[nathantannar4]

Would this case be covered by #922 now then?

[funct7]

There is a collectionView(_:didSelectItemsAt:) but you guys break the parent class behavior, and provide another delegate method that does essentially the same thing, except instead of being handled at the collection view level, it's sent to a gesture recognizer and the gesture recognizer calls it. At the same time, clients are forced to use a MessageCollectionViewCell.

I'm out of here.

[nathantannar4]

@funct7 You have been awfully critical and negative to support we have tried to provide to you. I'm sorry but you are wrong, we are not breaking the parent class behaviour. We are not disabling tap gesture, we are adding ones. I would encourage you to be more friendly to the open source community and those that make the projects possible rather than criticizing their work.

[funct7]

@nathantannar4 I apologize for being rather harsh. On a tight schedule here, so I lost my patience. Still, not a good reason for being rude. I'm sorry for that.

Nevertheless, I still think you guys are making bad decisions.

we are not breaking the parent class behaviour

If this were true, collectionView(_:didSelectItemsAt:) would be called when selecting a cell, regardless of the cell type. I just checked out the code on development. This is still not fixed.

We are not disabling tap gesture, we are adding ones.

This is exactly what I mean. You guys add a tap gesture that's not necessary, while doing so, breaking the parent class behavior.

For me, "breaking the parent class behavior" means "a class claims to be a subtype of some type, but doesn't work like the super type does." "MessagesCollectionView claims to be a subtype of UICollectionView but doesn't work like the UICollectionView does."

A UICollectionView subclass is supposed to call delegate methods when a cell is selected. I tap on MessagesCollectionView, nothing happens. The reason is the cell is not a subclass of MessageCollectionViewCell. The UICollectionView never stipulated that.

If you go back to my comments I say people should not be punished for not using MessageContentCell. If you can't see why that's a bad thing, I really have no hope here.

Same goes for the input bar issue. contentInset is there for a reason. If you want to override it, you must override it in a way that supports all that UICollectionView supports. Otherwise, people have no idea why things don't work.

[SD10]

Thanks for apologizing @funct7 🙃

Nevertheless, I still think you guys are making bad decisions.

I've definitely made a ton of bad decisions in MessageKit! The thing about having an open source framework is there are a lot of things you have to live with for the sake of stability. I tried to support way too many people's needs and I can't rewrite the project for everyone who opens a GitHub issue.

That being said, it doesn't mean we can't solve things over time. You're also more than welcome to discuss new implementation details and or contribute that work 😄

If you go back to my comments I say people should not be punished for not using MessageContentCell. If you can't see why that's a bad thing, I really have no hope here.

I agree with this 👍

A UICollectionView subclass is supposed to call delegate methods when a cell is selected. I tap on MessagesCollectionView, nothing happens. The reason is the cell is not a subclass of MessageCollectionViewCell. The UICollectionView never stipulated that.

I haven't looked at this part of the codebase in a while, so I'm not 100% sure why this is set up that way. Don't have time to look at your fork either, but maybe this is something we can improve?

[egorafanasenko]

I believe this stackoverflow could help to call a didSelectItemAt

[milosuzice]

Any news on this issue?