added dialog to change app layout settings #6840

fedrunov · 2022-08-16T08:58:54Z

Type of change

Feature
Bugfix
Technical
Other :

Content

adds a bottom sheet dialog with app layout settings

Motivation and context

part of #6502
closes #6506

Screenshots / GIFs

Light	Dark	Black

Tests

open home
open context menu (3 dots in top right corner)
select "layout preferences"
change preferences

Tested devices

Physical
Emulator
OS version(s): 11

ericdecanini

Implementation looks good, but there are some code improvements I would strongly suggest

ericdecanini · 2022-08-16T10:00:27Z

changelog.d/6506.wip

@@ -0,0 +1 @@
+[App Layout] added dialog to configure app layout


Nice idea using [App Layout]. I'll start doing this too

ericdecanini · 2022-08-16T10:00:56Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeLayoutPreferences.kt

+    companion object {
+        const val SETTINGS_PREFERENCES_HOME_RECENTS = "SETTINGS_PREFERENCES_HOME_RECENTS"
+        const val SETTINGS_PREFERENCES_HOME_FILTERS = "SETTINGS_PREFERENCES_HOME_FILTERS"
+        const val SETTINGS_PREFERENCES_USE_AZ_ORDER = "SETTINGS_PREFERENCES_USE_AZ_ORDER"
+    }


Optional: Kotlin code style guidelines suggest that companion objects should be at the bottom of the class rather than the top

ericdecanini · 2022-08-16T10:02:15Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeLayoutPreferences.kt

+    fun areFiltersEnabled(): Boolean {
+        return preferences.getBoolean(SETTINGS_PREFERENCES_HOME_FILTERS, false)
+    }


Optional: Many of these functions are single liners, maybe it's better to use this syntax:

Suggested change

fun areFiltersEnabled(): Boolean {

return preferences.getBoolean(SETTINGS_PREFERENCES_HOME_FILTERS, false)

}

fun areFiltersEnabled() = preferences.getBoolean(SETTINGS_PREFERENCES_HOME_FILTERS, false)

ericdecanini · 2022-08-16T10:11:52Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListViewModel.kt

+        favouritesFlow
+                .combine(dmsFLow) { hasFavourite, hasDm ->
+                    hasFavourite to hasDm
+                }.combine(filtersPreferencesFlow) { (hasFavourite, hasDm), areFiltersEnabled ->
+                    Triple(hasFavourite, hasDm, areFiltersEnabled)


You can use combine without any prefix to combine three flows in one go
https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/combine.html

Suggested change

favouritesFlow

.combine(dmsFLow) { hasFavourite, hasDm ->

hasFavourite to hasDm

}.combine(filtersPreferencesFlow) { (hasFavourite, hasDm), areFiltersEnabled ->

Triple(hasFavourite, hasDm, areFiltersEnabled)

combine(favouritesFlow, dmsFlow, filteresPreferencesFlow) { hasFavourite, hasDm, areFiltersEnabled ->

Triple(hasFavourite, hasDm, areFiltersEnabled)

Also note capitalisation on dmsFLow

onurays · 2022-08-16T10:34:31Z

...a/im/vector/app/features/home/room/list/home/layout/HomeLayoutSettingBottomDialogFragment.kt

+        }
+
+        views.homeLayoutSettingsRecents.setOnCheckedChangeListener { _, isChecked ->
+            preferences.setRecentsEnabled(isChecked)


I think we shouldn't change preferences before the user clicks to "Done" button. What do you think?

@amshakal could you please verify?

Yes, I believe no changes should be made before the user hits 'DONE'

This is strange to me to have a "DONE" button on a BottomSheet. I would prefer to have the change effect live and visible to the background. For instance when disabling "Show recents", the user will have a direct feedback by seeing the breadcrumbs row vanish in the background. They can even play with the toggle several times. Since it's easy to revert the change, I would simply remove the DONE button. Else what would be done if the user dismiss the bottom sheet without clicking on DONE? Cancel the pending change? Not a good UX to me.

On Android settings and in app settings (preference screen), there is no DONE button, any changes are immediately taking effect. Better to do the same here IMHO.

CC @amshakal

Changes are applied when user press DONE, so if user closes dialog in any other way this changes will just be lost. I don't have any strong opinion on this topic. I just felt that seeing changes in ui, while you switch options, feels more interactive, and since here it came with zero costs I decided to implement that way.

TLDR; Both options are good for me and there are no technical issues with both of them, so I let Amsha decide :)

While we don't have done button in settings, we can be a bit more explicit with a bottom sheet. I couldn't find anything for or against it in the material guidelines.

I am open to having interactive options but since there is an overlay at the back anyway, the user won't really be able to see the changes properly.

Let's start with this simple approach and we can iterate on it incase it feels like an odd pattern.

I had a chat with Nikita around this. Benoit, if this feels like a massive blocker/anti pattern. I am more than happy to remove the DONE button.

I guess users are used to close bottom sheets either by swiping to the bottom or by clicking on the back button.

As a general guidelines, I think that Buttons on BottomSheet are not a good UX pattern. I do not see lots of Button in the example of BottomSheet here and also in all the apps I use on my phone.

I would definitely remove the DONE button.

Got it. In that case Nikita, let's go with benoit's suggestion of removing the CTA. Sorry about the back and forth

onurays

Nice work! Can you please check my small remarks?

onurays · 2022-08-16T10:40:41Z

...c/main/java/im/vector/app/features/home/room/list/home/filter/HomeFilteredRoomsController.kt

@@ -27,8 +27,7 @@ import org.matrix.android.sdk.api.session.room.members.ChangeMembershipState
 import org.matrix.android.sdk.api.session.room.model.RoomSummary

 class HomeFilteredRoomsController(
-        private val roomSummaryItemFactory: RoomSummaryItemFactory,
-        private val showFilters: Boolean,
+        private val roomSummaryItemFactory: RoomSummaryItemFactory


Please add comma (,) at the end.

onurays · 2022-08-16T10:40:56Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomSection.kt


 sealed class HomeRoomSection {
    data class RoomSummaryData(
            val list: LiveData<PagedList<RoomSummary>>,
-            val showFilters: Boolean,
-            val filtersData: SharedFlow<List<HomeRoomFilter>>
+            val filtersData: SharedFlow<Optional<List<HomeRoomFilter>>>


Please add comma (,) at the end.

onurays · 2022-08-16T10:48:44Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListViewModel.kt

+    private fun observePreferences() {
+        preferences.registerFiltersListener { areFiltersEnabled ->
+            viewModelScope.launch {
+                filtersPreferencesFlow.emit(areFiltersEnabled)


Is this triggering immediately right after you register the listener? If so, we don't need the below lines.

For some reason it doesn't. If I remove that emit, it will not show filters until you change setting

-> DataStore :)

onurays

Thanks for the update!

bmarty · 2022-08-16T22:20:23Z

vector/src/main/java/im/vector/app/features/home/room/list/home/HomeLayoutPreferences.kt

+import im.vector.app.core.di.DefaultPreferences
+import javax.inject.Inject
+
+class HomeLayoutPreferences @Inject constructor(


I think it would be easier to use a DataStore rather than SharedPreferences. See VectorSessionStore or AnalyticsStore for some example.

bmarty · 2022-08-16T22:27:40Z

...a/im/vector/app/features/home/room/list/home/layout/HomeLayoutSettingBottomDialogFragment.kt

+@AndroidEntryPoint
+class HomeLayoutSettingBottomDialogFragment : VectorBaseBottomSheetDialogFragment<BottomSheetHomeLayoutSettingsBinding>() {
+
+    @Inject lateinit var preferences: HomeLayoutPreferences


This should be moved to a ViewModel, and the UI should be updated in the invalidate method.

Not sure we'll benefit from VM here. This dialog is too small and too specific to make it's part reusable and too simple to require separation of concerns. Having VM here will just increase amount of boilerplate and make it less easy to understand. We also have plenty of other dialogs without VMs and I did think it's because of reasons above. Did we change approach and now require to follow same pattern no matter of what?

I agree it's a bit more boilerplate code, but the idea is to have the logic in the ViewModel also to unit test it. Views (Fragment, etc.) should only be responsible of displaying things.
I guess for this particular part it can stay like that. Maybe better to spend effort to use DataStore.

bmarty

I have made some more remarks, on the UX and on the impl.

ElementBot · 2022-08-18T13:02:45Z

vector/src/main/res/layout/bottom_sheet_home_layout_settings.xml

+    android:id="@+id/rootLayout"
+    android:layout_width="match_parent"
+    android:layout_height="wrap_content"
+    android:background="?colorSurface"


⚠️ Possible overdraw: Root element paints background ?colorSurface with a theme that also paints a background (inferred theme is @style/Theme.Vector.Light)

⚠️ Possible overdraw: Root element paints background ?colorSurface with a theme that also paints a background (inferred theme is @style/Theme.Vector.Light)

sonarqubecloud · 2022-08-18T17:25:09Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

bmarty

Thanks for the update!

added dialog to change app layout settings

82d9a4a

fedrunov requested review from a team, onurays, ericdecanini and amshakal and removed request for a team August 16, 2022 08:58

ericdecanini approved these changes Aug 16, 2022

View reviewed changes

onurays reviewed Aug 16, 2022

View reviewed changes

changes according to code review

70e2157

fedrunov requested review from onurays and ericdecanini August 16, 2022 12:28

amshakal approved these changes Aug 16, 2022

View reviewed changes

onurays approved these changes Aug 16, 2022

View reviewed changes

bmarty reviewed Aug 16, 2022

View reviewed changes

ericdecanini approved these changes Aug 17, 2022

View reviewed changes

shared preferences replaces with datastore

7a31911

fedrunov requested a review from bmarty August 18, 2022 11:54

done button removed from setttings bottom sheet

ff31619

ElementBot reviewed Aug 18, 2022

View reviewed changes

bmarty approved these changes Aug 19, 2022

View reviewed changes

fedrunov merged commit 0629cae into develop Aug 19, 2022

fedrunov deleted the feature/nfe/edit_layout_config branch August 19, 2022 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added dialog to change app layout settings #6840

added dialog to change app layout settings #6840

fedrunov commented Aug 16, 2022

ericdecanini left a comment

ericdecanini Aug 16, 2022

ericdecanini Aug 16, 2022

ericdecanini Aug 16, 2022

ericdecanini Aug 16, 2022

onurays Aug 16, 2022

fedrunov Aug 16, 2022

amshakal Aug 16, 2022

bmarty Aug 16, 2022

fedrunov Aug 17, 2022

amshakal Aug 17, 2022

bmarty Aug 17, 2022

amshakal Aug 17, 2022

onurays left a comment

onurays Aug 16, 2022

onurays Aug 16, 2022

onurays Aug 16, 2022

fedrunov Aug 16, 2022

bmarty Aug 16, 2022

onurays left a comment

bmarty Aug 16, 2022

bmarty Aug 16, 2022

fedrunov Aug 17, 2022

bmarty Aug 17, 2022

bmarty left a comment

ElementBot Aug 18, 2022

sonarqubecloud bot commented Aug 18, 2022

bmarty left a comment

		@@ -0,0 +1 @@
		[App Layout] added dialog to configure app layout

added dialog to change app layout settings #6840

added dialog to change app layout settings #6840

Conversation

fedrunov commented Aug 16, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

ericdecanini 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

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

onurays 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

onurays 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

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 18, 2022

bmarty left a comment

Choose a reason for hiding this comment