ekazaev / ChatLayout

ChatLayout is an alternative solution to MessageKit. It uses custom UICollectionViewLayout to provide you full control over the presentation as well as all the tools available in UICollectionView. It supports dynamic cells and supplementary view sizes.
MIT License
898 stars 67 forks source link

AssertionFailure #5

Closed mschonvogel closed 3 years ago

mschonvogel commented 3 years ago

image

Hey, first of all, good work with this library! 🙂 I switched from MessageKit.

I use ChatLayout with RxDataSources and somehow every now and then I'm getting an assertion failure when adding a new message.

I have not yet looked at the layout in detail and wondered if you might know what the problem could be?

ekazaev commented 3 years ago

@mschonvogel Hi Thank you very much. Appreciate.

I never checked it with RxDataSources. But it should not be something big. Most likely I am missing something in my math. Can you please provide an example where I can play around with it?

Also, so I can start quicker, can you add print to the ChatLayout.prepare(forCollectionViewUpdates:) and print updateItems for me. print("\(updateItems)")

I need a log what RXDataSource sends there before you hit an assertion. It seems to be pretty straight forward. For some reason there is less items than the index that is in the item array.

Thank you.

ekazaev commented 3 years ago

@mschonvogel At the first look it seems that RXDataSources flattens somehow the update commands.

ekazaev commented 3 years ago

@mschonvogel Can you also try to replace an else block at line 514 in StateController with the next code:

                } else {
                    guard var item = self.item(for: indexPath.itemPath, kind: .cell, at: .beforeUpdate) else {
                        assertionFailure("Internal inconsistency")
                        return
                    }
                    item.resetSize()
                    var newIndexPath: IndexPath!
                    for (sectionIndex, section) in afterUpdateModel.sections.enumerated() {
                        if let itemIndex = section.items.firstIndex(where: { $0.id == item.id }) {
                            newIndexPath = ItemPath(item: itemIndex, section: sectionIndex).indexPath
                        }
                    }
                    afterUpdateModel.replaceItem(item, at: newIndexPath)
                    reloadedIndexes.insert(newIndexPath)
                }

If ypu wont get a crash It will give me some ideas about what is going on. But of corse an example with the crash is ideal.

mschonvogel commented 3 years ago

@ekazaev thank you for the quick reply

I've added RxDataSources to the example but I'm not able to replicate the bug in there: https://github.com/mschonvogel/ChatLayout/tree/rxdatasources

print("(updateItems)"):

[D(0,0), D(0,1), R(0,30)]
Fatal error: Internal inconsistency: file ChatLayout/SectionModel.swift, line 155
ekazaev commented 3 years ago

Thank you @mschonvogel Ill have a look soon. I think I know what is going on. So expect that it will be fixed soon.

mschonvogel commented 3 years ago

Thank you @ekazaev. Btw replacing the block at line 514 helped, it's not hitting the assertionFailure anymore.

However, prepare(forCollectionViewUpdates:) seems to get called twice now which leads to an ugly insert animation: print("(updateItems)"):

[D(0,0), D(0,1), R(0,25)]
[I(0,0), I(0,25)]
ekazaev commented 3 years ago

@mschonvogel That is absolutely normal that the prepare(forCollectionViewUpdates:) is being called twice or more times. It meana that the performBatchUpdates is also being called twice. But why it happens - its already up to you and how you detect the changes in your model. I would recommend you to check your model. and the comparison there. The chain of the commands has no sense. It deletes the cell and [0, 0] and then inserts it [0, 0]. Are you sure it is what suppose to happen? Why its not just calling reload [0, 0]. I cant say exactly how RXDataSources works. But in terms of DifferenceKit the identifier of said cell should stay the same but the content should be flagged as different. To control that is on your side. But Ill include the fix for your reload situation in the upcoming update after the proper testing. Ugly animation usually mean the model problem.

I can offer you to have a call so we can have a look at your particular situation together.

mschonvogel commented 3 years ago

@ekazaev Okay, I found the problem. 🤦‍♂️ I am using a Firebase Firestore query listener with a limit of 25. Whenever I insert a new message, an older one gets deleted. that's why the deletes were at the beginning of the collection.

Now everything looks fine when I insert a new message...: [R(0,34)] [I(0,35)]

I'll test this some more, but everything should work as expected now. I'm happy to jump on a call, to test the changes you made to the lib if it helps

ekazaev commented 3 years ago

@mschonvogel Good to know. Thank you. I have just released the 1.1.1 version. It contains performance improvements and also a fix for your issue. I neede to change the processing order. I covered your case with a unit test and it seems to be fine. Please check it out.

Please do not hesitate to contact me on twitter @ekazaev

mschonvogel commented 3 years ago

Thank you, it works as expected now!

ekazaev commented 3 years ago

@mschonvogel Fantastic! Happy to hear!