-
Notifications
You must be signed in to change notification settings - Fork 160
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
Open user profile and room with event from permalink #2776
Conversation
f5374d5
to
d460711
Compare
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
b2112b3
to
181742b
Compare
val userName: String?, | ||
val avatarUrl: String?, | ||
val isBlocked: AsyncData<Boolean>, | ||
val startDmActionState: AsyncAction<RoomId>, | ||
val displayConfirmationDialog: ConfirmationDialog?, | ||
val isCurrentUser: Boolean, | ||
val eventSink: (RoomMemberDetailsEvents) -> Unit | ||
val eventSink: (UserProfileEvents) -> Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Room member detail and User profile now have the same actions and rendering, so the state is common, but at some point we may want to add extra data here, like a RoomId?
, and extra action like Jump to Read Receipt
, only if a room is there.
I just tested it and the
However, on Android 12+ after the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments, nothing major. I'll review the code again once the new changes arrive. Thanks!
} | ||
} | ||
|
||
private suspend fun navigateTo(permalinkData: PermalinkData) { | ||
Timber.d("Navigating to $permalinkData") | ||
attachSession(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not new, but this duplicate attachSession
is quite confusing until you look at the underlying returned types. Maybe we should think about renaming them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's not clear, you're right (and it's due to the new intermediate LoggedInAppScopeFlowNode).
I have made it clearer in fb59776 hopefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those commits uploaded? GH doesn't seem to auto-detect them and I can't find them in the commit list view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some more tests for permalinks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point, added in f48cc81.
@@ -63,6 +66,26 @@ class ElementCallActivity : NodeComponentActivity(), CallScreenNavigator { | |||
} | |||
context.startActivity(intent) | |||
} | |||
|
|||
/** | |||
* Eventually start the ElementCallActivity, and return true if it's the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit vague. Maybe 'Start the ElementCallActivity' if the intent contains a valid URL, return
true` if it's the case'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks fa4ad4a.
...kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsNode.kt
Show resolved
Hide resolved
|
Fixed in 181742b, and it's working when triggering with adb, but for some reason, nothing happen when I click on the matrix.to website on open it here: |
Actually hide the attachment of LoggedInAppScopeFlowNode.
Same here. The intent-filter for the |
@bmarty I got it to work with: <intent-filter>
<action android:name="android.intent.action.VIEW" />
<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data android:scheme="element" />
<data android:host="user" />
<data android:host="room" />
</intent-filter> |
OK, everything is fine, I think I have handled all your remark @jmartinesp (and thanks for the fix about matrix.to website link). Let me know if there is anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes! LGTM!
Quality Gate passedIssues Measures |
Type of change
Content
Partially handle #2648 , since it will only work for Android 11 and before. Extra steps are required to make it work on Android 12 and higher. This is handled by https://github.com/element-hq/element.io-website/pull/275.
I have extracted some classes from the roomdetail module in order to be able to render a user profile screen (i.e. without having any room available). So extracting stuff to avoid copy pasting.
To be able to use host like
*.element.io
, I had to remove the declarationcall.element.io
from the Manifest about ElementCallActivity. All intent are now handled by the MainActivity, which will eventually start the ElementCallActivity.Motivation and context
External permalink handling.
Screenshots / GIFs
Tests
matrix.to links
adb shell am start -a android.intent.action.VIEW -d 'https://matrix.to/#/@benoitx:matrix.org'
NOTE Use and Android emulator with Android 11 or lower, else the link will be opened by the browser. When opened in the browser, the user can select Element and then get a link to open the permalink in the Element mobile application ("Continue" button below):
It should work on Android 12 and higher.
Edit Actually on mobile, matrix.to website is using a custom scheme (
element
), so I will add the support for it too:I have added the support here: 181742b, and it's working when triggering with adb, but for some reason, nothing happen when I click on the matrix.to website on
open it here
:element.io links
Test for instance with
adb shell am start -a android.intent.action.VIEW -c android.intent.category.BROWSABLE \ -d "https://app.element.io/#/room/!cuqHozLHNBgupgLMKN:matrix.org/%24LZDOueY3R8OD2ZYf8FLKtu95aF7imLBC3F5TIUj-4cc"
Note: will work on Android 12 and higher only when the element.io website will be deployed.
Tested devices
Checklist