Add customizable accessoryView besides messageContainer by hyouuu · Pull Request #710 · MessageKit/MessageKit

hyouuu · 2018-06-02T09:13:03Z

Add an accessoryView and give client a chance to config it to whatever they like (e.g. a button, image)

Does this close any currently open issues?

Found one ;) #190

Any relevant logs, error output, etc?

NA

Any other comments?

Let me know how you want to approach it!
This was originally PR'ed for 0.12 at #432

Where has this been tested?

Devices/Simulators: iPhone X device; iPhone 8+ simulator

iOS Version: 11.4

Swift Version: 4.1

MessageKit Version: 1.0.0-beta.1

Example screenshot where in configCell added a UIView with green background:

SD10 · 2018-06-02T17:23:52Z

Sources/Protocols/MessagesDataSource.swift

+    ///   - message: The `MessageType` that will be displayed by this cell.
+    ///   - indexPath: The `IndexPath` of the cell.
+    ///
+    func configCell(_ cell: MessageContentCell, for message: MessageType, at indexPath: IndexPath)


I don't think this method fits well with this pull request 🤔

Well I know it feels too flexible, but couldn't figure out another way to customize an accessory view - thoughts?

All this method does is adds a layer of indirection for cellForRowAtIndexPath. Everyone subclasses MessagesViewController to use the library. They should just override cellForRowAtIndexPath and do this themselves

Maybe we could add a method to configure the accessory view just like we're doing for the avatar 🤔

Let me try to mimic the way avatar is doing

SD10 · 2018-06-02T17:25:54Z

Sources/Views/Cells/MessageContentCell.swift

        cellTopLabel.text = nil
        messageTopLabel.text = nil
        messageBottomLabel.text = nil
+        for view in accessoryView.subviews {


I'm not sure about this approach, to be honest. I don't like adding or removing views from the view hierarchy in cellForRowAtIndexPath. I think if you need to do this, it's a sign you need more cells or a better abstraction.

Yes it is kind of arbitrary. Maybe providing a similar method to the configCell, a prepareForReuse call so client can do their customized preparation? Or better thoughts?

I mean I really don't want to provide any method for this. Where would prepareForReuse get called? They can just override MessageContentCell and do this themselves. I don't think we should have a method in the controller handling preparing the reuse of cells....

SD10 · 2018-06-02T17:27:40Z

Sources/Views/Cells/MessageContentCell.swift

+    open func layoutAccessoryView(with attributes: MessagesCollectionViewLayoutAttributes) {
+        guard attributes.accessoryViewSize != .zero else { return }
+
+        let y = (bounds.height - attributes.accessoryViewSize.height) / 2


Doesn't the accessoryViewPadding allow the user to adjust the y position here?

Good point. Will update

I'm wondering what this looks like when we have varying label sizes, avatar sizes, etc. What would this look like if the avatar size is bigger than the message? What if there is a very large label? Lots of things to think about

SD10 · 2018-06-02T17:29:01Z

Sources/Protocols/MessagesLayoutDelegate.swift

+    ///
+    /// - Note:
+    ///   The default value returned by this method is zero.
+    func accessoryViewSize(for message: MessageType, in messagesCollectionView: MessagesCollectionView) -> CGSize


I'm not sure if we want to make this a delegate method or to put it as a property of the layout object.
@zhongwuzw, this is a feature MessageKit should support so I'd like your advice?

I guess the delegate calls should be different by indexPath's section or item? I could make the change

Ah but by moving it to size calculator, we'd need another display delegate method like the

func configureAvatarView(_ avatarView: AvatarView, for message: MessageType, at indexPath: IndexPath, in messagesCollectionView: MessagesCollectionView)

, and then do something like

if let layout = messagesCollectionView.collectionViewLayout as? MessagesCollectionViewFlowLayout { layout.textMessageSizeCalculator.incomingAvatarSize = CGSize(width: 52, height: 52) layout.textMessageSizeCalculator.outgoingAvatarSize = CGSize(width: 52, height: 52) layout.textMessageSizeCalculator.incomingAvatarPosition = AvatarPosition(vertical: .messageCenter) layout.textMessageSizeCalculator.outgoingAvatarPosition = AvatarPosition(vertical: .messageCenter) }

, which I personally feel pretty weird. Let me know what you think

The reason for this is I really wanted to move away from having users configure things with the delegates. I'd rather they abstract the logic for the layout of a cell into a CellSizeCalculator object. Then provide the layout object with the calculator.

IMO, doing it another way does not feel right. The API we're using here feels like it was designed for an earlier version of MessageKit and not version 1.0

@SD10 , My some thoughts, wether we can provide a flag to not use accessoryView, not just set size to .zero, because I think many of the time, maybe user don't need accessoryView at all.

@zhongwuzw what if we just make the default size for it .zero?

@SD10 , .zero can invisible to user, I mean it would leads to unnecessary calculations, render pass (yeah, it's true, though it's invisible to user, it still has these works). I'm a performancer 😢 .
I wonder wether we need add a boolean to control this.

Maybe later we would add more feature views, I don't think we add them all on cell is a good approach. 😂 .

@zhongwuzw this is true 👍 but I've learned not to prematurely optimize. I only optimize if someone complains and then if we do benchmarks afterward. We do cell layout using frames and not auto-layout, so we already save so much. This is likely the last view we will support. I will not add views that don't exist in JSQMVC

SD10 · 2018-06-02T21:37:15Z

Sources/Protocols/MessagesLayoutDelegate.swift

+    ///
+    /// - Note:
+    ///   The default value returned by this method is zero.
+    func accessoryViewPadding(for message: MessageType, in messagesCollectionView: MessagesCollectionView) -> UIEdgeInsets


Padding would definitely need to be defined through the cell calculator objects and not via delegate

hyouuu · 2018-06-03T02:13:28Z

@SD10 updated and now it looks much like avatarView :D Let me know what you think!

SD10 · 2018-06-03T02:18:15Z

@hyouuu Thanks for making these changes 💯 This is definitely heading in the right direction. I am still a little bit worried about the layout of the accessory view, that is something I'll need to test and review in more detail.

SD10 · 2018-06-03T02:19:58Z

Sources/Views/Cells/MessageContentCell.swift

    }()

+    // Should only add customized subviews - don't change accessoryView itself.
+    open var accessoryView: UIView = .init()


Can you change this to open var accessoryView: UView = UIView()?

SD10 · 2018-06-03T02:21:18Z

In addition to the layout of the accessoryView probably not being correct, this also changes the height calculations for the size of the cell. There are some scenarios where someone could set an accessoryView that is taller than the rest of the message content.

SD10 · 2018-06-03T02:24:14Z

Sources/Views/Cells/MessageContentCell.swift

+    /// Positions the cell's accessory view.
+    /// - attributes: The `MessagesCollectionViewLayoutAttributes` for the cell.
+    open func layoutAccessoryView(with attributes: MessagesCollectionViewLayoutAttributes) {
+        guard attributes.accessoryViewSize != .zero else { return }


We should remove this line and set the frame regardless. Since cells are re-used, this will cause bugs. I need to fix these on the other layout methods in another PR.

I removed this line alongside other zero checks - let me know if you want me to revert the change for others :)

…w height

hyouuu · 2018-06-04T07:13:34Z

@zhongwuzw for some reason I couldn't find the comments you 2 added (but received emails) on GitHub :/

I'm on the side with @SD10 that we probably don't want to optimize for the performance of this specific case at the moment, but later on we could add more flexible switches not only for accessoryView, but also for cellTopLabel & cellBottomLabel, which I guess many people don't need either

SD10 · 2018-06-04T16:09:49Z

@hyouuu That's a good suggestion, but as I said to @zhongwuzw I'm really not concerned. In previous versions of MessageKit it was taking ~6 seconds to lay out 10,000 cells. We've decreased that to probably around ~1 second. Realistically, no one should have that many cells on screen at a time. So until we have people complaining about performance, I'm not going to prematurely optimize anything

PS. You can probably find the comments from @zhongwuzw in the code review above. Hit "show outdated" from the drop down.

hyouuu · 2018-06-04T19:55:56Z

Regarding the tests, do you use Carthage to install Quick etc? Haven't used it and couldn't build the tests directly. Let me know if you want to test it yourself and correct the tests or want me to do it ;)

SD10 · 2018-06-04T23:34:28Z

Yeah @hyouuu we use Carthage to install the test dependencies. You’ll need to run carthage bootstrap

_{Sent with GitHawk}

hyouuu · 2018-06-04T23:56:28Z

Turned out the failed test was not related to this PR, but that we had .natural for avatar horizontal position, so set a default .cellLeading; also fixed a few formatting things from warnings

hyouuu · 2018-06-05T06:28:51Z

Added one more commit to fix keyboard frame & insets as I was facing issues in my app - let me know if you actually want it to be in a separate PR - if so I'll remove it from my current fork right before we can push the current PR (as I need to use my fork to keep things working ;)

hyouuu · 2018-06-05T06:30:27Z

Sources/Controllers/MessagesViewController+Keyboard.swift


    internal func addKeyboardObservers() {
        NotificationCenter.default.addObserver(self, selector: #selector(MessagesViewController.handleKeyboardDidChangeState(_:)), name: .UIKeyboardWillChangeFrame, object: nil)
+        NotificationCenter.default.addObserver(self, selector: #selector(MessagesViewController.handleKeyboardDidChangeState(_:)), name: .UIKeyboardDidChangeFrame, object: nil)


Note: only listening to UIKeyboardWillChangeFrame will cause issues in certain situations where keyboard was in the process to hide/show and then we get into our msgCollectionView, then we missed the willChange event and didn't catch up with didChange event

hyouuu · 2018-06-05T06:31:30Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+    /// - Returns: The safeAreaInsets.bottom if its an iPhone with safe areas (e.g. X), else 0
+    internal var safeAreaBottomInset: CGFloat {
        if #available(iOS 11.0, *) {
-            guard UIScreen.main.nativeBounds.height == 2436 else { return 0 }


This doesn't make sense and is hacky - if a device has safe bottom, then we should just use it, no matter if it's an iPhone X or a future iPhone X+ or a Safe iPad

It is a hack but the logic you changed it to doesn’t work

hyouuu · 2018-06-05T06:32:31Z

Sources/Controllers/MessagesViewController.swift

-        // Hack to prevent animation of the contentInset after viewDidAppear
-        if isFirstLayout {
-            defer { isFirstLayout = false }
-            addKeyboardObservers()


Since we unobserve in deinit, we should pair and start observation in init or viewDidLoad - not sure why we would want to do it here, so corrected

I can’t really see the diff well for this in GitHawk, but certain keyboard events we only observe once to get the behavior we want

hyouuu · 2018-06-05T06:32:55Z

Sources/Controllers/MessagesViewController.swift

-    
+
    open override func viewDidLayoutSubviews() {
-        // Hack to prevent animation of the contentInset after viewDidAppear


So, moved the initial bottomInset setting to viewDidAppear lol

This is too late to call it. It won’t look how I want it. I’d appreciate it if you keep this PR in scope. It makes it hard to approve because now I can reject it for keyboard related things

This reverts commit 081ca7a.

hyouuu · 2018-06-05T20:24:56Z

Cool I reverted the changes in this branch, and will use another one for my project atm :) Let's figure that issue out after this one @SD10

zhongwuzw · 2018-06-07T03:54:11Z

Sources/Views/Cells/MessageContentCell.swift

+    /// - attributes: The `MessagesCollectionViewLayoutAttributes` for the cell.
+    open func layoutAccessoryView(with attributes: MessagesCollectionViewLayoutAttributes) {
+        var y = (bounds.height - attributes.accessoryViewSize.height) / 2
+        y -= attributes.accessoryViewPadding.vertical + attributes.accessoryViewPadding.top


y maybe should align to the center.y of the bubble?
cc @SD10

yeah @zhongwuzw this is one of the reasons why I'm delaying this. everything seems to look good except the calculations for where it is located. we also have to think about having a large accessoryView but then the rest of the cell content is very small

this also cannot go in 1.0, it is a 2.0 feature for sure

Does bubble refer to messageContainerView? If so I could make the change

@hyouuu it's not necessarily that simple. what if the accessoryView is set to a height of 30pts, but the size of the other labels + the size of the message is only 20 points? if you try and center it to the messageContainerView your accessoryView will now leak out of the cell. there are a lot of edge cases here

@SD10 doesn't these lines https://github.com/MessageKit/MessageKit/pull/710/files#diff-ed5b75922e64407792c5a2dc5c27ebb6R107 cover the accessoryView height greater than the rest cases?

@hyouuu You're right this could work, but I'll need to test to make sure

Thanks! Let me know if there's anything I can help with :D

SD10

@hyouuu Can you please add a CHANGELOG entry for this under the Added section?

hyouuu · 2018-06-27T01:59:16Z

Done! @SD10

SD10 · 2018-06-27T02:00:39Z

CHANGELOG.md

 - Added convenience method to set properties for all `MessageSizeCalculator`s.
 [#697](https://github.com/MessageKit/MessageKit/pull/697) by [@zhongwuzw](https://github.com/zhongwuzw).

+- Added customizable `accessoryView`, with a new `MessagesDisplayDelegate` function `configureAccessoryView`, and corresponding size & padding properties in `MessageSizeCalculator`. [#710](https://github.com/MessageKit/MessageKit/pull/710) by [@hyouuu](https://github.com/hyouuu)


@hyouuu You will have to pull in the latest changes from master. The CHANGELOG is outdated now

Merge from upstream

hyouuu · 2018-06-27T02:10:13Z

Ah sorry - should be good now @SD10

SD10 · 2018-06-27T04:12:43Z

Thank you for contributing to MessageKit! I've invited you to join the MessageKit GitHub organization - no pressure to accept! If you'd like more information on what that means, check out our contributing guidelines and join the MessageKit Slack channel. Feel free to reach out if you have any questions! 😃

hyouuu · 2018-06-27T04:18:50Z

It's an honor!! Thank you :D

charvoa · 2018-06-28T09:32:44Z

@SD10 when do you think this feature will be merged with master ?

SD10 · 2018-06-28T14:51:25Z

@charvoa Not sure unfortunately. A rough estimation would be 2-4 weeks. Right now my company is using a fork of MessageKit. That's why development has been so slow lately. Next week we're going to try to get off the fork and back to the main project.

Add customizable accessoryView besides messageContainer

9c23282

SD10 reviewed Jun 2, 2018

View reviewed changes

tinder-sgong added 2 commits June 2, 2018 14:18

consider accessoryViewPadding vertical attributes

b36924e

Fix configCell stub cell type

3554e3f

SD10 reviewed Jun 2, 2018

View reviewed changes

mimic avatar

7fde2fd

SD10 reviewed Jun 3, 2018

View reviewed changes

Don’t ignore size calculation on zero size, and consider accessoryVie…

9c3032d

…w height

Fix broken test (not related to this PR), and formatting

4dabb02

keyboard frame done more correctly

081ca7a

hyouuu commented Jun 5, 2018

View reviewed changes

Revert "keyboard frame done more correctly"

2aa83ee

This reverts commit 081ca7a.

zhongwuzw reviewed Jun 7, 2018

View reviewed changes

Merge branch 'development' into development

4086f3f

SD10 added the under review label Jun 9, 2018

SD10 requested changes Jun 27, 2018

View reviewed changes

Added entry in changelog

bd1c1b3

SD10 reviewed Jun 27, 2018

View reviewed changes

tinder-sgong and others added 3 commits June 26, 2018 19:04

revert changelog

7dd498a

Merge pull request #1 from MessageKit/development

ee08f3c

Merge from upstream

update changelog

957f5ff

SD10 approved these changes Jun 27, 2018

View reviewed changes

SD10 merged commit fab22c5 into MessageKit:development Jun 27, 2018

SD10 removed the under review label Jun 27, 2018

SD10 mentioned this pull request Jun 27, 2018

Add support for accessory view in cell #190

Closed

SD10 mentioned this pull request Aug 15, 2018

[BLOCKED] Release MessageKit v1.1.0 #818

Closed

2 tasks



		open override func viewDidLayoutSubviews() {
		// Hack to prevent animation of the contentInset after viewDidAppear

Conversation

hyouuu commented Jun 2, 2018

Does this close any currently open issues?

Any relevant logs, error output, etc?

Any other comments?

Where has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Jun 2, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyouuu Jun 2, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Jun 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

hyouuu commented Jun 3, 2018

Uh oh!

SD10 commented Jun 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyouuu commented Jun 4, 2018

Uh oh!

SD10 commented Jun 4, 2018

Uh oh!

hyouuu commented Jun 4, 2018

Uh oh!

SD10 commented Jun 4, 2018

Uh oh!

SD10 Jun 2, 2018 •

edited

Loading

hyouuu Jun 2, 2018 •

edited

Loading

SD10 Jun 4, 2018 •

edited

Loading

SD10 commented Jun 3, 2018 •

edited

Loading

hyouuu commented Jun 5, 2018 •

edited

Loading