Added lab feature to pin/unpin messages #7762

cintek · 2022-12-10T19:02:21Z

Type of change

Feature
Bugfix
Technical
Other :

Content

This PR adds a new feature so users can (un)pin messages and open a timeline where only the pinned messages are visible. The feature can be found and activated in the "Labs" settings.

Details of this feature:

It is possible to (un)pin messages
Users can open a timeline with all pinned messages
Users can jump to a specific messages from the pinned messages timeline to the "normal" timeline
Users have to have the necessary role to (un)pin messages

Little background:
Currently, if pinned messages are missing the client will load chunks of older messages like it does in the "normal" timeline until all pinned messages are available. This is not very efficient but it works for now.

Motivation and context

Solves #5933 and #357

Screenshots / GIFs

Here you can see the context menu which includes a button to pin a message. When a message is pinned already a button to do the opposite will be shown.

When there are pinned messages available a button to open a timeline with these messages appears at the top right corner.

Tests

(Un)Pinned messages and compared result with web client

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

Signed-off-by: Christoph Klassen christoph.klassen@intevation.de

bmarty

Thanks a lot for your Pull Request!

I have made a bunch of first remarks.

bmarty · 2022-12-12T11:10:06Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt

@@ -447,3 +448,11 @@ fun Event.supportsNotification() =

 fun Event.isContentReportable() =
        this.getClearType() in EventType.MESSAGE + EventType.STATE_ROOM_BEACON_INFO.values
+
+fun Event.getIdsOfPinnedEvents(): MutableList<String>? {


This should return a List instead of a MutableList. There are other similar remarks in your PR, I will just comment the others with List.

bmarty · 2022-12-12T11:10:14Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt

+    return getClearContent()?.toModel<PinnedEventsStateContent>()?.eventIds
+}
+
+fun Event.getPreviousIdsOfPinnedEvents(): MutableList<String>? {


bmarty · 2022-12-12T11:10:38Z

...ava/org/matrix/android/sdk/api/session/room/model/pinnedmessages/PinnedEventsStateContent.kt

+ */
+@JsonClass(generateAdapter = true)
+data class PinnedEventsStateContent(
+        @Json(name = "pinned") val eventIds: MutableList<String>


bmarty · 2022-12-12T11:13:52Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/state/StateService.kt

+    /**
+     * Pin a message of the room.
+     */
+    suspend fun pinMessage(eventIds: MutableList<String>)


This is taking a list of eventIds, so the doc is not accurate.

the SDK could expose a higher level API like

suspend fun pinEvent(eventId: String) suspend fun unpinEvent(eventId: String)

and compute the new event data regarding the current one.

it may expose this API

suspend fun setPinnedEvents(eventIds: List<String>)

as a bonus for advanced usages like removing all the pinned events, and pinning multiple events in one request.

For the current usages added by this PR, I would just expose pinEvent and unpinEvent methods here.

bmarty · 2022-12-12T11:15:18Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/state/StateService.kt

+    /**
+     * Get state event containing the IDs of pinned events of the room
+     */
+    fun getPinnedEventsState(): Event?


I would not add this method in this Service, since this is a shortcut for getStateEvent.

bmarty · 2022-12-12T11:41:19Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/format/NoticeEventFormatter.kt

@@ -118,6 +121,19 @@ class NoticeEventFormatter @Inject constructor(
        }
    }

+    private fun formatPinnedEvent(event: Event, disambiguatedDisplayName: String): CharSequence? {
+        val idsOfPinnedEvents: MutableList<String> = event.getIdsOfPinnedEvents() ?: return null
+        val previousIdsOfPinnedEvents: MutableList<String>? = event.getPreviousIdsOfPinnedEvents()


bmarty · 2022-12-12T11:41:50Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/format/NoticeEventFormatter.kt

@@ -178,6 +194,7 @@ class NoticeEventFormatter @Inject constructor(
    }

    fun format(event: Event, senderName: String?, isDm: Boolean): CharSequence? {
+        Timber.v("°°°°°°°°°°°°°°°°°°°format(event: Event, senderName: String?, isDm: Boolean)")


Please remove this tmp log.

bmarty · 2022-12-12T11:41:56Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/format/NoticeEventFormatter.kt

@@ -872,6 +889,7 @@ class NoticeEventFormatter @Inject constructor(
    }

    fun formatRedactedEvent(event: Event): String {
+        Timber.v("°°°°°°°formatRedactedEvent°°°°°°")


Please remove this tmp log.

bmarty · 2022-12-12T11:47:36Z

vector/src/main/java/im/vector/app/features/home/room/pinnedmessages/PinnedMessagesActivity.kt

+                is DisplayFragment.ErrorFragment -> {
+                    finish()
+                }
+            }


This is a bit over engineered. I would simply write

private fun initFragment() { if (isFirstCreation()) { val args = getPinnedMessagesTimelineArgs() if(args == null) { finish() } else { initPinnedMessagesTimelineFragment(args) } } }

And cleanup all unused code.

bmarty · 2022-12-12T11:48:57Z

...java/im/vector/app/features/home/room/pinnedmessages/arguments/PinnedMessagesTimelineArgs.kt

+        val avatarUrl: String?,
+        val roomEncryptionTrustLevel: RoomEncryptionTrustLevel?,
+        val rootPinnedMessageEventId: String?
+) : Parcelable


As per my previous remark, I would only keep the roomId here.

cintek · 2022-12-16T08:59:39Z

@bmarty thanks for your review! I edited the things you marked and also did some additional refactoring (renamed files and variables to have more consistency).
Since I was already working on the edits I also made the icons (to pin/unpin events) in the context menu bigger so their size fits better to the other ones.

cintek · 2022-12-16T09:11:53Z

In case that someone wants to know where the icons which are used in this PR come from : They are from Microsoft's Fluent Icons.

cintek · 2022-12-16T09:17:11Z

@bmarty I removed the property -XX:MaxPermSize=2048m in gradle.properties (diff) because I'm using a newer version of Java JDK which was complaining that this property is deprecated.
If you want I will add it again. I just forgot to add it again before commiting.

catfromplan9 · 2023-04-20T09:14:23Z

Waiting for this to be merged, it's a very useful feature. Right now I have to explore the room data and copy out the pin IDs, then check them one by one. It isnt very convenient at all

sschuberth · 2023-05-23T16:56:11Z

@bmarty I removed the property -XX:MaxPermSize=2048m in gradle.properties

I'd also vote for removing the MaxPermSize property (which has been deprecated since Java 1.8), as the source / target compatibility is at 11 anyway:

https://github.com/vector-im/element-android/blob/591b08f1ff426d5d7a5ee6e60599d809f45bbcc4/dependencies.gradle#L5-L6

artempas · 2023-10-20T13:00:27Z

@bmarty any updates?

grvn-ht · 2024-04-26T05:58:53Z

nice feature here! Does it need more changes before merging it @bmarty ?

cintek added 2 commits December 10, 2022 19:36

Added lab feature to pin/unpin messages

45f6bfc

added changelog

adc9136

cintek marked this pull request as ready for review December 10, 2022 19:15

bmarty added the Z-Community-PR Issue is solved by a community member's PR label Dec 12, 2022

bmarty requested changes Dec 12, 2022

View reviewed changes

cintek force-pushed the pin_messages branch from be6df4a to 015bd8b Compare December 16, 2022 08:45

edits after first review on Github

dd65c67

cintek force-pushed the pin_messages branch from 015bd8b to dd65c67 Compare December 16, 2022 08:55

cintek requested a review from bmarty December 16, 2022 08:56

SpiritCroc mentioned this pull request Aug 2, 2023

Pinned messages SchildiChat/SchildiChat-android#202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added lab feature to pin/unpin messages #7762

Added lab feature to pin/unpin messages #7762

cintek commented Dec 10, 2022 •

edited

bmarty left a comment

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

bmarty Dec 12, 2022

cintek commented Dec 16, 2022

cintek commented Dec 16, 2022

cintek commented Dec 16, 2022

catfromplan9 commented Apr 20, 2023

sschuberth commented May 23, 2023

artempas commented Oct 20, 2023

grvn-ht commented Apr 26, 2024

Added lab feature to pin/unpin messages #7762

Are you sure you want to change the base?

Added lab feature to pin/unpin messages #7762

Conversation

cintek commented Dec 10, 2022 • edited

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

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

cintek commented Dec 16, 2022

cintek commented Dec 16, 2022

cintek commented Dec 16, 2022

catfromplan9 commented Apr 20, 2023

sschuberth commented May 23, 2023

artempas commented Oct 20, 2023

grvn-ht commented Apr 26, 2024

cintek commented Dec 10, 2022 •

edited