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

Space list crash fix #5924

Merged
merged 4 commits into from
May 4, 2022
Merged

Space list crash fix #5924

merged 4 commits into from
May 4, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 4, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Try to fix https://github.com/matrix-org/element-android-rageshakes/issues/37765 :

Thread: main, Exception: java.lang.IllegalStateException: Two different ViewHolders have the same stable ID. Stable IDs in your adapter MUST BE unique and SHOULD NOT change.

According to the log, this is a space invite. We may have a race condition, where an invite is displayed for a space which is already joined? So displayed twice and the Epoxy item has the same ID. This PR adds a prefix for the space invitation items.

Also space invite will be filtered out from the space root list. This is probably the only required fix, but keeping the previous fix on the id does not hurt.

Motivation and context

Screenshots / GIFs

Before After
image image

Tests

  • No test performed.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label May 4, 2022
@bmarty bmarty requested review from a team, onurays and ouchadam and removed request for a team May 4, 2022 07:46
@bmarty
Copy link
Member Author

bmarty commented May 4, 2022

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -117,7 +123,7 @@ class SpaceSummaryController @Inject constructor(
?.forEach { roomSummary ->
spaceSummaryItem {
avatarRenderer(host.avatarRenderer)
id(roomSummary.roomId)
id("invite_${roomSummary.roomId}")
Copy link
Contributor

Choose a reason for hiding this comment

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

a unique id for the invite makes sense to me to fix the crash, we can iterate on the proper UX (whether two spaces with the same id should be present in the spaces list...)

@bmarty bmarty force-pushed the feature/bma/space_fix branch from cf53255 to 70c1310 Compare May 4, 2022 08:07
@@ -135,41 +141,42 @@ class SpaceSummaryController @Inject constructor(
}

rootSpaces
?.forEach { groupSummary ->
val isSelected = selected is RoomGroupingMethod.BySpace && groupSummary.roomId == selected.space()?.roomId
?.filter { it.membership != Membership.INVITE }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@bmarty bmarty enabled auto-merge May 4, 2022 08:26
@github-actions
Copy link

github-actions bot commented May 4, 2022

Unit Test Results

122 files  ±0  122 suites  ±0   2m 8s ⏱️ -1s
205 tests ±0  205 ✔️ ±0  0 💤 ±0  0 ±0 
690 runs  ±0  690 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 70c1310. ± Comparison against base commit 1e3ad30.

@bmarty bmarty merged commit 12d7cee into develop May 4, 2022
@bmarty bmarty deleted the feature/bma/space_fix branch May 4, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants