onmyway133 / DeepDiff

🦀Amazingly incredible extraordinary lightning fast diffing in Swift
https://onmyway133.com/apps/
Other
2.05k stars 143 forks source link

UICollectionView.reload(changes: ...) section-level equivalent missing. #32

Open jrogccom opened 5 years ago

jrogccom commented 5 years ago

Wondering what the impact is of replicating the reload logic from the UICollectionView+Extensions with the corresponding section-updating methods (reloadSections, deleteSections, insertSections, moveSections). I could submit a PR for this if this change hasn't been scoped out or hasn't been intentionally omitted.

snoozemoose commented 5 years ago

@jrogccom I started doing this some time ago but never fully finished it and I'd be happy to help out with this task. I could probably brush up the work that I've already done this coming weekend to get a starting point. But if you want to do it yourself I'll understand :-)

jrogccom commented 5 years ago

@snoozemoose For my own purposes I just copied the existing reload methods and kind of swapped in the section methods. What you were doing is probably more thorough so we should probably work off that. :)

snoozemoose commented 5 years ago

Sounds like it should be enough :-)! My solution was a whole-collection-matrix update where the reloadSection-part diff’ed the items in that section and called reloadItems. But I didn’t finish it so I’m not sure if it will work or if it’s an overkill solution. I’ll try to get it finished on Sunday and see if it’s worth having.

1 mars 2019 kl. 18:06 skrev JavierO notifications@github.com:

@snoozemoose For my own purposes I just copied the existing reload methods and kind of swapped in the section methods. What you were doing is probably more thorough so we should probably work off that. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

snoozemoose commented 5 years ago

@jrogccom I've spent a couple of hours on this today and I've got bad news, good news and some new code :-). Bad news first: if you implement the section logic in the same manner as the item logic is implemented today then you'll get exceptions thrown by the collection view if you do a section replace where the number of items in that section has changed. Good news is that you'll easily work around that by putting the outsideSectionsUpdate call inside the performBatchUpdates call. However, the animations in this case are so-so IMO since the cells will just cross fade into new cells. So the new code is that I've built is a section replace logic that calls the reloadItems logic in turn (formerly known as just reload). I've also made it so that all animated updates are done in a single performBatchUpdates call so the animations are synch'ed and nice. The negative is that this code became slightly hard to read and understand. If you're interested I could tidy it up a bit and publish it on my fork.

snoozemoose commented 5 years ago

@jrogccom @onmyway133 If it's interresting, check out my code in branch section_awareness: https://github.com/snoozemoose/DeepDiff/tree/section_awareness. It does introduce a struct Section which feels a bit limiting for a general implementation so any input on the code would be much appreciated. I've modified the Demo project so the new code is tested by just running that. Of course, the previous Demo code needs to be reinstated but it's just for quick testing.

jrogccom commented 5 years ago

@snoozemoose Interesting about the number of items .. 🤔. I'll checkout your branch when I get the chance!

onmyway133 commented 5 years ago

@jrogccom @snoozemoose thanks for the contributions to DeepDiff ❤️

For section diffing, the ideal scenario is to make this algorithm work on a matrix level (array of array). The solution that you did @snoozemoose is clever and that's what I intended to do when I started DeepDiff, it's simple and does the job. It misses "moving item across section" changes but I would say that is tricky to do.

Another thing is that, as I understand correctly, like in UITableView https://developer.apple.com/library/archive/documentation/UserExperience/Conceptual/TableView_iPhone/ManageInsertDeleteRow/ManageInsertDeleteRow.html, animation of items and sections function calls must be called in certain order for it to work properly, and sometimes trial and errors.

For what I'm doing now, a lazy solution, is to have just cells. I implement headers and footers as just normal cells so there is only 1 array of items.

@snoozemoose I would love you to submit a PR, with more tests so we can cover other common cases.

snoozemoose commented 5 years ago

@onmyway133 Your idea for a matrix level algorithm sounds very interesting and would be fun to dig into. "moving item across section" would be tricky to do in an efficient manner, to put it mildly :-), but that makes it such a fun challenge. Meanwhile, I'll work some more on the current section aware code and submit a PR when it's getting near ready. It might take a week or two before I get a chance to work on it but I'll get it done eventually :-)

ton252 commented 5 years ago

@snoozemoose I saw your solution. Very interesting idea, but it not works in this case:

    // Random swap 2 sections
    let i1 = Int.random(in: 0..<1)
    let i2 = i1 == 0 ? 1 : 0

    let tmp = newSections[i1]
    newSections[i1] = newSections[i2]
    newSections[i2] = tmp

    // Change number of elements in section

    newSections[i1].items = DataSet.generateItems()

I think the problem is in diffId in Section class