From 3062d14a783f31211b0d836eeea0847cce7f69ba Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 16:07:24 -0600 Subject: [PATCH 1/3] Convert ImportanceAlgorithm over to using NotificationColor instead Fixes https://github.com/vector-im/riot-web/issues/14362 implicitly By re-using constructs we already have, we don't need to invent code which figures it out. --- .../list-ordering/ImportanceAlgorithm.ts | 78 ++++++------------- 1 file changed, 23 insertions(+), 55 deletions(-) diff --git a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts index 88789d3a50..b3f1c2b146 100644 --- a/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts +++ b/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts @@ -19,47 +19,29 @@ import { Room } from "matrix-js-sdk/src/models/room"; import { RoomUpdateCause, TagID } from "../../models"; import { SortAlgorithm } from "../models"; import { sortRoomsWithAlgorithm } from "../tag-sorting"; -import * as Unread from '../../../../Unread'; import { OrderingAlgorithm } from "./OrderingAlgorithm"; - -/** - * The determined category of a room. - */ -export enum Category { - /** - * The room has unread mentions within. - */ - Red = "RED", - /** - * The room has unread notifications within. Note that these are not unread - * mentions - they are simply messages which the user has asked to cause a - * badge count update or push notification. - */ - Grey = "GREY", - /** - * The room has unread messages within (grey without the badge). - */ - Bold = "BOLD", - /** - * The room has no relevant unread messages within. - */ - Idle = "IDLE", -} +import { NotificationColor } from "../../../notifications/NotificationColor"; +import { RoomNotificationStateStore } from "../../../notifications/RoomNotificationStateStore"; interface ICategorizedRoomMap { // @ts-ignore - TS wants this to be a string, but we know better - [category: Category]: Room[]; + [category: NotificationColor]: Room[]; } interface ICategoryIndex { // @ts-ignore - TS wants this to be a string, but we know better - [category: Category]: number; // integer + [category: NotificationColor]: number; // integer } // Caution: changing this means you'll need to update a bunch of assumptions and // comments! Check the usage of Category carefully to figure out what needs changing // if you're going to change this array's order. -const CATEGORY_ORDER = [Category.Red, Category.Grey, Category.Bold, Category.Idle]; +const CATEGORY_ORDER = [ + NotificationColor.Red, + NotificationColor.Grey, + NotificationColor.Bold, + NotificationColor.None, // idle +]; /** * An implementation of the "importance" algorithm for room list sorting. Where @@ -92,10 +74,10 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { // noinspection JSMethodCanBeStatic private categorizeRooms(rooms: Room[]): ICategorizedRoomMap { const map: ICategorizedRoomMap = { - [Category.Red]: [], - [Category.Grey]: [], - [Category.Bold]: [], - [Category.Idle]: [], + [NotificationColor.Red]: [], + [NotificationColor.Grey]: [], + [NotificationColor.Bold]: [], + [NotificationColor.None]: [], }; for (const room of rooms) { const category = this.getRoomCategory(room); @@ -105,25 +87,11 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } // noinspection JSMethodCanBeStatic - private getRoomCategory(room: Room): Category { - // Function implementation borrowed from old RoomListStore - - const mentions = room.getUnreadNotificationCount('highlight') > 0; - if (mentions) { - return Category.Red; - } - - let unread = room.getUnreadNotificationCount() > 0; - if (unread) { - return Category.Grey; - } - - unread = Unread.doesRoomHaveUnreadMessages(room); - if (unread) { - return Category.Bold; - } - - return Category.Idle; + private getRoomCategory(room: Room): NotificationColor { + // 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, this.tagId); + return state.color; } public async setRooms(rooms: Room[]): Promise { @@ -217,7 +185,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } } - private async sortCategory(category: Category) { + private async sortCategory(category: NotificationColor) { // This should be relatively quick because the room is usually inserted at the top of the // category, and most popular sorting algorithms will deal with trying to keep the active // room at the top/start of the category. For the few algorithms that will have to move the @@ -234,7 +202,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } // noinspection JSMethodCanBeStatic - private getCategoryFromIndices(index: number, indices: ICategoryIndex): Category { + private getCategoryFromIndices(index: number, indices: ICategoryIndex): NotificationColor { for (let i = 0; i < CATEGORY_ORDER.length; i++) { const category = CATEGORY_ORDER[i]; const isLast = i === (CATEGORY_ORDER.length - 1); @@ -250,7 +218,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } // noinspection JSMethodCanBeStatic - private moveRoomIndexes(nRooms: number, fromCategory: Category, toCategory: Category, indices: ICategoryIndex) { + private moveRoomIndexes(nRooms: number, fromCategory: NotificationColor, toCategory: NotificationColor, indices: ICategoryIndex) { // We have to update the index of the category *after* the from/toCategory variables // in order to update the indices correctly. Because the room is moving from/to those // categories, the next category's index will change - not the category we're modifying. @@ -261,7 +229,7 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { this.alterCategoryPositionBy(toCategory, +nRooms, indices); } - private alterCategoryPositionBy(category: Category, n: number, indices: ICategoryIndex) { + private alterCategoryPositionBy(category: NotificationColor, n: number, indices: ICategoryIndex) { // Note: when we alter a category's index, we actually have to modify the ones following // the target and not the target itself. From cfc39dc4a93a62638d0c4f28f13bbebb58f5ff43 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 16:10:05 -0600 Subject: [PATCH 2/3] Remove now-dead code from sublist resizing --- src/components/views/rooms/RoomSublist2.tsx | 9 +++--- src/stores/room-list/ListLayout.ts | 33 ++------------------- 2 files changed, 6 insertions(+), 36 deletions(-) diff --git a/src/components/views/rooms/RoomSublist2.tsx b/src/components/views/rooms/RoomSublist2.tsx index bfbdd3a161..c351384be4 100644 --- a/src/components/views/rooms/RoomSublist2.tsx +++ b/src/components/views/rooms/RoomSublist2.tsx @@ -20,7 +20,7 @@ import * as React from "react"; import { createRef } from "react"; import { Room } from "matrix-js-sdk/src/models/room"; import classNames from 'classnames'; -import {RovingAccessibleButton, RovingTabIndexWrapper} from "../../../accessibility/RovingTabIndex"; +import { RovingAccessibleButton, RovingTabIndexWrapper } from "../../../accessibility/RovingTabIndex"; import { _t } from "../../../languageHandler"; import AccessibleButton from "../../views/elements/AccessibleButton"; import RoomTile2 from "./RoomTile2"; @@ -36,12 +36,12 @@ import RoomListStore from "../../../stores/room-list/RoomListStore2"; import { ListAlgorithm, SortAlgorithm } from "../../../stores/room-list/algorithms/models"; import { DefaultTagID, TagID } from "../../../stores/room-list/models"; import dis from "../../../dispatcher/dispatcher"; +import defaultDispatcher from "../../../dispatcher/dispatcher"; import NotificationBadge from "./NotificationBadge"; import { ListNotificationState } from "../../../stores/notifications/ListNotificationState"; import AccessibleTooltipButton from "../elements/AccessibleTooltipButton"; import { Key } from "../../../Keyboard"; -import defaultDispatcher from "../../../dispatcher/dispatcher"; -import {ActionPayload} from "../../../dispatcher/payloads"; +import { ActionPayload } from "../../../dispatcher/payloads"; import { Enable, Resizable } from "re-resizable"; import { Direction } from "re-resizable/lib/resizer"; import { polyfillTouchEvent } from "../../../@types/polyfill"; @@ -130,8 +130,7 @@ export default class RoomSublist2 extends React.Component { private calculateInitialHeight() { const requestedVisibleTiles = Math.max(Math.floor(this.layout.visibleTiles), this.layout.minVisibleTiles); const tileCount = Math.min(this.numTiles, requestedVisibleTiles); - const height = this.layout.tilesToPixelsWithPadding(tileCount, this.padding); - return height; + return this.layout.tilesToPixelsWithPadding(tileCount, this.padding); } private get padding() { diff --git a/src/stores/room-list/ListLayout.ts b/src/stores/room-list/ListLayout.ts index f1900487bc..caf2e92bd1 100644 --- a/src/stores/room-list/ListLayout.ts +++ b/src/stores/room-list/ListLayout.ts @@ -18,11 +18,6 @@ import { TagID } from "./models"; const TILE_HEIGHT_PX = 44; -// this comes from the CSS where the show more button is -// mathematically this percent of a tile when floating. -//const RESIZER_BOX_FACTOR = 0.78; -const RESIZER_BOX_FACTOR = 0; - interface ISerializedListLayout { numTiles: number; showPreviews: boolean; @@ -82,36 +77,12 @@ export class ListLayout { } public get minVisibleTiles(): number { - return 1 + RESIZER_BOX_FACTOR; + return 1; } public get defaultVisibleTiles(): number { // This number is what "feels right", and mostly subject to design's opinion. - return 5 + RESIZER_BOX_FACTOR; - } - - public setVisibleTilesWithin(newVal: number, maxPossible: number) { - maxPossible = maxPossible + RESIZER_BOX_FACTOR; - if (newVal > maxPossible) { - this.visibleTiles = maxPossible; - } else { - this.visibleTiles = newVal; - } - } - - public calculateTilesToPixelsMin(maxTiles: number, n: number, possiblePadding: number): number { - // Only apply the padding if we're about to use maxTiles as we need to - // plan for the padding. If we're using n, the padding is already accounted - // for by the resizing stuff. - let padding = 0; - if (maxTiles < n) { - padding = possiblePadding; - } - return this.tilesToPixels(Math.min(maxTiles, n)) + padding; - } - - public tilesWithResizerBoxFactor(n: number): number { - return n + RESIZER_BOX_FACTOR; + return 5; } public tilesWithPadding(n: number, paddingPx: number): number { From 0e49c4343c07875fa9220dce47e464db34789b44 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 10 Jul 2020 21:59:12 -0600 Subject: [PATCH 3/3] Internalize algorithm updates in the new room list store Fixes https://github.com/vector-im/riot-web/issues/14411 The act of setting/changing the algorithm was causing the update function to be marked, meaning we wouldn't trigger an update until something else happened later. To get around this, and still support internal functions spamming calls without multiple updates, we simply move the guts to an internalized function and make the public interface do a trigger. --- src/stores/room-list/RoomListStore2.ts | 16 ++++++++++++---- src/utils/MarkedExecution.ts | 11 ----------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index 38a9509e0c..5ba448b93c 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -368,10 +368,14 @@ export class RoomListStore2 extends AsyncStore { } public async setTagSorting(tagId: TagID, sort: SortAlgorithm) { + await this.setAndPersistTagSorting(tagId, sort); + this.updateFn.trigger(); + } + + private async setAndPersistTagSorting(tagId: TagID, sort: SortAlgorithm) { await this.algorithm.setTagSorting(tagId, sort); // TODO: Per-account? https://github.com/vector-im/riot-web/issues/14114 localStorage.setItem(`mx_tagSort_${tagId}`, sort); - this.updateFn.triggerIfWillMark(); } public getTagSorting(tagId: TagID): SortAlgorithm { @@ -407,10 +411,14 @@ export class RoomListStore2 extends AsyncStore { } public async setListOrder(tagId: TagID, order: ListAlgorithm) { + await this.setAndPersistListOrder(tagId, order); + this.updateFn.trigger(); + } + + private async setAndPersistListOrder(tagId: TagID, order: ListAlgorithm) { await this.algorithm.setListOrdering(tagId, order); // TODO: Per-account? https://github.com/vector-im/riot-web/issues/14114 localStorage.setItem(`mx_listOrder_${tagId}`, order); - this.updateFn.triggerIfWillMark(); } public getListOrder(tagId: TagID): ListAlgorithm { @@ -458,10 +466,10 @@ export class RoomListStore2 extends AsyncStore { const listOrder = this.calculateListOrder(tag); if (tagSort !== definedSort) { - await this.setTagSorting(tag, tagSort); + await this.setAndPersistTagSorting(tag, tagSort); } if (listOrder !== definedOrder) { - await this.setListOrder(tag, listOrder); + await this.setAndPersistListOrder(tag, listOrder); } } } diff --git a/src/utils/MarkedExecution.ts b/src/utils/MarkedExecution.ts index de6cf05953..b0b8fdf63d 100644 --- a/src/utils/MarkedExecution.ts +++ b/src/utils/MarkedExecution.ts @@ -53,15 +53,4 @@ export class MarkedExecution { this.reset(); // reset first just in case the fn() causes a trigger() this.fn(); } - - /** - * Triggers the function if a mark() call would mark it. If the function - * has already been marked this will do nothing. - */ - public triggerIfWillMark() { - if (!this.marked) { - this.mark(); - this.trigger(); - } - } }