recents carousel for new home screen layout #6707

fedrunov · 2022-08-01T11:31:36Z

Type of change

Feature
Bugfix
Technical
Other :

Content

The carousel with recent chats, based on Breadcrumbs API

Motivation and context

part of #6502
closes #6504

Screenshots / GIFs

Light	Dark	Black

# Conflicts: # vector/build.gradle # vector/src/main/java/im/vector/app/features/home/HomeDetailFragment.kt # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListFragment.kt # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListViewModel.kt # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomSection.kt

ericdecanini

Implementation looks good. Please just revert the VectorFeature

ericdecanini · 2022-08-02T10:37:55Z

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

@@ -53,5 +53,5 @@ class DefaultVectorFeatures : VectorFeatures {
    override fun isScreenSharingEnabled(): Boolean = true
    override fun forceUsageOfOpusEncoder(): Boolean = false
    override fun shouldStartDmOnFirstMessage(): Boolean = false
-    override fun isNewAppLayoutEnabled(): Boolean = false
+    override fun isNewAppLayoutEnabled(): Boolean = true


Please keep this false by default

mnaturel · 2022-08-03T08:03:22Z

vector/src/main/java/im/vector/app/VectorApplication.kt

@@ -140,6 +144,11 @@ class VectorApplication :
        Mavericks.initialize(debugMode = false)
        EpoxyController.defaultDiffingHandler = EpoxyAsyncUtil.getAsyncBackgroundHandler()
        EpoxyController.defaultModelBuildingHandler = EpoxyAsyncUtil.getAsyncBackgroundHandler()
+        Carousel.setDefaultGlobalSnapHelperFactory(object : Carousel.SnapHelperFactory() {


Maybe we should extract this into a dedicated private method (e.g. initDefaultSnapHelper) to reduce the size of the onCreate method and ease the readability.

I've moved it to configureEpoxy method along with other epoxy related things

mnaturel · 2022-08-03T08:07:55Z

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

@@ -180,6 +183,14 @@ class HomeRoomListFragment @Inject constructor(
                    }
                }.adapter
            }
+            is HomeRoomSection.RecentRoomsData -> RecentRoomCarouselController(


I see RecentRoomCarouselController is injectable. I think we should inject this controller in this fragment instead of AvatarRenderer.

mnaturel · 2022-08-03T08:13:37Z

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

+            is HomeRoomSection.RecentRoomsData -> RecentRoomCarouselController(
+                    avatarRenderer = avatarRenderer
+            ).also { controller ->
+                controller.listener = this


I don't see any clean up of RecyclerViews in onDestroyView() of the fragment. Could we override it and clean the adapters associated to the lists (there is a RecyclerView.cleanUp() extension) + removing the listener from the controllers as we are passing the Fragment instance here.

mnaturel · 2022-08-03T08:25:31Z

.../main/java/im/vector/app/features/home/room/list/home/recent/RecentRoomCarouselController.kt

+                id("recents_carousel")
+                withModelsFrom(data) { originalRoomSummary ->
+
+                    var roomSummary = originalRoomSummary


I think this variable is not necessary. And if it is, it should be a val I guess.

mnaturel · 2022-08-03T08:27:59Z

.../main/java/im/vector/app/features/home/room/list/home/recent/RecentRoomCarouselController.kt

+    }
+}
+
+inline fun <T> CarouselModelBuilder.withModelsFrom(


Should we declare it as private as it is only used in this controller?

mnaturel

Tested. LGTM. I have just added some comments to improve code.

mnaturel

Thanks for the updates! I have tested the app again but when enabling the new app layout, I don't see the carousel of recent rooms like before. Do I need to do something else? Also, since we no more have the left side menu in this new layout, I cannot find the hidden developer menu to access feature flags for example. Where can I find it when I am logged in?

fedrunov · 2022-08-05T13:04:19Z

@mnaturel I blindly assumed that merging few PRs from dev won't break anything, while actually they broke whole thing. I've fixed problem in a separate pr and also update some margins for recents.

mnaturel · 2022-08-05T15:19:26Z

@mnaturel I blindly assumed that merging few PRs from dev won't break anything, while actually they broke whole thing. I've fixed problem in a separate pr and also update some margins for recents.

Thanks for the update. Now it works but I have a UX question: when I enter a room by pressing one of the item of the recent chats list and pressing back to come back home, it changes the ordering of the recent chats. I would expect to only change when posting messages on it. Was it done on purpose? And if the order changes would be possible to reload the position of the list at the start? Or maybe we could simply load the recent chat once when entering the home (not observing them).

fedrunov · 2022-08-08T07:38:20Z

I would expect to only change when posting messages on it.

This is how breadcrumbs work now, I haven't changed behaviour of that. @daniellekirkwood what do you think?

And if the order changes would be possible to reload the position of the list at the start? Or maybe we could simply load the recent chat once when entering the home (not observing them).

I think this is only an issue when you have you list scrolled to the left (first item) and so when something moves to first position you expect list to stay at same position (0) not at same item (position = 1 after change). Do you know a way how to implement this? I know that we can react on inserting/changing item, but we'd also need to check scroll position (or first completely visible item position) before that, to not scroll when it's not necessary

daniellekirkwood · 2022-08-08T08:57:31Z

when I enter a room by pressing one of the item of the recent chats list and pressing back to come back home, it changes the ordering of the recent chats. I would expect to only change when posting messages on it. Was it done on purpose?

Yes! This is how it's supposed to work - its the same on all the platforms. Recents act as Breadcrumbs so users can quickly jump in and out of the chats that they find most interesting.

And if the order changes would be possible to reload the position of the list at the start? Or maybe we could simply load the recent chat once when entering the home (not observing them).

It would make sense for the list to start at 0 again when hitting the home screen but happy to mark that as a "nice-to-have" for launch if we think it will take too much time to implement @fedrunov

mnaturel · 2022-08-08T09:34:19Z

In this case, I guess we could try adding a call to RecyclerView.scrollToPosition(0) when arriving on the home screen?

amshakal · 2022-08-08T12:09:46Z

Looking good. Just a few UI questions:

What font size are we using to display room names in recents? It seems way too big
When filters are deactivated and recents are activated, is it possible to have a line between them? It might imprive the hierarchy. Attaching image for reference.

Otherwise, all is good.

fedrunov · 2022-08-08T13:01:39Z

In this case, I guess we could try adding a call to RecyclerView.scrollToPosition(0) when arriving on the home screen?

this will reset your scroll every time you go back, this could be very annoying. I've filed a separate issue for this, let's unblock this PR and move discussion there: #6776

ElementBot · 2022-08-08T15:36:31Z

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

+    android:id="@+id/recentRoot"
+    android:layout_width="60dp"
+    android:layout_height="wrap_content"
+    android:background="?android:colorBackground"


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

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

ElementBot · 2022-08-08T15:36:31Z

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

+    android:background="?android:colorBackground"
+    android:clickable="true"
+    android:focusable="true"
+    android:foreground="?attr/selectableItemBackground"


⚠️ Attribute android:foreground has no effect on API levels lower than 23 (current min is 21)

⚠️ Attribute android:foreground has no effect on API levels lower than 23 (current min is 21)

sonarcloud · 2022-08-08T16:25:05Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

ericdecanini

LGTM

NIkita Fedrunov added 4 commits July 21, 2022 16:03

new room list implementation for EditLayout

87ff248

code review changes

c7f6163

lint fixes

997b4fc

recents carousel for new home screen layout

b76957c

fedrunov requested review from ericdecanini and a team and removed request for a team August 1, 2022 11:31

fedrunov requested review from a team, mnaturel and ericdecanini and removed request for a team and ericdecanini August 2, 2022 08:50

ericdecanini suggested changes Aug 2, 2022

View reviewed changes

mnaturel reviewed Aug 3, 2022

View reviewed changes

NIkita Fedrunov added 2 commits August 3, 2022 17:41

review fixes

dda62d4

Merge branch 'develop' into feature/nfe/edit_layout_recents_list

e205b96

fedrunov requested review from mnaturel and ericdecanini August 3, 2022 15:49

mnaturel reviewed Aug 4, 2022

View reviewed changes

fixed margins

406d30a

Merge branch 'develop' into feature/nfe/edit_layout_recents_list

8f92f85

fedrunov requested review from amshakal and mnaturel August 5, 2022 13:02

mnaturel approved these changes Aug 8, 2022

View reviewed changes

recent room title font style changed to Body

6f6f615

ElementBot reviewed Aug 8, 2022

View reviewed changes

amshakal approved these changes Aug 8, 2022

View reviewed changes

ericdecanini approved these changes Aug 9, 2022

View reviewed changes

fedrunov merged commit 6045eac into develop Aug 9, 2022

fedrunov deleted the feature/nfe/edit_layout_recents_list branch August 9, 2022 12:31

ouchadam pushed a commit that referenced this pull request Aug 9, 2022

recents carousel for new home screen layout (#6707)

1c8bd36

mnaturel pushed a commit that referenced this pull request Aug 10, 2022

recents carousel for new home screen layout (#6707)

fe78ce5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

recents carousel for new home screen layout #6707

recents carousel for new home screen layout #6707

fedrunov commented Aug 1, 2022 •

edited

Loading

ericdecanini left a comment

ericdecanini Aug 2, 2022

mnaturel Aug 3, 2022

fedrunov Aug 3, 2022

mnaturel Aug 3, 2022

mnaturel Aug 3, 2022

mnaturel Aug 3, 2022

mnaturel Aug 3, 2022

mnaturel left a comment

mnaturel left a comment

fedrunov commented Aug 5, 2022

mnaturel commented Aug 5, 2022 •

edited

Loading

fedrunov commented Aug 8, 2022

daniellekirkwood commented Aug 8, 2022 •

edited

Loading

mnaturel commented Aug 8, 2022

amshakal commented Aug 8, 2022

fedrunov commented Aug 8, 2022

ElementBot Aug 8, 2022

ElementBot Aug 8, 2022

sonarcloud bot commented Aug 8, 2022

ericdecanini left a comment

recents carousel for new home screen layout #6707

recents carousel for new home screen layout #6707

Conversation

fedrunov commented Aug 1, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

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

mnaturel left a comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

fedrunov commented Aug 5, 2022

mnaturel commented Aug 5, 2022 • edited Loading

fedrunov commented Aug 8, 2022

daniellekirkwood commented Aug 8, 2022 • edited Loading

mnaturel commented Aug 8, 2022

amshakal commented Aug 8, 2022

fedrunov commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 8, 2022

ericdecanini left a comment

Choose a reason for hiding this comment

fedrunov commented Aug 1, 2022 •

edited

Loading

mnaturel commented Aug 5, 2022 •

edited

Loading

daniellekirkwood commented Aug 8, 2022 •

edited

Loading