New Layout - Labs Flag (to replace feature flag) #7038

ericdecanini · 2022-09-06T10:52:04Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adds new layout flag as a labs flag

Motivation and context

Screenshots / GIFs

(see bottom option)

Tests

Switch on and off labs flag and see that new layout is switched on and off accordingly

Tested devices

Physical
Emulator
OS version(s): Android 12

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()

bmarty

Just a point on default value usage.

bmarty · 2022-09-06T12:14:05Z

vector/src/main/res/xml/vector_settings_labs.xml

@@ -83,4 +83,10 @@
        android:summary="@string/labs_enable_element_call_permission_shortcuts_summary"
        android:title="@string/labs_enable_element_call_permission_shortcuts" />

+    <im.vector.app.core.preference.VectorSwitchPreference
+        android:defaultValue="false"


You must use settings_labs_new_app_layout_default here too.

…ers_for_space' into feature/eric/new-layout-labs

bmarty · 2022-09-06T12:48:25Z

@ericdecanini I just had a discussion with @fedrunov and I think it's better to keep the feature flag, to be able to completely disable the new app layout feature. It can be useful for forks and also for us if there is a problem with the new app layout.

So if the feature flag is true, the lab flag is displayed. If the feature flag is false, the labs flag is hidden.

Also we will have:

VectorPreferences.isNewAppLayoutEnabled(): Boolean {
    return vectorFeatures.isNewAppLayoutEnabled() &&
        defaultPrefs.getBoolean(SETTINGS_LABS_NEW_APP_LAYOUT_KEY, getDefault(R.bool.settings_labs_new_app_layout_default))
}

Can you do that please? Thanks!

bmarty

See my comment

…ayout-labs # Conflicts: # vector/src/main/java/im/vector/app/features/settings/VectorSettingsLabsFragment.kt

bmarty · 2022-09-06T13:40:11Z

vector/src/main/java/im/vector/app/core/platform/VectorBaseActivity.kt

@@ -249,7 +249,7 @@ abstract class VectorBaseActivity<VB : ViewBinding> : AppCompatActivity(), Maver

        initUiAndData()

-        if (vectorFeatures.isNewAppLayoutEnabled()) {
+        if (vectorPreferences.isNewAppLayoutEnabled()) {


Since both vectorFeatures.isNewAppLayoutEnabled() and vectorPreferences.isNewAppLayoutEnabled() exist now, there is a risk of using the former instead of the latter, especially now that we have many open PR on the subject. Maybe rename the later to something else to force a compilation error on existing PR.

Something like vectorFeatures.isNewAppLayoutFeatureEnabled(). Also maybe document it to tell the developer that they may want to use vectorPreferences.isNewAppLayoutEnabled() instead, to take care of the labs settings.

I admit this is quite ugly...

bmarty · 2022-09-06T13:42:52Z

vector/src/main/java/im/vector/app/features/settings/VectorPreferences.kt

@@ -63,6 +65,7 @@ class VectorPreferences @Inject constructor(
        const val SETTINGS_BACKGROUND_SYNC_PREFERENCE_KEY = "SETTINGS_BACKGROUND_SYNC_PREFERENCE_KEY"
        const val SETTINGS_BACKGROUND_SYNC_DIVIDER_PREFERENCE_KEY = "SETTINGS_BACKGROUND_SYNC_DIVIDER_PREFERENCE_KEY"
        const val SETTINGS_LABS_PREFERENCE_KEY = "SETTINGS_LABS_PREFERENCE_KEY"
+        const val SETTINGS_LABS_NEW_APP_LAYOUT_KEY = "SETTINGS_LABS_ENABLE_NEW_LAYOUT"


generally the val and the string has the same content (sort of convention, which is nice when looking across the whole codebase for the string), so maybe change to

Suggested change

const val SETTINGS_LABS_NEW_APP_LAYOUT_KEY = "SETTINGS_LABS_ENABLE_NEW_LAYOUT"

const val SETTINGS_LABS_NEW_APP_LAYOUT_KEY = "SETTINGS_LABS_NEW_APP_LAYOUT_KEY"

Do not forget to also change on vector_settings_labs.xml

ericdecanini · 2022-09-06T14:08:58Z

vector-app/src/androidTest/java/im/vector/app/ui/robot/ElementRobot.kt

@@ -152,7 +152,7 @@ class ElementRobot {
    }

    fun signout(expectSignOutWarning: Boolean) {
-        if (features.isNewAppLayoutEnabled()) {
+        if (features.isNewAppLayoutFeatureEnabled()) {


Now that we have it as a labs flag, we can take an approach of making the checks on one layout, enabling the labs flag with the robot and making the checks on the new version. I'll do this in #7020 as to not clutter this pr

OK, good idea! Thanks

bmarty · 2022-09-06T14:14:44Z

vector/src/main/java/im/vector/app/features/VectorFeatures.kt

+     * This is only to enable if the labs flag should be visible and effective
+     * If on the client-side you want functionality that should be enabled with the new layout,
+     * use [VectorPreferences.isNewAppLayoutEnabled] instead
+     */


bmarty

I think at some places we want to check the labs flag. Can you double check?

bmarty · 2022-09-06T14:15:29Z

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

@@ -120,7 +120,7 @@ class HomeActivityViewModel @AssistedInject constructor(

    private fun observeReleaseNotes() = withState { state ->
        // we don't want to show release notes for new users or after relogin
-        if (state.authenticationDescription == null && vectorFeatures.isNewAppLayoutEnabled()) {
+        if (state.authenticationDescription == null && vectorFeatures.isNewAppLayoutFeatureEnabled()) {


so we want to check the labs flag here, no?

bmarty · 2022-09-06T14:16:00Z

vector/src/main/java/im/vector/app/features/settings/VectorSettingsLabsFragment.kt

        }

        findPreference<VectorSwitchPreference>(VectorPreferences.SETTINGS_LABS_UNREAD_NOTIFICATIONS_AS_TAB)?.let { pref ->
-            pref.isVisible = !vectorFeatures.isNewAppLayoutEnabled()
+            pref.isVisible = !vectorFeatures.isNewAppLayoutFeatureEnabled()


same, we want to check the labs flag here.

What we should do probably here is make it visible depending on the feature flag, and make it enabled depending on the labs flag

OK, good idea.

bmarty · 2022-09-06T14:16:51Z

vector/src/main/java/im/vector/app/features/settings/VectorSettingsPreferencesFragment.kt

@@ -102,7 +102,7 @@ class VectorSettingsPreferencesFragment :
        }

        findPreference<Preference>(VectorPreferences.SETTINGS_PREF_SPACE_CATEGORY)!!.let { pref ->
-            pref.isVisible = !vectorFeatures.isNewAppLayoutEnabled()
+            pref.isVisible = !vectorFeatures.isNewAppLayoutFeatureEnabled()


same, we want to check the labs flag here.

Same as above

bmarty · 2022-09-06T15:42:08Z

vector/src/main/java/im/vector/app/features/settings/VectorSettingsPreferencesFragment.kt

@@ -103,6 +103,7 @@ class VectorSettingsPreferencesFragment :

        findPreference<Preference>(VectorPreferences.SETTINGS_PREF_SPACE_CATEGORY)!!.let { pref ->
            pref.isVisible = !vectorFeatures.isNewAppLayoutFeatureEnabled()
+            pref.isEnabled = !vectorPreferences.isNewAppLayoutEnabled()


This has to be refreshed as well (like in configureUnreadNotificationsAsTabPreference()). Maybe rename configureUnreadNotificationsAsTabPreference() and use it to refresh all pref enable state?

sorry, please ignore, this is not the same Fragment.

bmarty

One more important remark about the drawer.

bmarty · 2022-09-06T15:57:23Z

vector/src/main/java/im/vector/app/features/home/HomeActivity.kt

@@ -217,7 +217,7 @@ class HomeActivity :
        roomListSharedActionViewModel = viewModelProvider[RoomListSharedActionViewModel::class.java]
        views.drawerLayout.addDrawerListener(drawerListener)
        if (isFirstCreation()) {
-            if (vectorFeatures.isNewAppLayoutEnabled()) {
+            if (vectorPreferences.isNewAppLayoutEnabled()) {


I think we want to unlock the drawer in the else block, since this is now a runtime setting.

Good catch!

bmarty · 2022-09-06T15:58:41Z

vector/src/main/java/im/vector/app/features/settings/VectorSettingsPreferencesFragment.kt

@@ -103,6 +103,7 @@ class VectorSettingsPreferencesFragment :

        findPreference<Preference>(VectorPreferences.SETTINGS_PREF_SPACE_CATEGORY)!!.let { pref ->
            pref.isVisible = !vectorFeatures.isNewAppLayoutFeatureEnabled()
+            pref.isEnabled = !vectorPreferences.isNewAppLayoutEnabled()


sorry, please ignore, this is not the same Fragment.

bmarty

Thanks!

…ayout-labs

bmarty · 2022-09-07T08:58:56Z

@ericdecanini can you check the failing test checkCantVerifyPopup locally please?

…ayout-labs

bmarty · 2022-09-08T10:31:22Z

vector-app/src/androidTest/java/im/vector/app/ui/robot/ElementRobot.kt

-
+class ElementRobot(
+        private val labsPreferences: LabFeaturesPreferences = LabFeaturesPreferences(false)
+) {


Maybe you can invoke the constructor in UiAllScreensSanityTest this way:

private val elementRobot = ElementRobot( LabFeaturesPreferences( InstrumentationRegistry.getInstrumentation() .targetContext .resources .getBoolean(R.bool.settings_labs_new_app_layout_default) ) )

…e status + small fix for tests

sonarqubecloud · 2022-09-08T12:21:50Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

ericdecanini added 2 commits September 6, 2022 12:48

Replaces feature flag with labs flag

2450954

Adds changelog file

fbe5918

ericdecanini marked this pull request as ready for review September 6, 2022 10:55

ericdecanini requested a review from fedrunov September 6, 2022 10:55

fedrunov approved these changes Sep 6, 2022

View reviewed changes

ericdecanini enabled auto-merge September 6, 2022 10:57

Fixes lint error

a851b0a

bmarty requested changes Sep 6, 2022

View reviewed changes

bmarty disabled auto-merge September 6, 2022 12:14

Uses referenced default value for new layout labs flag

5e0d84b

ericdecanini requested a review from bmarty September 6, 2022 12:44

Merge remote-tracking branch 'origin/feature/nfe/app_layout_hide_filt…

91b4918

…ers_for_space' into feature/eric/new-layout-labs

bmarty requested changes Sep 6, 2022

View reviewed changes

ericdecanini added 2 commits September 6, 2022 15:30

Re-adds feature flag

468c7b6

Merge remote-tracking branch 'origin/develop' into feature/eric/new-l…

31a3552

…ayout-labs # Conflicts: # vector/src/main/java/im/vector/app/features/settings/VectorSettingsLabsFragment.kt

ericdecanini requested a review from bmarty September 6, 2022 13:32

bmarty reviewed Sep 6, 2022

View reviewed changes

Changes vector feature preference key and function name for clarity

b7efd63

ericdecanini commented Sep 6, 2022

View reviewed changes

ericdecanini requested a review from bmarty September 6, 2022 14:09

bmarty reviewed Sep 6, 2022

View reviewed changes

bmarty requested changes Sep 6, 2022

View reviewed changes

ericdecanini added 2 commits September 6, 2022 17:31

Fixes wrong use of feature flag instead of labs flag

d77ce27

Changes some preferences to be enabled or disabled by labs flag

9564c8f

ericdecanini requested a review from bmarty September 6, 2022 15:32

bmarty requested changes Sep 6, 2022

View reviewed changes

Unlocks the drawer layout when not new layout

6c23634

bmarty approved these changes Sep 6, 2022

View reviewed changes

Fixes lint error

afbb76f

ericdecanini enabled auto-merge September 7, 2022 06:38

Merge remote-tracking branch 'origin/develop' into feature/eric/new-l…

64c8789

…ayout-labs

Merge remote-tracking branch 'origin/develop' into feature/eric/new-l…

37b2163

…ayout-labs

ericdecanini added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 7, 2022

bmarty disabled auto-merge September 8, 2022 08:57

robots are not checking feature flag for newAppLayout

6badbe7

bmarty reviewed Sep 8, 2022

View reviewed changes

sanity test now reads properties value to determine app layout featur…

2ab0343

…e status + small fix for tests

bmarty approved these changes Sep 8, 2022

View reviewed changes

bmarty merged commit de17c47 into develop Sep 8, 2022

bmarty deleted the feature/eric/new-layout-labs branch September 8, 2022 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Layout - Labs Flag (to replace feature flag) #7038

New Layout - Labs Flag (to replace feature flag) #7038

ericdecanini commented Sep 6, 2022 •

edited

Loading

bmarty left a comment

bmarty Sep 6, 2022

bmarty commented Sep 6, 2022 •

edited

Loading

bmarty left a comment

bmarty Sep 6, 2022

bmarty Sep 6, 2022

ericdecanini Sep 6, 2022 •

edited

Loading

bmarty Sep 6, 2022

bmarty Sep 6, 2022

bmarty left a comment

bmarty Sep 6, 2022

bmarty Sep 6, 2022

ericdecanini Sep 6, 2022

bmarty Sep 6, 2022

bmarty Sep 6, 2022

ericdecanini Sep 6, 2022

bmarty Sep 6, 2022

bmarty Sep 6, 2022

bmarty left a comment

bmarty Sep 6, 2022

ericdecanini Sep 6, 2022

bmarty Sep 6, 2022

bmarty left a comment

bmarty commented Sep 7, 2022

bmarty Sep 8, 2022

sonarqubecloud bot commented Sep 8, 2022

	const val SETTINGS_LABS_NEW_APP_LAYOUT_KEY = "SETTINGS_LABS_ENABLE_NEW_LAYOUT"
	const val SETTINGS_LABS_NEW_APP_LAYOUT_KEY = "SETTINGS_LABS_NEW_APP_LAYOUT_KEY"

New Layout - Labs Flag (to replace feature flag) #7038

New Layout - Labs Flag (to replace feature flag) #7038

Conversation

ericdecanini commented Sep 6, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Sep 6, 2022 • edited Loading

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty commented Sep 7, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 8, 2022

ericdecanini commented Sep 6, 2022 •

edited

Loading

bmarty commented Sep 6, 2022 •

edited

Loading

ericdecanini Sep 6, 2022 •

edited

Loading