Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort muted rooms to the bottom of their section of the room list #10592

Merged
merged 39 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b937438
muted-to-the-bottom POC
Apr 13, 2023
5292e47
split muted rooms in natural algorithm
Apr 18, 2023
07606b1
add previous event to account data dispatch
Apr 18, 2023
668cf8e
add muted to notification state
Apr 18, 2023
6450a73
sort muted rooms to the bottom
Apr 18, 2023
94ed2a6
only split muted rooms when sorting is RECENT
Apr 19, 2023
4352b46
Merge branch 'develop' into kerry/sort-muted
Apr 19, 2023
1b095d3
remove debugs
Apr 19, 2023
3ed9133
Merge branch 'develop' into kerry/sort-muted
Apr 19, 2023
acb7606
use RoomNotifState better
Apr 19, 2023
17c72fb
add default notifications test util
Apr 17, 2023
e638e15
test getChangedOverrideRoomPushRules
Apr 20, 2023
cd00719
remove file
Apr 20, 2023
625e286
test roomudpate in roomliststore
Apr 20, 2023
b8cb050
unit test ImportanceAlgorithm
Apr 21, 2023
86194ab
Merge branch 'develop' into kerry/test-importance-algorithm
Apr 21, 2023
e44f8be
strict fixes
Apr 21, 2023
b92f410
Merge branch 'develop' into kerry/sort-muted
Apr 21, 2023
a4a05d2
Merge branch 'kerry/test-importance-algorithm' into kerry/sort-muted
Apr 21, 2023
4028131
test recent x importance with muted rooms
Apr 21, 2023
f5faa5f
unit test NaturalAlgorithm
Apr 21, 2023
097b81b
Merge branch 'kerry/test-importance-algorithm' into kerry/sort-muted
Apr 21, 2023
1c81d6f
Merge branch 'develop' into kerry/sort-muted
Apr 24, 2023
652a41f
test naturalalgorithm with muted rooms
Apr 24, 2023
84ea40f
strict fixes
Apr 24, 2023
b2e9281
Merge branch 'develop' into kerry/sort-muted
Apr 25, 2023
bbb19f5
comments
Apr 25, 2023
6a15a48
add push rules test utility
Apr 25, 2023
33a32f6
Merge branch 'kerry/push-rules-test-util' into kerry/sort-muted
Apr 25, 2023
bfb9ed6
strict fixes
Apr 26, 2023
6eb155b
more strict
Apr 26, 2023
c418923
Merge branch 'develop' into kerry/sort-muted
Apr 27, 2023
0b9997c
tidy comment
Apr 27, 2023
237b4ca
document previousevent on account data dispatch event
Apr 27, 2023
43f3316
simplify (?) room mute rule utilities, comments
Apr 27, 2023
ca05e61
Merge branch 'develop' into kerry/sort-muted
Apr 28, 2023
8654bea
Merge branch 'develop' into kerry/sort-muted
May 1, 2023
30be86f
remove debug
May 5, 2023
947f994
Merge branch 'develop' into kerry/sort-muted
May 5, 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
35 changes: 30 additions & 5 deletions src/RoomNotifs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,19 +182,44 @@ function findOverrideMuteRule(roomId: string): IPushRule | null {
return null;
}
for (const rule of cli.pushRules.global.override) {
if (rule.enabled && isRuleForRoom(roomId, rule) && isMuteRule(rule)) {
if (rule.enabled && isRuleRoomMuteRuleForRoomId(roomId, rule)) {
return rule;
}
}
return null;
}

function isRuleForRoom(roomId: string, rule: IPushRule): boolean {
if (rule.conditions?.length !== 1) {
/**
* Checks if a given rule is a room mute rule as implemented by EW
* - matches every event in one room (one condition that is an event match on roomId)
* - silences notifications (one action that is `DontNotify`)
* @param rule - push rule
* @returns {boolean} - true when rule mutes a room
*/
export function isRuleMaybeRoomMuteRule(rule: IPushRule): boolean {
return (
// matches every event in one room
rule.conditions?.length === 1 &&
rule.conditions[0].kind === ConditionKind.EventMatch &&
rule.conditions[0].key === "room_id" &&
// silences notifications
isMuteRule(rule)
);
}

/**
* Checks if a given rule is a room mute rule as implemented by EW
* @param roomId - id of room to match
* @param rule - push rule
* @returns {boolean} true when rule mutes the given room
*/
function isRuleRoomMuteRuleForRoomId(roomId: string, rule: IPushRule): boolean {
if (!isRuleMaybeRoomMuteRule(rule)) {
return false;
}
const cond = rule.conditions[0];
return cond.kind === ConditionKind.EventMatch && cond.key === "room_id" && cond.pattern === roomId;
// isRuleMaybeRoomMuteRule checks this condition exists
const cond = rule.conditions![0]!;
return cond.pattern === roomId;
}

function isMuteRule(rule: IPushRule): boolean {
Expand Down
9 changes: 8 additions & 1 deletion src/actions/MatrixActionCreators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function createSyncAction(matrixClient: MatrixClient, state: string, prevState:
* @property {MatrixEvent} event the MatrixEvent that triggered the dispatch.
* @property {string} event_type the type of the MatrixEvent, e.g. "m.direct".
* @property {Object} event_content the content of the MatrixEvent.
* @property {MatrixEvent} previousEvent the previous account data event of the same type, if present
*/

/**
Expand All @@ -56,14 +57,20 @@ function createSyncAction(matrixClient: MatrixClient, state: string, prevState:
*
* @param {MatrixClient} matrixClient the matrix client.
* @param {MatrixEvent} accountDataEvent the account data event.
* @param {MatrixEvent | undefined} previousAccountDataEvent the previous account data event of the same type, if present
* @returns {AccountDataAction} an action of type MatrixActions.accountData.
*/
function createAccountDataAction(matrixClient: MatrixClient, accountDataEvent: MatrixEvent): ActionPayload {
function createAccountDataAction(
matrixClient: MatrixClient,
accountDataEvent: MatrixEvent,
previousAccountDataEvent?: MatrixEvent,
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
): ActionPayload {
return {
action: "MatrixActions.accountData",
event: accountDataEvent,
event_type: accountDataEvent.getType(),
event_content: accountDataEvent.getContent(),
previousEvent: previousAccountDataEvent,
Copy link
Member

Choose a reason for hiding this comment

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

add this to the definition of AccountDataAction please.

Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit asymmetric that we add the type and content of accountDataEvent to the action separately, but use a single field for previousAccountDataEvent. Did you consider splitting previousAccountDataEvent for symmetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really like the destructuring in the first place as the event is also included in full.
Most other matrix actions that have events in their payload don't do any destructuring.
previousEvent will also always have the same type as event

Copy link
Member

Choose a reason for hiding this comment

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

oh yes, I'd failed to spot the presence of event. What a strange structure. 👍 to your solution here.

};
}

Expand Down
1 change: 1 addition & 0 deletions src/stores/notifications/NotificationColor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
import { _t } from "../../languageHandler";

export enum NotificationColor {
Muted,
// Inverted (None -> Red) because we do integer comparisons on this
None, // nothing special
// TODO: Remove bold with notifications: https://github.com/vector-im/element-web/issues/14227
Expand Down
12 changes: 10 additions & 2 deletions src/stores/notifications/NotificationState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export interface INotificationStateSnapshotParams {
symbol: string | null;
count: number;
color: NotificationColor;
muted: boolean;
}

export enum NotificationStateEvents {
Expand All @@ -42,6 +43,7 @@ export abstract class NotificationState
protected _symbol: string | null = null;
protected _count = 0;
protected _color: NotificationColor = NotificationColor.None;
protected _muted = false;

private watcherReferences: string[] = [];

Expand All @@ -66,6 +68,10 @@ export abstract class NotificationState
return this._color;
}

public get muted(): boolean {
return this._muted;
}

public get isIdle(): boolean {
return this.color <= NotificationColor.None;
}
Expand Down Expand Up @@ -110,16 +116,18 @@ export class NotificationStateSnapshot {
private readonly symbol: string | null;
private readonly count: number;
private readonly color: NotificationColor;
private readonly muted: boolean;

public constructor(state: INotificationStateSnapshotParams) {
this.symbol = state.symbol;
this.count = state.count;
this.color = state.color;
this.muted = state.muted;
}

public isDifferentFrom(other: INotificationStateSnapshotParams): boolean {
const before = { count: this.count, symbol: this.symbol, color: this.color };
const after = { count: other.count, symbol: other.symbol, color: other.color };
const before = { count: this.count, symbol: this.symbol, color: this.color, muted: this.muted };
const after = { count: other.count, symbol: other.symbol, color: other.color, muted: other.muted };
return JSON.stringify(before) !== JSON.stringify(after);
}
}
3 changes: 3 additions & 0 deletions src/stores/notifications/RoomNotificationState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,12 @@ export class RoomNotificationState extends NotificationState implements IDestroy
const snapshot = this.snapshot();

const { color, symbol, count } = RoomNotifs.determineUnreadState(this.room);
const muted =
RoomNotifs.getRoomNotifsState(this.room.client, this.room.roomId) === RoomNotifs.RoomNotifState.Mute;
this._color = color;
this._symbol = symbol;
this._count = count;
this._muted = muted;

// finally, publish an update if needed
this.emitIfUpdated(snapshot);
Expand Down
12 changes: 12 additions & 0 deletions src/stores/room-list/RoomListStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { RoomListStore as Interface, RoomListStoreEvent } from "./Interface";
import { SlidingRoomListStoreClass } from "./SlidingRoomListStore";
import { UPDATE_EVENT } from "../AsyncStore";
import { SdkContextClass } from "../../contexts/SDKContext";
import { getChangedOverrideRoomMutePushRules } from "./utils/roomMute";

interface IState {
// state is tracked in underlying classes
Expand Down Expand Up @@ -289,6 +290,17 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
this.onDispatchMyMembership(<any>payload);
return;
}

const possibleMuteChangeRoomIds = getChangedOverrideRoomMutePushRules(payload);
if (possibleMuteChangeRoomIds) {
for (const roomId of possibleMuteChangeRoomIds) {
const room = roomId && this.matrixClient.getRoom(roomId);
if (room) {
await this.handleRoomUpdate(room, RoomUpdateCause.PossibleMuteChange);
}
}
this.updateFn.trigger();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const CATEGORY_ORDER = [
NotificationColor.Grey,
NotificationColor.Bold,
NotificationColor.None, // idle
NotificationColor.Muted,
];

/**
Expand Down Expand Up @@ -81,6 +82,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
[NotificationColor.Grey]: [],
[NotificationColor.Bold]: [],
[NotificationColor.None]: [],
[NotificationColor.Muted]: [],
};
for (const room of rooms) {
const category = this.getRoomCategory(room);
Expand All @@ -94,7 +96,8 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
// It's fine for us to call this a lot because it's cached, and we shouldn't be
// wasting anything by doing so as the store holds single references
const state = RoomNotificationStateStore.instance.getRoomState(room);
return state.color;
console.log(room.roomId, state.muted, state.color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remaining console.log

return this.isMutedToBottom && state.muted ? NotificationColor.Muted : state.color;
}

public setRooms(rooms: Room[]): void {
Expand Down Expand Up @@ -164,15 +167,25 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
return this.handleSplice(room, cause);
}

if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) {
if (
cause !== RoomUpdateCause.Timeline &&
cause !== RoomUpdateCause.ReadReceipt &&
cause !== RoomUpdateCause.PossibleMuteChange
) {
throw new Error(`Unsupported update cause: ${cause}`);
}

const category = this.getRoomCategory(room);
// don't react to mute changes when we are not sorting by mute
if (cause === RoomUpdateCause.PossibleMuteChange && !this.isMutedToBottom) {
return false;
}

if (this.sortingAlgorithm === SortAlgorithm.Manual) {
return false; // Nothing to do here.
}

const category = this.getRoomCategory(room);

const roomIdx = this.getRoomIndex(room);
if (roomIdx === -1) {
throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`);
Expand Down
Loading
Loading