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.

Sign up for GitHub

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

Jump to bottom

Fixes room not being in space after upgrade #6200

Merged
merged 7 commits into from
Jul 20, 2022
Merged

Fixes room not being in space after upgrade #6200

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
af4f8d0
Fixes upgraded room duplication
ericdecanini May 30, 2022
8d93044
Fixes room filter not including UPGRADE_ROOM_NOT_JOINED
ericdecanini May 31, 2022
01b7395
Adds changelog file
ericdecanini May 31, 2022
f6b0e8d
Adds isUpgraded method to VersioningState
ericdecanini Jun 3, 2022
36fc2fb
Merge remote-tracking branch 'origin/bugfix/eric/fix-upgrade-room' in…
ericdecanini Jun 7, 2022
e87d970
Merge remote-tracking branch 'origin/develop' into bugfix/eric/upgrad…
ericdecanini Jun 29, 2022
654eede
Adds VersioningStateTest
ericdecanini Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/6200.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixes room not being in space after upgrade

2 changes: 0 additions & 2 deletions ...src/main/java/im/vector/app/features/home/room/detail/upgrade/UpgradeRoomViewModelTask.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ class UpgradeRoomViewModelTask @Inject constructor(
// autoJoin = currentInfo.autoJoin ?: false,
suggested = currentInfo.suggested
)

parentSpace.removeChildren(params.roomId)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we know why this was originally added?

I would assume the room id is changed after upgrade and we're cleaning the legacy room 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would guess this was the first attempt at removing the duplication, and though it worked for that purpose, you would no longer see the pre-upgraded room in the space but you'd see it in Home

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be valuable for other cases, some counters could use spaces children number or some check (if space is empty or not), so absence of removal could cause some side effects.

@BillCarsonFr you added this line, maybe you could verify if removing it could cause any problems?

Copy link
Member

Choose a reason for hiding this comment

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

Yes in the original code when migrating a room, we were removing the old room from the space and then add the new one. The annoying side effect is that people that haven't yet migrated will then see the room as not being in the space anymore (it will probably jump back to home).
Now we want to keep both old & new as part of the migration process, and to have the UI smarter and detect duplicate rooms and only show the new one.
Like that new comers will only see the new room, and existing users will see the old one in the space with the CTA to join the migrated one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about some places, where we could get rooms without applying this filter, like getting space''s room count or something like that. 🤔

}
}
} catch (failure: Throwable) {
Expand Down
5 changes: 5 additions & 0 deletions vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import org.matrix.android.sdk.api.failure.MatrixError.Companion.M_UNRECOGNIZED
import org.matrix.android.sdk.api.session.room.members.ChangeMembershipState
import org.matrix.android.sdk.api.session.room.model.RoomType
import org.matrix.android.sdk.api.session.room.model.SpaceChildInfo
import org.matrix.android.sdk.api.session.room.model.VersioningState
import org.matrix.android.sdk.api.util.toMatrixItem
import javax.inject.Inject

Expand Down Expand Up @@ -110,6 +111,7 @@ class SpaceDirectoryController @Inject constructor(
?.filter {
it.parentRoomId == (data.hierarchyStack.lastOrNull() ?: data.spaceId)
}
?.filterNot { it.isUpgradedRoom(data) }
Copy link
Contributor

Choose a reason for hiding this comment

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

it will filter out rooms with UPGRADED_ROOM_NOT_JOINED VersioningState, is it intentional? If room was upgraded, but user haven't joined upgraded room yet, what user will see? We change state from NONE to UPGRADED_ROOM_NOT_JOINED on TombStone event and so it could happen while user is on a background and doesn't interact with the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional yeah, this only applies to the old version of upgraded rooms, so them not being active there's no use having them shown in Explore Rooms

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, we should filter out if the room is an old version of a room that is also in the space (we want to avoid apparant duplicates in the list).

Copy link
Contributor

Choose a reason for hiding this comment

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

So, when room is upgraded, everyone in the room gets tombstone event and state is changed to UPGRADED_ROOM_NOT_JOINED until they will join new room. isUpgradedRoom will return true and so such users won't see room in a list. Shouldn't we hide it only after users will join new Room?

Copy link
Member

Choose a reason for hiding this comment

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

We should check what web is doing and do the same. What we don't want is for the user to see a duplicated room (for the user it's pretty much the same, after all room upgrade is just a technical thing).
I guess that in the case you mentionned, I would want as a user to see only one room and probably that I joined?
So that mean we show the old room? or the new one but with an indicator that I joined the old version?
anyhow we should align with web and amend later with better behavior if needed

Copy link
Member

Choose a reason for hiding this comment

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

We already have a de-duplication mechanism for upgraded room in the room list. We need similar thing in the explore screen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see web doing this same as this PR. The pre-upgraded room is kept in the space

image

Copy link
Contributor

Choose a reason for hiding this comment

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

But you don't keep it, you filter out any room with UPGRADED_ROOM_NOT_JOINED, so it's different from web, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean keep in the sense that it's not deleted from the space in the db, it's just filtered out in the UI

?: emptyList()

if (flattenChildInfo.isEmpty()) {
Expand Down Expand Up @@ -209,4 +211,7 @@ class SpaceDirectoryController @Inject constructor(
}
}
}

private fun SpaceChildInfo.isUpgradedRoom(data: SpaceDirectoryState) =
data.knownRoomSummaries.any { it.roomId == childRoomId && it.versioningState != VersioningState.NONE }
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: could be a little easier on the brain to lift the isUpgraded = it.versioningState != VersioningState.NONE to an extension on the VersioningState

has the benefit of keeping the conditions here positive and future proofs this logic from any possible new versioning states which aren't upgrades

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, good shout!

}