Fix reactions summary crash

[ariskotsomitopoulos]

Fixing NotSerializableException crash on reactions summary

Closes #5463

[bmarty]

LGTM (did not test it though)

[bmarty]

I though the idea was to avoid having lambda in data class. Anyway I understand why it should fix the crash.

On my side, I would have added a new sub interface to TimelineEventController.Callback, but I did not try to do so (this is maybe not better).

[ouchadam]

I would be up for avoiding the bundle of functions by passing an interface instead (not a blocker for this PR from me)

[ariskotsomitopoulos]

I Suspect the problem is due to the @Parcelize and not the data class. If the crash continue then we will change it with a callback.

I also tried with callback at first but then you would have to implement that callback on TimelineFragment and we don't want that there. I think this is a bit more clear with lamda

[ganfra]

I would avoid creating a new data class to be pass everywhere. Would prefer having ReactionsSummaryCallback as interface with empty default implementation. Then let TimelineEventController.Callback inherits this interface and just delegate to reactionListFactory in the case of onShowLess and onShowMore

[ariskotsomitopoulos]

This was my first attempt. Let me check if I can enhance it fast for the release.

[ouchadam]

thanks for the fix! 🤞 could be nice to avoid holding the callbacks in a data class, not a blocker for the release fix

EDIT - just seen @ganfra 's comment, will avoid merging until we're all aligned

[ganfra]

thanks for the fix! 🤞 could be nice to avoid holding the callbacks in a data class, not a blocker for the release fix

EDIT - just seen @ganfra 's comment, will avoid merging until we're all aligned

We can change that later for not blocking the release :)

[github-actions]

Unit Test Results

  96 files  ±0    96 suites  ±0   1m 22s ⏱️ +4s
172 tests ±0  172 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
564 runs  ±0  564 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 84b3f63. ± Comparison against base commit 104f948.

[ariskotsomitopoulos]

thanks for the fix! 🤞 could be nice to avoid holding the callbacks in a data class, not a blocker for the release fix

EDIT - just seen @ganfra 's comment, will avoid merging until we're all aligned

The implementation with callbacks is not working as expected, so I will not submit the PR. Maybe I miss something feel free @ganfra or whoever to try it when you have some time. I would suggest to add the current solution to the release and check if the problem persist

[ouchadam]

thanks for looking into it 👍 let's create a separate ticket to iterate on the area