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

Unable to join room by name #4345

Merged
merged 5 commits into from
Oct 26, 2021
Merged

Conversation

ouchadam
Copy link
Contributor

Fixes #4255

When joining a room by name a Can't join error is displayed, this was caused by a missing MatrixToBottomSheet.InteractionListener

However the current flow was finishing the activity which meant the sheet was never displayed.

The fix was to only trigger the navigation interceptor for the room activity navigation instead of including the bottom sheets. The bottom sheet instances live within the activity.

  • Also fixes a hilt related crash due to a missing EntryPoint
BEFORE AFTER AFTER ALREADY JOINED
before-room-link after-join-link after-existing-room

…t when linking from the public rooms

- the activity is still finished causing the popup to not actually display
…vigation, not the bottomsheets

- the bottomsheets require the activity to stay around as they host the sheet instance, fixes missing join sheets
@github-actions
Copy link

github-actions bot commented Oct 26, 2021

Unit Test Results

  48 files  ±0    48 suites  ±0   54s ⏱️ -1s
  91 tests ±0    91 ✔️ ±0  0 💤 ±0  0 ±0 
244 runs  ±0  244 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 272baa5. ± Comparison against base commit 01a29f6.

♻️ This comment has been updated with latest results.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM one remark about missing Hilt annotation

@@ -51,6 +52,7 @@ data class RoomPreviewData(
get() = MatrixItem.RoomItem(roomId, roomName ?: roomAlias, avatarUrl)
}

@AndroidEntryPoint
Copy link
Member

Choose a reason for hiding this comment

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

This is out of scope of this PR, but I made a quick check and this annotation is also missing for:

  • DebugPermissionActivity
  • PinActivity
  • SignedOutActivity
  • SpacePeopleActivity
  • SpacePreviewActivity

I think we can take the opportunity to add it also there. Thanks! CC @ganfra

Copy link
Contributor Author

@ouchadam ouchadam Oct 26, 2021

Choose a reason for hiding this comment

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

thanks for the list! was going to raise in the daily sync

will update the PR 👍

272baa5

Copy link
Member

Choose a reason for hiding this comment

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

Those Activities should be opened by the allScreensTest(), so maybe we should create a dedicated PR with those change, IDK. But it definitely need to be fixed before Hilt is released to the store, so better to do it ASAP

Copy link
Contributor Author

@ouchadam ouchadam Oct 26, 2021

Choose a reason for hiding this comment

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

agreed we should fix these asap, ideally a separate PR would have been better, at least the change within this PR is quite small

I also double checked the remaining activities and couldn't find any others that were missing the entry point 🤞

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks Adam

@bmarty bmarty enabled auto-merge October 26, 2021 17:30
@bmarty bmarty merged commit a19999a into develop Oct 26, 2021
@bmarty bmarty deleted the feature/adm/unable-to-join-by-name branch October 26, 2021 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joining by typing room address doesn't work
2 participants