[Device management] Rename a session (PSG-747) #7189

mnaturel · 2022-09-21T09:12:06Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adding a new screen to rename a given session. It can be reached from the session overview screen using the overflow menu.

Motivation and context

Closes #7158

Screenshots / GIFs

Tests

Enable the new device manager feature flag
Go to Settings -> Security & Privacy -> Show all sessions (V2, WIP)
Press a session entry to go to a session overview
Press the overflow menu
Press the rename action
Check the focus is on the text field and the keaboard is opened when landing on the screen
Edit the name
Press the "Save" button
Check the session name is correctly updated everywhere
Press a session entry to go to a session overview
Press the overflow menu
Press the rename action
Edit the name
Press the close button
Check the session name has not been changed

Tested devices

Physical
Emulator
OS version(s): Android 11

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

jmartinesp

Minor comment, other than that it LGTM.

jmartinesp · 2022-09-22T07:59:42Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/rename/RenameSessionFragment.kt

+        if (renameEditTextInitialized.not()) {
+            views.renameSessionEditText.setText(state.editedDeviceName)
+            views.renameSessionEditText.setSelection(views.renameSessionEditText.length())
+            renameEditTextInitialized = true
+        }
+        views.renameSessionSave.isEnabled = state.editedDeviceName.isNotEmpty()


This looks a bit weird. If it's a one time event, shouldn't it be done through a ViewEvent? Or if it needs to be part of the state, could we check the contents of renameSessionEditText before applying these changes instead?

For me it should be in the ViewState. The reason is that we should be able to reconstruct the view at any moment using the ViewState. So for the enable status of the save button, we should use info from the ViewState. Like you said ViewEvent is a one time event and so not persisted, meaning we will lose the enable status of the button.
But I don't understand the last part of your comment:

could we check the contents of renameSessionEditText before applying these changes instead?

Can you add more details about your suggestion?

I was suggesting to either add a RenameSessionViewEvent.Initialized(val deviceName: String) that can be consumed once, or maybe checking if (views.renameSessionEditText.text.isNullOrEmpty()) before applying the state here instead of adding the renameEditTextInitialized variable for that, since it seems like you only want to set the view contents when this data is first loaded, I guess because otherwise keyboard input and state updates would clash on the EditText.

Oh sorry I got it! I will try to improve. I completely misunderstood your suggestion at first...

I have done an update: see a8d7723. It adds more complexity to the ViewModel but I think it is more logical this way. Let me know what you think about it.

jmartinesp

Thanks for the changes. LGTM!

ElementBot · 2022-09-23T15:13:17Z

	Warnings
⚠️	vector/src/main/AndroidManifest.xml#L203 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L203 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L206 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L206 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L260 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L260 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L269 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L269 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L276 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L276 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L282 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L282 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L401 - Exported receiver does not require permission
⚠️	vector/src/main/AndroidManifest.xml#L401 - Exported receiver does not require permission

Generated by 🚫 dangerJS against 7a4bf83

sonarqubecloud · 2022-09-26T08:29:57Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

67.6% Coverage
0.0% Duplication

mnaturel marked this pull request as ready for review September 21, 2022 09:32

mnaturel requested review from a team and jmartinesp and removed request for a team September 21, 2022 09:32

jmartinesp approved these changes Sep 22, 2022

View reviewed changes

Base automatically changed from feature/mna/device-manager-verify-other-session to develop September 22, 2022 09:46

mnaturel requested a review from jmartinesp September 22, 2022 13:09

jmartinesp approved these changes Sep 22, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/device-manager-rename-session branch 2 times, most recently from 55b8a60 to 5399ef6 Compare September 23, 2022 15:07

Maxime NATUREL added 19 commits September 26, 2022 09:46

Adding changelog entry

45b7248

Default structure for the new code

1091ae4

Adding new method into navigator

3de96a3

Adding menu with rename action in session overview screen

82fabf4

Small improvement in code readability

54aec63

Adding edit text into the screen

c3d359e

Small renaming of navigator method

7eab37e

Setup a custom toolbar

33cf3d3

Adding save button and view actions

b134d35

Rename session use case

d7afea7

Improve rename session use case and add tests

4a3966d

Adding unit tests for navigator

c96d671

Adding unit tests for view model

5498a7e

Focus on edit text + open keyboard when landing on screen

bd9b843

Adding warning info view

5de097e

Fix file name for styles of session warning info view

f0fe636

Updating info icon tint color

e437c9e

Add Init action + corresponding initialized event

fd85ad0

Fix lint issue

7a4bf83

mnaturel force-pushed the feature/mna/device-manager-rename-session branch from 5399ef6 to 7a4bf83 Compare September 26, 2022 07:47

mnaturel merged commit 2231498 into develop Sep 26, 2022

mnaturel deleted the feature/mna/device-manager-rename-session branch September 26, 2022 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Device management] Rename a session (PSG-747) #7189

[Device management] Rename a session (PSG-747) #7189

mnaturel commented Sep 21, 2022 •

edited

Loading

jmartinesp left a comment

jmartinesp Sep 22, 2022

mnaturel Sep 22, 2022

jmartinesp Sep 22, 2022

mnaturel Sep 22, 2022

mnaturel Sep 22, 2022 •

edited

Loading

jmartinesp left a comment

ElementBot commented Sep 23, 2022 •

edited

Loading

sonarqubecloud bot commented Sep 26, 2022

[Device management] Rename a session (PSG-747) #7189

[Device management] Rename a session (PSG-747) #7189

Conversation

mnaturel commented Sep 21, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

jmartinesp left a comment

Choose a reason for hiding this comment

jmartinesp Sep 22, 2022

Choose a reason for hiding this comment

mnaturel Sep 22, 2022

Choose a reason for hiding this comment

jmartinesp Sep 22, 2022

Choose a reason for hiding this comment

mnaturel Sep 22, 2022

Choose a reason for hiding this comment

mnaturel Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

jmartinesp left a comment

Choose a reason for hiding this comment

ElementBot commented Sep 23, 2022 • edited Loading

sonarqubecloud bot commented Sep 26, 2022

mnaturel commented Sep 21, 2022 •

edited

Loading

mnaturel Sep 22, 2022 •

edited

Loading

ElementBot commented Sep 23, 2022 •

edited

Loading