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

Replace DelegateProxy with concrete forward implementation #58

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

Conversation

toshi0383
Copy link
Contributor

@toshi0383 toshi0383 commented Aug 27, 2021

fixes: Issues like #23 , #36

I faced this non-reversed index issue with willDisplay delegate.


func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) {
let messages = sections[indexPath.section]
let message = messages[indexPath.row]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this PR, you likely to get crash here because given indexPath is not "reversed", which does not match indexPath passed to cellForRow.

.map { i -> [MessageModel] in
(0..<i)
.map {
MessageModel(imageName: "marty1", message: "\($0) hello", time: "00:00")
Copy link
Contributor Author

@toshi0383 toshi0383 Aug 27, 2021

Choose a reason for hiding this comment

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

@marty-suzuki
You can change MessageModel contents after merge, but please keep

  • displaying indexPath to screen
  • and to have different number of rows for each section

so it better demonstrates how the things work under the hood.

Copy link
Owner

@marty-suzuki marty-suzuki left a comment

Choose a reason for hiding this comment

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

Thank you for fixing issues.
Almost fixes are good. But to remove DelegateProxy is unconsidered implementing scrollviewWillbeginDragging and so on ​to outside of ReverseExtension. (scrollviewWillbeginDragging is one of example.)
On the other hand, to not remove DelegateProxy causes calling ReverseExtension's delegate implementations and outside delegate implementations.
We need to think about other solutions.

@marty-suzuki
Copy link
Owner

@toshi0383
I've fixed an issue of (#58 (review)) by #59 🛠
Would you mind reverting DelegateProxy deletion by 3ac2547 ?
On the other hand, would you mind re-committing concrete forward implementations like 3ac2547 ?

@toshi0383
Copy link
Contributor Author

Thank you! I’ll do when I have free time, but feel free to take over this one because it will probably be later this year.

@toshi0383 toshi0383 force-pushed the ts-section branch 2 times, most recently from 88da462 to 9cf32ec Compare October 22, 2021 00:43
to demonstrate indexPaths passed via re.delegate is correctly "reversed".
@toshi0383
Copy link
Contributor Author

@marty-suzuki
Rebased to master and re-commited forward implementation.
But it seems like section is not reversed. I will debug this later.

Bottom section (section index 0) should have 2 rows but has 10.
section-not-reversed

Let me know if you have any clue.🙏

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.

2 participants