New feature: marking rooms as unread [MSC2867] #6314

alangecker · 2021-07-04T13:38:12Z

implements matrix-org/matrix-spec-proposals#2867
strongly related, but not exactly the same element-hq/element-meta#347
already released within SchildiChat

Description

This PR...

...adds a new button for marking a room as unread
...shows a visual indicator based on com.famedly.marked_unread
...cleares the com.famedly.marked_unread when the room gets opened

Questions

since I'm still quite unfamiliar with the codebase, I got some questions about this PR:

~~should this react to the stable prefix m.marked_unread already as well? or is the usual approach to add it as soon as the MSC is in a further state?~~
the current code feels like too much is duplicated but can't really say where to rather put the setting & retrieval of the unread state. maybe even put part of in into https://github.com/matrix-org/matrix-js-sdk? would be happy about any hint :)
~~Any suggestions for an fitting icon?~~

This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

SimonBrandner

Thank you for working on this! Generally, looks great! Just some minor details

Could you please include a sign-off as described here?

Also, I think this should probably be hidden under a labs flag since it is using an unstable prefix

should this react to the stable prefix m.marked_unread already as well? or is the usual approach to add it as soon as the MSC is in a further state?

As far as I know, this usually isn't done.

the current code feels like too much is duplicated but can't really say where to rather put the setting & retrieval of the unread state. maybe even put part of in into matrix-org/matrix-js-sdk? would be happy about any hint :)

I probably wouldn't put it into the js-sdk but you could either create a new file for this or put it into an existing one (e.g. Unread.ts)

src/components/structures/TimelinePanel.tsx

src/components/views/rooms/RoomTile.tsx

src/i18n/strings/en_EN.json

src/stores/notifications/RoomNotificationState.ts

alangecker · 2021-07-04T17:51:03Z

thanks a lot for the feedback! :)

I signed off my commits now
the feature is now behind a feature flag: feature_mark_unread
I hope I resolved all your other points as well :)

SimonBrandner

Two more tiny things 😀

src/stores/notifications/RoomNotificationState.ts

alangecker · 2021-07-04T19:07:58Z

@SimonBrandner
as many as you find! :)

regarding an icon: can I just add any license compatible one or would a design team potentially become unhappy? :D

SimonBrandner

LGTM

Regarding the icon: I think you can use any icon that fits the license. Though it's quite probable the design team will suggest their own in the end

turt2live · 2021-07-22T01:51:06Z

@alangecker what's the status of this? It's flagged as WIP and has a bit of a confusing history to it. Is it ready for review by our teams?

alangecker · 2021-07-23T18:15:10Z

@turt2live I still didn't manage to find and integrate an icon, that's why it is still flagged as WIP. From the functionality I wouldn't do any more changes and would be happy about a review :)

also if the design team can imagine deciding for an icon, I would be pleased! :)

turt2live

I'm concerned more generally about the MSC itself as this doesn't give a reliable count for the user to rely upon, so they end up trying to discern the difference between manually marked unread, unread, and mentioned.

I'll copy these comments to the MSC, but overall I don't feel this is a problem that account data should be solving.

src/Rooms.ts

src/stores/notifications/RoomNotificationState.ts

turt2live · 2021-08-05T20:42:34Z

src/stores/notifications/RoomNotificationState.ts

@@ -76,6 +87,12 @@ export class RoomNotificationState extends NotificationState implements IDestroy
        }
    };

+    private handleRoomAccountDataUpdate = (ev: MatrixEvent) => {


this doesn't feel like the right type for account data: there's a ton of missing fields on account data events.

With every occurrence in matrix-react-sdk there is .on("Room.accountData") used along with MatrixEvent as the type for its first argument.

What could be the alternative type? :)

src/stores/notifications/RoomNotificationState.ts

alangecker · 2021-08-22T12:05:15Z

seems like there is support for going a different path for that feature (matrix-org/matrix-spec-proposals#2867 (review)), so I won't continue here for now.

I'm still motivated working on an implementation for this feature, but would need others to lead the spec discussions! :)

SpiritCroc · 2021-11-26T12:15:25Z

@alangecker thank you for this PR! We have included it in SchildiChat-web/desktop: SchildiChat#2
Do you want to be mentioned in the changelog?

alangecker · 2021-11-26T22:50:21Z

@SpiritCroc I was a bit sad to stop the development, wanted to wait for the spec discussions first, but it's very nice that it's being used now nevertheless! :) 🎉

I would be pleased about credits in changelogs, but I'm also totally fine without! ;)

Signed-off-by: chandi <git@chandi.it>

From Google's material design icons: https://fonts.google.com/icons?selected=Material+Icons&icon.query=eye License: https://google.github.io/material-design-icons/ "We have made these icons available for you to incorporate them into your products under the Apache License Version 2.0. Feel free to remix and re-share these icons and documentation in your products. We'd love attribution in your app's about screen, but it's not required. The only thing we ask is that you not re-sell the icons themselves."

TypeError: Cannot read property 'getAccountData' of null at d (Rooms.ts:160) at e.removeUnreadMarker (TimelinePanel.tsx:884) at e.<anonymous> (TimelinePanel.tsx:1140) at ui (react-dom.production.min.js:131) at ps (react-dom.production.min.js:220) at wc (react-dom.production.min.js:259) at t.unstable_runWithPriority (scheduler.production.min.js:18) at jo (react-dom.production.min.js:122) at xc (react-dom.production.min.js:252) at pc (react-dom.production.min.js:243) when you have the panel for mentions open.

ToDo: Do this also if marked as unread on other device

alangecker · 2022-01-10T14:59:51Z

since there is currently no development on the MSC, I thought about pushing this further :)

things done

I've rebased now the branch on top of the current develop and added the additional commits being done by @SpiritCroc and @su-ex from the SchildiChat branch, except keeping this feature still disabled by default.
resolving most of the open conversations

open questions

Despite the fact that the MSC is currently still fundamentally in question, can this still be merged behind a feature flag? @turt2live
The commits by @SpiritCroc and @su-ex are missing a sign-off. Is that an issue and if so how can we resolve that without the complicated path of giving everyone access to this branch and each one has to rebase the commits?^^ couldn't it legally be fine if everyone states here that they sign-off every commit in this PR? :D
Two icons from google material design got added e5810fb. Is that license wise fine? @SimonBrandner

SimonBrandner · 2022-01-10T15:11:11Z

If @SpiritCroc and @su-ex just sign-off in a comment on this PR, stuff should be ok, I believe
IIRC, Material Design icons don't require attribution, so there shouldn't be any problems but I've been wrong before

SpiritCroc · 2022-01-11T11:00:23Z

Signed-off-by: Tobias Büttner dev@spiritcroc.de

What I know about the license of material design icons I've also written in the commit message of e5810fb.

su-ex · 2022-01-11T17:14:57Z

Signed-off-by: Quirin Götz codeworks@supercable.onl

turt2live · 2022-04-08T21:02:36Z

So the story on this is that it's in a bit of a questionable bit spec-wise. I think before we commit to maintenance of this feature we'd want to see a slightly more stable spec discussion first. For the time being, I'm going to close this because it's bitrotting beyond reasonable repair, but it would still count as an implementation against the MSC if needed (though there are plenty of other implementations on the MSC as well). The code would still be here for someone to pick up if needed down the line.

Thanks for taking the time to look at it, and apologies for the long turnaround.

SimonBrandner suggested changes Jul 4, 2021

View reviewed changes

alangecker force-pushed the mark-unread branch 2 times, most recently from d555a9d to 8c5a8fe Compare July 4, 2021 16:55

SimonBrandner reviewed Jul 4, 2021

View reviewed changes

src/stores/notifications/RoomNotificationState.ts Outdated Show resolved Hide resolved

src/stores/notifications/RoomNotificationState.ts Outdated Show resolved Hide resolved

SimonBrandner approved these changes Jul 4, 2021

View reviewed changes

t3chguy requested a review from a team July 7, 2021 18:03

turt2live removed the request for review from a team July 22, 2021 01:50

germain-gg requested a review from a team July 26, 2021 07:35

turt2live changed the title ~~WIP: new feature: marking rooms as unread [MSC2867]~~ New feature: marking rooms as unread [MSC2867] Jul 30, 2021

turt2live reviewed Aug 5, 2021

View reviewed changes

SpiritCroc mentioned this pull request Nov 8, 2021

Mark as unread SchildiChat/schildichat-desktop#109

Closed

16 tasks

ShadowJonathan mentioned this pull request Nov 30, 2021

Add ability to mark messages unread element-hq/element-meta#347

Open

4 tasks

alangecker and others added 11 commits January 10, 2022 14:58

new feature: marking rooms as unread [MSC2867]

2e84df2

Signed-off-by: chandi <git@chandi.it>

single place for unread state retrieval and changing

49451cd

Signed-off-by: chandi <git@chandi.it>

'mark as unread' feature behind labs flag

2ff656f

Signed-off-by: chandi <git@chandi.it>

remove unnecessary checks

2f90208

Signed-off-by: chandi <git@chandi.it>

reduce notification state updates, less generic name for watcher ref

0e5cad7

Signed-off-by: chandi <git@chandi.it>

Render marked-as-unread badge as green exclamation mark

6b28f80

Only show "mark as unread" if applicable (i.e., room not already unread)

66bb12f

Add "mark as read" action

e509a58

Muted rooms may also be rendered as "marked as unread"

0a63fb0

su-ex and others added 6 commits January 10, 2022 15:08

Appease linter

c1fd33a

Remove focus from room tile after hiding menu

1ac2201

Show home if current room is marked unread

26d1219

Use UnstableValue class from matrix-js-sdk

6dcd393

Mark unread: Fix marking current room as unread jump out

25e7554

ToDo: Do this also if marked as unread on other device

feature_mark_unread: add missing labsGroup

1404586

alangecker force-pushed the mark-unread branch 2 times, most recently from 578fec3 to 63f5ccb Compare January 10, 2022 14:40

make linter happy, missing i18n string, clean up

d0a33ad

alangecker force-pushed the mark-unread branch from 63f5ccb to d0a33ad Compare January 10, 2022 14:47

turt2live self-requested a review January 10, 2022 17:43

turt2live closed this Apr 8, 2022

phipz mentioned this pull request Mar 14, 2023

"Mark room as Unread" with a 'dot' on the room list element-hq/element-meta#891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: marking rooms as unread [MSC2867] #6314

New feature: marking rooms as unread [MSC2867] #6314

alangecker commented Jul 4, 2021 •

edited by github-actions bot

Loading

SimonBrandner left a comment

alangecker commented Jul 4, 2021

SimonBrandner left a comment

alangecker commented Jul 4, 2021

SimonBrandner left a comment

turt2live commented Jul 22, 2021

alangecker commented Jul 23, 2021

turt2live left a comment

turt2live Aug 5, 2021

alangecker Jan 10, 2022

alangecker commented Aug 22, 2021

SpiritCroc commented Nov 26, 2021

alangecker commented Nov 26, 2021

alangecker commented Jan 10, 2022 •

edited

Loading

SimonBrandner commented Jan 10, 2022

SpiritCroc commented Jan 11, 2022

su-ex commented Jan 11, 2022

turt2live commented Apr 8, 2022

New feature: marking rooms as unread [MSC2867] #6314

New feature: marking rooms as unread [MSC2867] #6314

Conversation

alangecker commented Jul 4, 2021 • edited by github-actions bot Loading

Description

Questions

SimonBrandner left a comment

Choose a reason for hiding this comment

alangecker commented Jul 4, 2021

SimonBrandner left a comment

Choose a reason for hiding this comment

alangecker commented Jul 4, 2021

SimonBrandner left a comment

Choose a reason for hiding this comment

turt2live commented Jul 22, 2021

alangecker commented Jul 23, 2021

turt2live left a comment

Choose a reason for hiding this comment

turt2live Aug 5, 2021

Choose a reason for hiding this comment

alangecker Jan 10, 2022

Choose a reason for hiding this comment

alangecker commented Aug 22, 2021

SpiritCroc commented Nov 26, 2021

alangecker commented Nov 26, 2021

alangecker commented Jan 10, 2022 • edited Loading

things done

open questions

SimonBrandner commented Jan 10, 2022

SpiritCroc commented Jan 11, 2022

su-ex commented Jan 11, 2022

turt2live commented Apr 8, 2022

alangecker commented Jul 4, 2021 •

edited by github-actions bot

Loading

alangecker commented Jan 10, 2022 •

edited

Loading