Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor handling updates #779

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor handling updates #779

wants to merge 2 commits into from

Conversation

wiruzx
Copy link
Contributor

@wiruzx wiruzx commented Jul 18, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #779 (1aeaea4) into master (82b430e) will increase coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 1aeaea4 differs from pull request most recent head 7b7fa5a. Consider uploading reports for the commit 7b7fa5a to get more accurate results

@@            Coverage Diff             @@
##           master     #779      +/-   ##
==========================================
+ Coverage   47.63%   47.69%   +0.05%     
==========================================
  Files         133      133              
  Lines        6491     6498       +7     
==========================================
+ Hits         3092     3099       +7     
  Misses       3399     3399              
Impacted Files Coverage Δ
...er/ChatMessages/New/CollectionUpdateProvider.swift 0.00% <0.00%> (ø)
...Messages/New/NewChatMessageCollectionAdapter.swift 1.55% <0.00%> (+0.05%) ⬆️
...NewChatMessageCollectionAdapterConfiguration.swift 0.00% <0.00%> (ø)
Chatto/sources/SerialTaskQueue.swift 62.16% <0.00%> (-37.84%) ⬇️
...Chat Items/TextMessages/Views/TextBubbleView.swift 81.38% <0.00%> (+3.19%) ⬆️
...oller/Collaborators/ChatItemPresenterFactory.swift 61.53% <0.00%> (+3.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82b430e...7b7fa5a. Read the comment docs.

@@ -31,18 +32,29 @@ public final class NewChatMessageCollectionAdapter: NSObject,
// MARK: - Private type declarations

private struct ModelUpdates {
struct ScrollPositionData {
let oldRect: CGRect
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just naming. Do you think it would make sense to name it just rect and the property holding this value would call it oldScrollPositionData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What ScrollPositionData essentially contains is an old rect, and a new indexPath.

Should I rename referenceIndexPath to newIndexPath, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, got it. Let's keep it like this then 👍

let changes = updates.changes
let layout = updates.layoutModel
let visibleCellsAreValid = self.visibleCellsAreValid(changes: updates.changes)
let wantsReloadData = updateType != .normal && updateType != .firstSync
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do switch to enforce that we handle all possible new use cases?

guard let self = self, let collectionView = collectionView else { return }

switch scrollAction {
case .scrollToBottom:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just to confirm) Is this not needed anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants