-
-
Notifications
You must be signed in to change notification settings - Fork 831
Sort muted rooms to the bottom of their section of the room list #10592
Changes from 31 commits
b937438
5292e47
07606b1
668cf8e
6450a73
94ed2a6
4352b46
1b095d3
3ed9133
acb7606
17c72fb
e638e15
cd00719
625e286
b8cb050
86194ab
e44f8be
b92f410
a4a05d2
4028131
f5faa5f
097b81b
1c81d6f
652a41f
84ea40f
b2e9281
bbb19f5
6a15a48
33a32f6
bfb9ed6
6eb155b
c418923
0b9997c
237b4ca
43f3316
ca05e61
8654bea
30be86f
947f994
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,12 +58,17 @@ function createSyncAction(matrixClient: MatrixClient, state: string, prevState: | |
* @param {MatrixEvent} accountDataEvent the account data event. | ||
* @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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add this to the definition of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yes, I'd failed to spot the presence of |
||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ limitations under the License. | |
import { _t } from "../../languageHandler"; | ||
|
||
export enum NotificationColor { | ||
Muted, // the room | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this comment - are there some words missing? |
||
// 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ const CATEGORY_ORDER = [ | |
NotificationColor.Grey, | ||
NotificationColor.Bold, | ||
NotificationColor.None, // idle | ||
NotificationColor.Muted, | ||
]; | ||
|
||
/** | ||
|
@@ -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); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remaining |
||
return this.isMutedToBottom && state.muted ? NotificationColor.Muted : state.color; | ||
} | ||
|
||
public setRooms(rooms: Room[]): void { | ||
|
@@ -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}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,42 +21,191 @@ import { SortAlgorithm } from "../models"; | |
import { sortRoomsWithAlgorithm } from "../tag-sorting"; | ||
import { OrderingAlgorithm } from "./OrderingAlgorithm"; | ||
import { RoomUpdateCause, TagID } from "../../models"; | ||
import { RoomNotificationStateStore } from "../../../notifications/RoomNotificationStateStore"; | ||
|
||
type NaturalCategorizedRoomMap = { | ||
defaultRooms: Room[]; | ||
mutedRooms: Room[]; | ||
}; | ||
|
||
/** | ||
* Uses the natural tag sorting algorithm order to determine tag ordering. No | ||
* additional behavioural changes are present. | ||
*/ | ||
export class NaturalAlgorithm extends OrderingAlgorithm { | ||
private cachedCategorizedOrderedRooms: NaturalCategorizedRoomMap = { | ||
defaultRooms: [], | ||
mutedRooms: [], | ||
}; | ||
public constructor(tagId: TagID, initialSortingAlgorithm: SortAlgorithm) { | ||
super(tagId, initialSortingAlgorithm); | ||
} | ||
|
||
public setRooms(rooms: Room[]): void { | ||
this.cachedOrderedRooms = sortRoomsWithAlgorithm(rooms, this.tagId, this.sortingAlgorithm); | ||
const { defaultRooms, mutedRooms } = this.categorizeRooms(rooms); | ||
|
||
this.cachedCategorizedOrderedRooms = { | ||
defaultRooms: sortRoomsWithAlgorithm(defaultRooms, this.tagId, this.sortingAlgorithm), | ||
mutedRooms: sortRoomsWithAlgorithm(mutedRooms, this.tagId, this.sortingAlgorithm), | ||
}; | ||
this.buildCachedOrderedRooms(); | ||
} | ||
|
||
public handleRoomUpdate(room: Room, cause: RoomUpdateCause): boolean { | ||
const isSplice = cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved; | ||
const isInPlace = cause === RoomUpdateCause.Timeline || cause === RoomUpdateCause.ReadReceipt; | ||
const isInPlace = | ||
cause === RoomUpdateCause.Timeline || | ||
cause === RoomUpdateCause.ReadReceipt || | ||
cause === RoomUpdateCause.PossibleMuteChange; | ||
const isMuted = this.isMutedToBottom && this.getRoomIsMuted(room); | ||
|
||
if (!isSplice && !isInPlace) { | ||
throw new Error(`Unsupported update cause: ${cause}`); | ||
} | ||
|
||
if (cause === RoomUpdateCause.NewRoom) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of if conditions, some additional comments are welcomed :) Maybe split the content of the function in multiple private functions to make it easier to read. |
||
this.cachedOrderedRooms.push(room); | ||
if (isMuted) { | ||
this.cachedCategorizedOrderedRooms.mutedRooms = sortRoomsWithAlgorithm( | ||
[...this.cachedCategorizedOrderedRooms.mutedRooms, room], | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
} else { | ||
this.cachedCategorizedOrderedRooms.defaultRooms = sortRoomsWithAlgorithm( | ||
[...this.cachedCategorizedOrderedRooms.defaultRooms, room], | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
} | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} else if (cause === RoomUpdateCause.RoomRemoved) { | ||
const idx = this.getRoomIndex(room); | ||
if (idx >= 0) { | ||
this.cachedOrderedRooms.splice(idx, 1); | ||
return this.removeRoom(room); | ||
} else if (cause === RoomUpdateCause.PossibleMuteChange) { | ||
if (this.isMutedToBottom) { | ||
return this.onPossibleMuteChange(room); | ||
} else { | ||
logger.warn(`Tried to remove unknown room from ${this.tagId}: ${room.roomId}`); | ||
return false; | ||
} | ||
} | ||
|
||
// TODO: Optimize this to avoid useless operations: https://github.com/vector-im/element-web/issues/14457 | ||
// For example, we can skip updates to alphabetic (sometimes) and manually ordered tags | ||
this.cachedOrderedRooms = sortRoomsWithAlgorithm(this.cachedOrderedRooms, this.tagId, this.sortingAlgorithm); | ||
|
||
if (isMuted) { | ||
this.cachedCategorizedOrderedRooms.mutedRooms = sortRoomsWithAlgorithm( | ||
this.cachedCategorizedOrderedRooms.mutedRooms, | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
} else { | ||
this.cachedCategorizedOrderedRooms.defaultRooms = sortRoomsWithAlgorithm( | ||
this.cachedCategorizedOrderedRooms.defaultRooms, | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
} | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} | ||
|
||
/** | ||
* Remove a room from the cached room list | ||
* @param room Room to remove | ||
* @returns {boolean} true when room list should update as result | ||
*/ | ||
private removeRoom(room: Room): boolean { | ||
const defaultIndex = this.cachedCategorizedOrderedRooms.defaultRooms.findIndex((r) => r.roomId === room.roomId); | ||
if (defaultIndex > -1) { | ||
this.cachedCategorizedOrderedRooms.defaultRooms.splice(defaultIndex, 1); | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} | ||
const mutedIndex = this.cachedCategorizedOrderedRooms.mutedRooms.findIndex((r) => r.roomId === room.roomId); | ||
if (mutedIndex > -1) { | ||
this.cachedCategorizedOrderedRooms.mutedRooms.splice(mutedIndex, 1); | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} | ||
|
||
logger.warn(`Tried to remove unknown room from ${this.tagId}: ${room.roomId}`); | ||
// room was not in cached lists, no update | ||
return false; | ||
} | ||
|
||
/** | ||
* Sets cachedOrderedRooms from cachedCategorizedOrderedRooms | ||
*/ | ||
private buildCachedOrderedRooms(): void { | ||
this.cachedOrderedRooms = [ | ||
...this.cachedCategorizedOrderedRooms.defaultRooms, | ||
...this.cachedCategorizedOrderedRooms.mutedRooms, | ||
]; | ||
} | ||
|
||
private getRoomIsMuted(room: Room): boolean { | ||
// 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.muted; | ||
} | ||
|
||
private categorizeRooms(rooms: Room[]): NaturalCategorizedRoomMap { | ||
if (!this.isMutedToBottom) { | ||
return { defaultRooms: rooms, mutedRooms: [] }; | ||
} | ||
return rooms.reduce<NaturalCategorizedRoomMap>( | ||
(acc, room: Room) => { | ||
if (this.getRoomIsMuted(room)) { | ||
acc.mutedRooms.push(room); | ||
} else { | ||
acc.defaultRooms.push(room); | ||
} | ||
return acc; | ||
}, | ||
{ defaultRooms: [], mutedRooms: [] } as NaturalCategorizedRoomMap, | ||
); | ||
} | ||
|
||
private onPossibleMuteChange(room: Room): boolean { | ||
const isMuted = this.getRoomIsMuted(room); | ||
if (isMuted) { | ||
const defaultIndex = this.cachedCategorizedOrderedRooms.defaultRooms.findIndex( | ||
(r) => r.roomId === room.roomId, | ||
); | ||
|
||
// room has been muted | ||
if (defaultIndex > -1) { | ||
// remove from the default list | ||
this.cachedCategorizedOrderedRooms.defaultRooms.splice(defaultIndex, 1); | ||
// add to muted list and reorder | ||
this.cachedCategorizedOrderedRooms.mutedRooms = sortRoomsWithAlgorithm( | ||
[...this.cachedCategorizedOrderedRooms.mutedRooms, room], | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
// rebuild | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} | ||
} else { | ||
const mutedIndex = this.cachedCategorizedOrderedRooms.mutedRooms.findIndex((r) => r.roomId === room.roomId); | ||
|
||
// room has been unmuted | ||
if (mutedIndex > -1) { | ||
// remove from the muted list | ||
this.cachedCategorizedOrderedRooms.mutedRooms.splice(mutedIndex, 1); | ||
// add to default list and reorder | ||
this.cachedCategorizedOrderedRooms.defaultRooms = sortRoomsWithAlgorithm( | ||
[...this.cachedCategorizedOrderedRooms.defaultRooms, room], | ||
this.tagId, | ||
this.sortingAlgorithm, | ||
); | ||
// rebuild | ||
this.buildCachedOrderedRooms(); | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A room-specific rule is a different thing to what this function appears to be looking for (https://spec.matrix.org/v1.6/client-server-api/#push-rules).
I'd suggest finding a different name, but a comment would also be good, defining exactly what it matches.
The fact it only matches rules with a single condition is a bit obscure: why doesn't it include rules which have multiple conditions (of which one is room match)? I can easily see future developers deciding to change that, which seems like it would break
isRuleForRoom
, so it seems particularly important to call out in a comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair call.
I've (hopefully) simplified this a bit, and added some comments