[Feature] Pinned messages list #3392

ganfra · 2024-09-04T12:11:40Z

Content

Add the Pinned messages list screen and all the ways to navigate (from it and to it).

Motivation and context

Closes #3214

Screenshots / GIFs

See recorded screenshots in the PR.

Tests

Open a room with pinned events
Click on the "View all" pinned banner button
Interacts with the pinned events timeline (long click etc etc).

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
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

…anch in the navigation

…tests

github-actions · 2024-09-04T12:18:01Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/iPS227

bmarty

Nice work, thanks!
Only a few remarks, and I have found a small bug, which is not blocking I think.

bmarty · 2024-09-05T10:27:15Z

...main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListState.kt

+        val actionListState: ActionListState,
+        val eventSink: (PinnedMessagesListEvents) -> Unit,
+    ) : PinnedMessagesListState {
+        val loadedPinnedMessagesCount = timelineItems.count { timelineItem -> timelineItem is TimelineItem.Event }


Can we have other TimelineItem types in timelineItems?

Yes we do have day divider as virtual items

Ah yes, thanks.

bmarty · 2024-09-05T10:29:17Z

...lin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt

+            aLoadingPinnedMessagesListState(),
+            anEmptyPinnedMessagesListState(),
+            aLoadedPinnedMessagesListState(
+                timelineItems = aTimelineItemList(aTimelineItemTextContent())


Maybe use a better list to have a more realistic preview:

is a bit wrong.

And maybe include a Poll to check the rendering of this code.

bmarty · 2024-09-05T10:34:18Z

.../android/features/messages/impl/pinned/list/PinnedMessagesListTimelineActionPostProcessor.kt

+            is TimelineItemAction.ViewSource -> true
+            else -> false
+        }
+    }


Clever way to build the actions, hence in this case, I think "Unpin" could be displayed before "Forward" (as per the design by the way)

I've changed a bit to fix the ordering

bmarty · 2024-09-05T10:40:04Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

+                                if (pushToBackstack) {
+                                    backstack.push(target)
+                                } else {
+                                    backstack.singleTop(target)


This does not have any effect if the target is already in the stack.

To repro the issue:

Navigate to the All pinned event

on pinned event X, select View in timeline

Repeat 1 and 2 (using the same event): nothing happen.

To fix, maybe add a random int field in NavTarget.Room?

Ah yes, good catch

bmarty · 2024-09-05T10:47:27Z

.../android/features/messages/impl/pinned/list/PinnedMessagesListTimelineActionPostProcessor.kt

+import io.element.android.features.messages.impl.actionlist.model.TimelineItemAction
+import io.element.android.features.messages.impl.actionlist.model.TimelineItemActionPostProcessor
+
+object PinnedMessagesListTimelineActionPostProcessor : TimelineItemActionPostProcessor {


Can it be a regular class instead?

bmarty · 2024-09-05T12:43:44Z

.../messages/api/src/main/kotlin/io/element/android/features/messages/api/MessagesEntryPoint.kt

    interface Callback : Plugin {
        fun onRoomDetailsClick()
        fun onUserDataClick(userId: UserId)
-        fun onPermalinkClick(data: PermalinkData)
+        fun onPermalinkClick(data: PermalinkData, pushToBackstack: Boolean = true)


Maybe safer to avoid default values for parameters. I let you decide.

bmarty · 2024-09-05T12:47:19Z

...ries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt

+    enum class Mode {
+        LIVE,
+        FOCUSED_ON_EVENT,
+        FOCUSED_ON_PINNED_EVENTS


FOCUSED_ON_PINNED_EVENTS is a bit confusing to me. In this mode, only the Pinned Events will be in the items, right? So maybe rename to PINNED_EVENTS, or PINNED_EVENTS_ONLY?

Yeah it's because this is a focused timeline underneath, but I agree it's a bit confusing.

sonarcloud · 2024-09-05T16:04:20Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

codecov · 2024-09-05T16:28:19Z

Codecov Report

Attention: Patch coverage is 83.23108% with 82 lines in your changes missing coverage. Please review.

Project coverage is 82.57%. Comparing base (2c8b0d0) to head (332e38b).
Report is 69 commits behind head on develop.

Files with missing lines	Patch %	Lines
...essages/impl/pinned/list/PinnedMessagesListView.kt	80.16%	18 Missing and 6 partials ⚠️
...es/impl/pinned/list/PinnedMessagesListPresenter.kt	85.71%	2 Missing and 11 partials ⚠️
...imeline/components/TimelineItemGroupedEventsRow.kt	29.41%	12 Missing ⚠️
...ssages/impl/pinned/PinnedEventsTimelineProvider.kt	74.19%	6 Missing and 2 partials ⚠️
...t/PinnedMessagesListTimelineActionPostProcessor.kt	14.28%	6 Missing ⚠️
...eatures/messages/impl/DefaultMessagesEntryPoint.kt	0.00%	4 Missing ⚠️
...mpl/pinned/banner/PinnedMessagesBannerPresenter.kt	88.23%	2 Missing and 2 partials ⚠️
...es/messages/impl/actionlist/ActionListPresenter.kt	90.90%	1 Missing and 2 partials ⚠️
.../android/libraries/matrix/api/timeline/Timeline.kt	0.00%	3 Missing ⚠️
...droid/features/roomdetails/impl/RoomDetailsView.kt	81.81%	0 Missing and 2 partials ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3392      +/-   ##
===========================================
- Coverage    82.60%   82.57%   -0.03%     
===========================================
  Files         1694     1703       +9     
  Lines        39651    40027     +376     
  Branches      4821     4866      +45     
===========================================
+ Hits         32752    33051     +299     
- Misses        5216     5270      +54     
- Partials      1683     1706      +23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty

Thanks for the update!

bmarty · 2024-09-06T14:10:18Z

...lin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt

            cancelAndIgnoreRemainingEvents()
-            assert(unpinEventLambda)
+
+            assert(successUnpinEventLambda)


The assertion should be done earlier, else the test does not check the order of the lambda invocation.

bmarty · 2024-09-06T14:11:23Z

...lin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt

+                    ),
+                    aTimelineItemEvent(
+                        isMine = true,
+                        content = aTimelineItemFileContent("A pinned file?"),


ganfra added 17 commits August 28, 2024 12:35

Pinned events : add the PinnedMessagesTimeline feature classes and br…

44ccae3

…anch in the navigation

Timeline : add a mode to differentiate between live/focused/pinned

741c367

Pinned messages : start branching ui to the timeline

46776a9

Pinned events : try sharing pinned events timeline instance

f06ba84

Pinned events : fix compilation

530777a

Pinned events : start displaying actions in timeline

2193b9a

Pinned events : simplify poll content view

bd327aa

Pinned events : better navigation from ViewInTimeline or permalink

90beb0a

Pinned events : add debounce on list

4d0e544

Pinned messages list : navigation from room details

e840581

Pinned messages list : add loading for pinned messages count and add …

8c0a0d8

…tests

Pinned messages list : fix and add tests

fad17f0

Pinned messages list : add some ui tests

cd8a708

Pinned messages list : handle error state (shouldn't really happen)

9ed9559

Pinned messages : add comment on PinnedEventsTimelineProvider

c6e7e7d

Pinned messages list : fix all tests after changes

51f7599

Merge branch 'develop' into feature/fga/pinned_messages_list

12e7e05

ganfra added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Sep 4, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Sep 4, 2024

ganfra added the PR-Wip For anything that isn't ready to ship and will be enabled at a later date label Sep 4, 2024

ElementBot and others added 4 commits September 4, 2024 12:22

Update screenshots

57a8e3b

Pinned messages: clean code

c30398c

Pinned messages : fix test after merging develop

ed94266

Pinned messages list : add more test for better coverage.

e9b37bd

ganfra marked this pull request as ready for review September 4, 2024 15:52

ganfra requested a review from a team as a code owner September 4, 2024 15:52

ganfra requested review from bmarty and removed request for a team September 4, 2024 15:52

bmarty reviewed Sep 5, 2024

View reviewed changes

Pinned messages list : improve and fix code after PR review.

d3420da

ganfra added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Sep 5, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Sep 5, 2024

Update screenshots

332e38b

ganfra requested a review from bmarty September 5, 2024 17:04

ganfra added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Sep 6, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Sep 6, 2024

bmarty approved these changes Sep 6, 2024

View reviewed changes

ganfra merged commit b802a19 into develop Sep 6, 2024
28 of 29 checks passed

ganfra deleted the feature/fga/pinned_messages_list branch September 6, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Pinned messages list #3392

[Feature] Pinned messages list #3392

ganfra commented Sep 4, 2024

github-actions bot commented Sep 4, 2024 •

edited

Loading

bmarty left a comment

bmarty Sep 5, 2024

ganfra Sep 5, 2024

bmarty Sep 5, 2024

bmarty Sep 5, 2024

bmarty Sep 5, 2024

ganfra Sep 5, 2024

bmarty Sep 5, 2024

ganfra Sep 5, 2024

bmarty Sep 5, 2024

bmarty Sep 5, 2024

bmarty Sep 5, 2024

ganfra Sep 5, 2024

sonarcloud bot commented Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

bmarty left a comment

bmarty Sep 6, 2024

bmarty Sep 6, 2024

[Feature] Pinned messages list #3392

[Feature] Pinned messages list #3392

Conversation

ganfra commented Sep 4, 2024

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Sep 4, 2024 • 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

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

sonarcloud bot commented Sep 5, 2024

Quality Gate failed

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

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

github-actions bot commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading