Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Display rooms & threads as unread (bold) if threads have unread messages. #9763

Merged
merged 14 commits into from
Jan 11, 2023
Merged

Display rooms & threads as unread (bold) if threads have unread messages. #9763

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
511fbd7
Display rooms as unread (bold) if threads have unread messages.
clokep Dec 12, 2022
ad4feef
Display threads in the threads list as unread (bold).
clokep Dec 13, 2022
82fdbac
Add a badge to the threads icon if any threads are unread.
clokep Dec 13, 2022
06c6a5f
Merge remote-tracking branch 'upstream/develop' into unread-threads
clokep Jan 4, 2023
c324fe7
Merge remote-tracking branch 'upstream/develop' into unread-threads
clokep Jan 9, 2023
b8a42ca
Add an extra check for uninitialized timelines.
clokep Jan 9, 2023
2daf604
Attempt to appease linter.
clokep Jan 9, 2023
8e7dc94
More linting fixes.
clokep Jan 10, 2023
22afd72
Rename variable.
clokep Jan 10, 2023
5ca3fe1
Fix tests.
clokep Jan 10, 2023
3ef2830
Merge remote-tracking branch 'upstream/develop' into unread-threads
clokep Jan 10, 2023
771d6da
Update tests
germain-gg Jan 10, 2023
dd1fe22
Merge branch 'develop' into unread-threads
Jan 10, 2023
a92e181
Merge branch 'develop' into unread-threads
Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 22 additions & 23 deletions src/Unread.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
*/

import { Room } from "matrix-js-sdk/src/models/room";
import { Thread } from "matrix-js-sdk/src/models/thread";
import { MatrixEvent } from "matrix-js-sdk/src/models/event";
import { EventType } from "matrix-js-sdk/src/@types/event";
import { M_BEACON } from "matrix-js-sdk/src/@types/beacon";
Expand Down Expand Up @@ -59,36 +60,34 @@ export function doesRoomHaveUnreadMessages(room: Room): boolean {
return false;
}

for (const timeline of [room, ...room.getThreads()]) {
// If the current timeline has unread messages, we're done.
if (doesRoomOrThreadHaveUnreadMessages(timeline)) {
return true;
}
}
// If we got here then no timelines were found with unread messages.
return false;
}

export function doesRoomOrThreadHaveUnreadMessages(room: Room | Thread): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Mostly choosing a random place to start a thread.)

We need to figure out how to handle threads that exist before this change or users will see many rooms marked as unread as threads that exited before MSC3771 will likely not have a read receipt and will appear as unread.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsouquet I think you were going to write a bit about our options here? From what I remember of our conversation we want to mark any "existing" threads as read up to their current point when this rolls out (so we don't mark all "old" threads as unread). Potential options:

  1. Send a read receipt in each thread to the server and receive them down sync. (This likely wouldn't work since we would essentially DoS the server & we don't load all threads on load anyway.)
  2. Mark any threads before a certain time as read. This isn't desirable since different people will start using the new code at different times.

I'm not sure we came up with others (besides something about changing the entire storage of events to be closer to what mobile does).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have discussed this during a web meeting last week.
One alternative approach that has not been listed above is to figure out for each room when the first threaded read receipt has been sent and to discard all the threads that haven't received an update since that point in time from the computation made in this pull request

Sounds like the most sensible thing to do, we might need to force an initial sync, to make sure we have access to that data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the first threaded read receipt has been sent and to discard all the threads that haven't received an update since that point in time from the computation made in this pull request

How do we compare "when" this way? Using the ts of the receipt and the origin_server_ts of the events?

Copy link
Member Author

@clokep clokep Jan 5, 2023 •
edited by germain-gg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This work is now essentially in matrix-org/matrix-js-sdk#3020 matrix-org/matrix-js-sdk#3031, although we might need more tweaks here.

clokep marked this conversation as resolved.
Show resolved Hide resolved
const myUserId = MatrixClientPeg.get().getUserId();

// as we don't send RRs for our own messages, make sure we special case that
// if *we* sent the last message into the room, we consider it not unread!
// Should fix: https://github.com/vector-im/element-web/issues/3263
// https://github.com/vector-im/element-web/issues/2427
// ...and possibly some of the others at
// https://github.com/vector-im/element-web/issues/3363
if (room.timeline.at(-1)?.getSender() === myUserId) {
return false;
}

// get the most recent read receipt sent by our account.
// N.B. this is NOT a read marker (RM, aka "read up to marker"),
// despite the name of the method :((
const readUpToId = room.getEventReadUpTo(myUserId!);

if (!SettingsStore.getValue("feature_threadstable")) {
// as we don't send RRs for our own messages, make sure we special case that
// if *we* sent the last message into the room, we consider it not unread!
// Should fix: https://github.com/vector-im/element-web/issues/3263
// https://github.com/vector-im/element-web/issues/2427
// ...and possibly some of the others at
// https://github.com/vector-im/element-web/issues/3363
if (room.timeline.length && room.timeline[room.timeline.length - 1].getSender() === myUserId) {
return false;
}
}

// if the read receipt relates to an event is that part of a thread
// we consider that there are no unread messages
// This might be a false negative, but probably the best we can do until
// the read receipts have evolved to cater for threads
if (readUpToId) {
const event = room.findEventById(readUpToId);
if (event?.getThread()) {
return false;
}
}

// this just looks at whatever history we have, which if we've only just started
// up probably won't be very much, so if the last couple of events are ones that
// don't count, we don't know if there are any events that do count between where
Expand Down
31 changes: 29 additions & 2 deletions src/components/views/right_panel/RoomHeaderButtons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ limitations under the License.
import React from "react";
import classNames from "classnames";
import { NotificationCountType, Room, RoomEvent } from "matrix-js-sdk/src/models/room";
import { ThreadEvent } from "matrix-js-sdk/src/models/thread";
import { Feature, ServerSupport } from "matrix-js-sdk/src/feature";

import { _t } from "../../../languageHandler";
Expand All @@ -44,6 +45,7 @@ import { NotificationStateEvents } from "../../../stores/notifications/Notificat
import PosthogTrackers from "../../../PosthogTrackers";
import { ButtonEvent } from "../elements/AccessibleButton";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
import { doesRoomOrThreadHaveUnreadMessages } from "../../../Unread";

const ROOM_INFO_PHASES = [
RightPanelPhases.RoomSummary,
Expand Down Expand Up @@ -154,7 +156,17 @@ export default class RoomHeaderButtons extends HeaderButtons<IProps> {
if (!this.supportsThreadNotifications) {
this.threadNotificationState?.on(NotificationStateEvents.Update, this.onNotificationUpdate);
} else {
// Notification badge may change if the notification counts from the
// server change, if a new thread is created or updated, or if a
// receipt is sent in the thread.
this.props.room?.on(RoomEvent.UnreadNotifications, this.onNotificationUpdate);
this.props.room?.on(RoomEvent.Receipt, this.onNotificationUpdate);
this.props.room?.on(RoomEvent.Timeline, this.onNotificationUpdate);
this.props.room?.on(RoomEvent.Redaction, this.onNotificationUpdate);
this.props.room?.on(RoomEvent.LocalEchoUpdated, this.onNotificationUpdate);
this.props.room?.on(RoomEvent.MyMembership, this.onNotificationUpdate);
this.props.room?.on(ThreadEvent.New, this.onNotificationUpdate);
this.props.room?.on(ThreadEvent.Update, this.onNotificationUpdate);
Comment on lines +159 to +169
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are mostly copied from useUnreadNotifications:

matrix-react-sdk/src/hooks/useUnreadNotifications.ts

Lines 40 to 53 in ad4feef

useEventEmitter(
room,
RoomEvent.UnreadNotifications,
(unreadNotifications: NotificationCount, evtThreadId?: string) => {
// Discarding all events not related to the thread if one has been setup
if (threadId && threadId !== evtThreadId) return;
updateNotificationState();
},
);
useEventEmitter(room, RoomEvent.Receipt, () => updateNotificationState());
useEventEmitter(room, RoomEvent.Timeline, () => updateNotificationState());
useEventEmitter(room, RoomEvent.Redaction, () => updateNotificationState());
useEventEmitter(room, RoomEvent.LocalEchoUpdated, () => updateNotificationState());
useEventEmitter(room, RoomEvent.MyMembership, () => updateNotificationState());

}
this.onNotificationUpdate();
RoomNotificationStateStore.instance.on(UPDATE_STATUS_INDICATOR, this.onUpdateStatus);
Expand All @@ -166,6 +178,13 @@ export default class RoomHeaderButtons extends HeaderButtons<IProps> {
this.threadNotificationState?.off(NotificationStateEvents.Update, this.onNotificationUpdate);
} else {
this.props.room?.off(RoomEvent.UnreadNotifications, this.onNotificationUpdate);
this.props.room?.off(RoomEvent.Receipt, this.onNotificationUpdate);
this.props.room?.off(RoomEvent.Timeline, this.onNotificationUpdate);
this.props.room?.off(RoomEvent.Redaction, this.onNotificationUpdate);
this.props.room?.off(RoomEvent.LocalEchoUpdated, this.onNotificationUpdate);
this.props.room?.off(RoomEvent.MyMembership, this.onNotificationUpdate);
this.props.room?.off(ThreadEvent.New, this.onNotificationUpdate);
this.props.room?.off(ThreadEvent.Update, this.onNotificationUpdate);
}
RoomNotificationStateStore.instance.off(UPDATE_STATUS_INDICATOR, this.onUpdateStatus);
}
Expand All @@ -191,9 +210,17 @@ export default class RoomHeaderButtons extends HeaderButtons<IProps> {
return NotificationColor.Red;
case NotificationCountType.Total:
return NotificationColor.Grey;
default:
return NotificationColor.None;
}
// We don't have any notified messages, but we might have unread messages. Let's
// find out.
for (const thread of this.props.room!.getThreads()) {
// If the current thread has unread messages, we're done.
if (doesRoomOrThreadHaveUnreadMessages(thread)) {
return NotificationColor.Bold;
}
}
// Otherwise, no notification color.
return NotificationColor.None;
}

private onUpdateStatus = (notificationState: SummarizedNotificationState): void => {
Expand Down
13 changes: 8 additions & 5 deletions src/hooks/useUnreadNotifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ limitations under the License.
*/

import { NotificationCount, NotificationCountType, Room, RoomEvent } from "matrix-js-sdk/src/models/room";
import { Thread } from "matrix-js-sdk/src/models/thread";
import { useCallback, useEffect, useState } from "react";

import { getUnsentMessages } from "../components/structures/RoomStatusBar";
import { getRoomNotifsState, getUnreadNotificationCount, RoomNotifState } from "../RoomNotifs";
import { NotificationColor } from "../stores/notifications/NotificationColor";
import { doesRoomHaveUnreadMessages } from "../Unread";
import { doesRoomOrThreadHaveUnreadMessages } from "../Unread";
import { EffectiveMembership, getEffectiveMembership } from "../utils/membership";
import { useEventEmitter } from "./useEventEmitter";

Expand Down Expand Up @@ -75,12 +76,14 @@ export const useUnreadNotifications = (
setColor(NotificationColor.Red);
} else if (greyNotifs > 0) {
setColor(NotificationColor.Grey);
} else if (!threadId) {
// TODO: No support for `Bold` on threads at the moment

} else {
// We don't have any notified messages, but we might have unread messages. Let's
// find out.
const hasUnread = doesRoomHaveUnreadMessages(room);
let roomOrThread: Room | Thread = room;
if (threadId) {
roomOrThread = room.getThread(threadId)!;
}
const hasUnread = doesRoomOrThreadHaveUnreadMessages(roomOrThread);
setColor(hasUnread ? NotificationColor.Bold : NotificationColor.None);
}
}
Expand Down
Loading