Enhancement/load more messages and maintain scroll position by azurechen · Pull Request #284 · MessageKit/MessageKit

azurechen · 2017-10-25T18:34:46Z

What does this implement/fix? Explain your changes.

Pull down to load more messages and keep the position.
The example was updated. Reviewers can try the effect at ConversationViewController

Does this close any currently open issues?

#186 Maintain scroll position after loading new messages

Where has this been tested?

Devices/Simulators: iPhone 6, iPhone 7s Simulators

iOS Version: 10

Swift Version: 3.2

MessageKit Version: 0.9.0

SD10 · 2017-10-26T03:59:09Z

Sources/Views/MessagesCollectionView.swift

        scrollToItem(at: indexPath, at: .bottom, animated: animated)
    }
+
+    override open func reloadData() {


@azurechen Thanks for submitting this PR! Unfortunately, I don't think overriding reloadData is the right approach to achieving this functionality.

I notice this is the second PR you've submitted recently. You should join the MessageKit Slack channel so we can discuss design decisions.

I think some MessageKit users have already implemented this functionality on their own, it would be good to discuss/compare it to what they've done.

Hi I have joined slack few days ago. Thanks so much.
I have explained my idea on the following comment. I don't think I really override the reloadData method I just extend it.

nathantannar4

Hey @azurechen I like this feature! I agree with @SD10 that we shouldn't override reloadData to get this functionality. Perhaps making a separate function that inserts new messages

azurechen · 2017-10-26T06:27:39Z

I have consider using another method to insert old messages. But let me explain why I think override reloadData is a better idea.

First, base on the pattern of original UICollectionView and UITableView. Developer should use the same messageList to handle the data source. If we use another method to insert old data not directly change messageList will break this pattern.
In other word, update the messageList is necessary, and reloadData should be necessary after developer change the value of messageList.

Second, in a sense I didn't really override the reloadData method because if not trigger the refreshing. The behavior is definitely same as the original reloadData. I just extend the ability.

if !messageLoadMoreControl.isRefreshing {
    super.reloadData()
    return
}

Extend reloadData can make load more function simple and elegant. Only needs two lines can enable load more function.

Register the selector

messageLoadMoreControl.addTarget(self, action: #selector(loadMoreMessages), for: .valueChanged)

And insert older data to the messageList

self.messageList.insert(contentsOf: messages, at: 0)

I have been thinking about how to use a beautiful way to implement it for a few days. I really want to contribute because I use MessageKit in my project even it is still on the beta state.
I may not often discuss on the Slack because my job. But I already join the room and keep following your discussion.

And please try the example, I believe the effect is satisfying.

cwalo · 2017-10-26T13:18:55Z

@azurechen I thought about this a bit more and I mentioned in slack, but I think a second method - reloadAndMaintainOffset() - might be more ideal. It makes the behavior clear and provides a mechanism for users to use whatever reload/refresh control they want. It would be up to the user to decide to call it or the standard reloadData method.

azurechen · 2017-10-26T15:14:32Z

@cwalo I think it is a perfect compromise!
I will take your suggestion and update the PR soon. Thank you.

cwalo

Can't approve until we get some more discussion on this.

cwalo · 2017-10-26T02:24:13Z

Sources/Views/MessagesCollectionView.swift

+
+            // reset the contentOffset after data is updated
+            self.setContentOffset(CGPoint(
+                x: self.contentOffset.x + (afterContentSize.width - beforeContentSize.width),


For readability, consider creating a let variable for the new offset and then use that in setContentOffset.

cwalo · 2017-10-26T15:44:51Z

Sources/Views/MessagesCollectionView.swift


    open weak var messageCellDelegate: MessageCellDelegate?
+
+    open var messageLoadMoreControl = UIRefreshControl()


I'm not so sure this should live in the collectionview. Perhaps move to MessagesViewController and add a method for adding/removing it. Similar to JSQ in that regard.

I agree moving it to MessagesViewController make sense. but because of !messageLoadMoreControl.isRefreshing. it have to be put in MessageCollectionView

I agree. It could also just be called refreshControl and lie in MessagesViewController

Yep, agree with you both 👍

Possibly, we don't even need to provide this control? The method is the most important thing. Then the user can hook it up to whatever they want? Need to think about this a little bit more

There is a possible issue that different control component has different sliding animation duration. if the animation not stop, it is hard to calculate the right offset.
Do you think we can temporarily keep this control, and provide another interface to user in later version to customize their own refreshControl?

I though an idea. Create a new class name MessageMoreControl but not extends UIRefreshControl.
And user have to setup the component by

messageMoreControl = MessageMoreControl(component: UIRefreshControl()) messageMoreControl.delegate = self

user can choose use UIRefreshControl or customized view

messageMoreControl = MessageMoreControl(component: CustomRefreshView()) messageMoreControl.delegate = self

and provide a MessageMoreControlDelegate for user load more data

protocol MessageMoreControlDelegate { func didPullDown(of messageMoreControl: MessageMoreControl) }

cwalo · 2017-10-26T15:46:40Z

Sources/Views/MessagesCollectionView.swift

    }
+
+    public func reloadAndMaintainOffset() {
+        if !messageLoadMoreControl.isRefreshing {


I'm waffling between whether or not this method should do any checking of the refresh control or if the user should do it. Otherwise, it should only calculate the new offset and refresh. If this were the case, the user could do any checking they need to, then just call super. @SD10 thoughts?

I agree, the user should probably do this outside of the call to reloadAndMaintainOffset() to determine if the method is called at all. Also, may need to improve method naming on this one 🤔

@SD10 could you suggest a name? I think the method name should include reloadData to imply that is a extended method of reloadData

cwalo

One question about automaticallyAdjustsScrollViewInsets = true. Otherwise, looks good. 👍

cwalo · 2017-10-30T16:04:02Z

Example/Sources/ConversationViewController.swift

    override func viewDidLoad() {
        super.viewDidLoad()
+
+        automaticallyAdjustsScrollViewInsets = true


Is this relevant to the PR?

for the UIRefreshControl of this example. If not set automaticallyAdjustsScrollViewInsets, the refreshControl will be covered by navigation bar.

alijaza · 2017-11-15T19:02:43Z

Will this be part of v0.11.0?

SD10 · 2017-12-04T07:18:42Z

@alijaza I'm reviewing this again now

I'm wondering if this functionality can be moved into the MessagesCollectionViewFlowLayout,
then users can toggle a flag to enable/disable this functionality.

See this comment

cc @cwalo What do you think?

…intain-scroll-position

SD10 · 2017-12-17T05:44:13Z

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! 😃

azurechen added 3 commits October 26, 2017 01:15

Add a UIRefreshControl to be the indicator of load more function.

74b90dd

Override the reloadData method of MessageCollectionView.

2fd137b

Update the example.

d17b500

SD10 requested review from cwalo and nathantannar4 October 26, 2017 02:12

SD10 reviewed Oct 26, 2017

View reviewed changes

nathantannar4 requested changes Oct 26, 2017

View reviewed changes

Replace reloadData by a new method reloadAndMaintainOffset.

a675650

cwalo reviewed Oct 26, 2017

View reviewed changes

nathantannar4 mentioned this pull request Oct 26, 2017

How to load earlier messages? #184

Closed

azurechen added 2 commits October 28, 2017 21:37

Remove refreshControl in MessageCollectionView.

85550ab

Update reloadDataAndKeepOffset function.

0832ba3

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

cwalo approved these changes Oct 30, 2017

View reviewed changes

SD10 added the under review label Oct 30, 2017

nathantannar4 approved these changes Oct 31, 2017

View reviewed changes

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

SD10 closed this Dec 4, 2017

SD10 reopened this Dec 4, 2017

Merge branch 'development' into enhancement/load-more-messages-and-ma…

7ac0324

…intain-scroll-position

SD10 changed the base branch from development to master December 17, 2017 05:27

SD10 changed the base branch from master to development December 17, 2017 05:28

SD10 approved these changes Dec 17, 2017

View reviewed changes

SD10 removed the under review label Dec 17, 2017

SD10 merged commit 67e6998 into MessageKit:development Dec 17, 2017

SD10 added a commit that referenced this pull request Dec 17, 2017

Add CHANGELOG entry for #284

1d4db92

This was referenced Dec 17, 2017

Maintain scroll position after loading new messages #186

Closed

Version 0.12.0 #415

Merged


		open weak var messageCellDelegate: MessageCellDelegate?

		open var messageLoadMoreControl = UIRefreshControl()

Conversation

azurechen commented Oct 25, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

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!

nathantannar4 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cwalo commented Oct 26, 2017

Uh oh!

azurechen commented Oct 26, 2017

Uh oh!

cwalo left a comment

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!

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!

cwalo left a comment

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!

alijaza commented Nov 15, 2017

Uh oh!

SD10 commented Dec 4, 2017

Uh oh!

SD10 commented Dec 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

azurechen commented Oct 26, 2017 •

edited by BasThomas

Loading