Link new device using QrCode - First version #5909
Conversation
|
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5909 +/- ##
===========================================
+ Coverage 81.40% 81.53% +0.12%
===========================================
Files 2485 2532 +47
Lines 66239 67120 +881
Branches 8414 8517 +103
===========================================
+ Hits 53924 54728 +804
- Misses 9224 9263 +39
- Partials 3091 3129 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add unit test on RustLinkMobileHandler Add unit test on DefaultLinkNewDeviceEntryPoint
bmarty
force-pushed
the
feature/bma/qrCodeLogin
branch
from
73b9916 to
ec10a0b
Compare
bmarty
changed the title
|
I'm just looking at the functionality for now, I think I found a small issue: screen-20251218-130155.mp4If you exit that screen and start the process again everything seems to work. If not, after some time I see a 'something went wrong' screen, and when tapping on 'start over' I'm taken back to the 'what type of device do you want to link?' screen, with an additional error dialog on the screen. The error I mentioned (after an almost 60s timeout): error.mp4 |
jmartinesp
left a comment
jmartinesp
left a comment
There was a problem hiding this comment.
The flow works quite well overall, the UX is nice, good job! I did find some issues with cancelling the flow, which I displayed above. There are also some minor improvements to the code we could make.
| import io.element.android.libraries.core.log.logger.LoggerTag | ||
|
|
||
| object LoggerTags { | ||
| val linkNewDevice = LoggerTag("LinkNewDevice") |
There was a problem hiding this comment.
Is this so the tag is used for all the logs in the link new device login instead of having a per-screen one?
There was a problem hiding this comment.
The tag can be used as a parent tag and so we can get log like this for instance:
elementx: [LinkNewDevice/LinkNewDeviceFlowNode] step: Uninitialized | LinkNewDeviceFlowNode.kt:129
| import io.element.android.libraries.androidutils.system.setFullBrightness | ||
|
|
||
| @Composable | ||
| fun ForceMaxBrightness() { |
| # Stick to 3.3.3 because of https://github.com/zxing/zxing/issues/1170 | ||
| google_zxing = "com.google.zxing:core:3.3.3" |
There was a problem hiding this comment.
I wonder if we should think about replacing zxing_cpp with some google alternative too (there seem to be some 'android integration' ones). Not for this PR, of course.
| var job1: Job? = null | ||
| var job2: Job? = null |
There was a problem hiding this comment.
Maybe make these a bit more descriptive 😅 ?
There was a problem hiding this comment.
Should we increase the screenshot size so it contains all list items?
There was a problem hiding this comment.
We could change the value defined in the declaration of @PreviewWithLargeHeight but maybe do this in a dedicated PR?
There was a problem hiding this comment.
It can be done on a separate PR, yes. Also, since this may continue growing, maybe it would be better to use separa @Preview(height = ...) annotations for day and night so we can customise the value only for this screen.
Translations are not located in the current module. They will be imported in the correct module once the current PR with the Localazy config has been merged.
|
Thanks for the review @jmartinesp. I have fixed the navigation issue you found and another one that I have seen too. |
|
Content
Add, behind a feature flag, a new "Link new device" entry in the main preference screen.
When this user click on this new entry, they have the option to either show a QR code so that another device can login to the same account using "Sign in with Qr Code" button (this flow is working), or scan a QrCode displayed on a client able to render a QrCode to get logged in (this flow is not working yet).
Need matrix-org/matrix-rust-sdk#5957 to be finalized, but will be done in a follow up PR.
Lint errors are due to translation still in the default location. A full Localazy sync will be performed just before merging the PR (so once approved).
Motivation and context
Simplify login and verifyng another device on another client.
Epic: https://github.com/element-hq/element-internal/issues/697
Screenshots / GIFs
On the video below, another Android device is scanning the QR code and get logged in and verified at the end of the flow. Not that the feature to login using a QrCode already exist and this PR does not touch it.
Screen_recording_20251218_122012.mp4
Tests
If log in to a home server which does not support QrCode, an error should be displayed when the user select "Link new device"
Tested devices
Checklist