From 92e86af162b9d37f79a0c2b1693730c8b65dd3a3 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jul 2020 17:07:51 +0100 Subject: [PATCH 01/15] Show more/Show less keep focus in a relevant place Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/rooms/RoomSublist2.tsx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/components/views/rooms/RoomSublist2.tsx b/src/components/views/rooms/RoomSublist2.tsx index 18f8f4e2f6..9d15e64e7b 100644 --- a/src/components/views/rooms/RoomSublist2.tsx +++ b/src/components/views/rooms/RoomSublist2.tsx @@ -140,15 +140,25 @@ export default class RoomSublist2 extends React.Component { }; private onShowAllClick = () => { - // TODO a11y keep focus somewhere useful: https://github.com/vector-im/riot-web/issues/14180 + const numVisibleTiles = this.numVisibleTiles; this.props.layout.visibleTiles = this.props.layout.tilesWithPadding(this.numTiles, MAX_PADDING_HEIGHT); this.forceUpdate(); // because the layout doesn't trigger a re-render + setImmediate(this.focusRoomTile, numVisibleTiles); // focus the tile after the current bottom one }; private onShowLessClick = () => { - // TODO a11y keep focus somewhere useful: https://github.com/vector-im/riot-web/issues/14180 this.props.layout.visibleTiles = this.props.layout.defaultVisibleTiles; this.forceUpdate(); // because the layout doesn't trigger a re-render + // focus will flow to the show more button here + }; + + private focusRoomTile = (index: number) => { + if (!this.sublistRef.current) return; + const elements = this.sublistRef.current.querySelectorAll(".mx_RoomTile2"); + const element = elements && elements[index]; + if (element) { + element.focus(); + } }; private onOpenMenuClick = (ev: InputEvent) => { @@ -520,6 +530,7 @@ export default class RoomSublist2 extends React.Component { ); if (this.props.isMinimized) showMoreText = null; + // TODO Roving tab index / treeitem?: https://github.com/vector-im/riot-web/issues/14180 showNButton = ( From f18db23cc4e7138c8b5a55598430711c59bf65fe Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jul 2020 17:18:56 +0100 Subject: [PATCH 02/15] Remove some TODOs Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/structures/LeftPanel2.tsx | 3 --- src/components/views/rooms/RoomSublist2.tsx | 1 - src/components/views/rooms/RoomTile2.tsx | 3 --- 3 files changed, 7 deletions(-) diff --git a/src/components/structures/LeftPanel2.tsx b/src/components/structures/LeftPanel2.tsx index 4b954d7843..b2a2384cb2 100644 --- a/src/components/structures/LeftPanel2.tsx +++ b/src/components/structures/LeftPanel2.tsx @@ -70,8 +70,6 @@ export default class LeftPanel2 extends React.Component { private tagPanelWatcherRef: string; private focusedElement = null; - // TODO: a11y: https://github.com/vector-im/riot-web/issues/14180 - constructor(props: IProps) { super(props); @@ -264,7 +262,6 @@ export default class LeftPanel2 extends React.Component { onVerticalArrow={this.onKeyDown} /> { ); - // TODO: a11y (see old component): https://github.com/vector-im/riot-web/issues/14180 // Note: the addRoomButton conditionally gets moved around // the DOM depending on whether or not the list is minimized. // If we're minimized, we want it below the header so it diff --git a/src/components/views/rooms/RoomTile2.tsx b/src/components/views/rooms/RoomTile2.tsx index c6cd401803..abb31a6f71 100644 --- a/src/components/views/rooms/RoomTile2.tsx +++ b/src/components/views/rooms/RoomTile2.tsx @@ -118,8 +118,6 @@ const NotifOption: React.FC = ({active, onClick, iconClassNam }; export default class RoomTile2 extends React.Component { - // TODO: a11y: https://github.com/vector-im/riot-web/issues/14180 - constructor(props: IProps) { super(props); @@ -390,7 +388,6 @@ export default class RoomTile2 extends React.Component { public render(): React.ReactElement { // TODO: Invites: https://github.com/vector-im/riot-web/issues/14198 - // TODO: a11y proper: https://github.com/vector-im/riot-web/issues/14180 const classes = classNames({ 'mx_RoomTile2': true, From 4edd3dfc6c9b8bc3185adaa231b82ba65fa4ea10 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jul 2020 17:46:33 +0100 Subject: [PATCH 03/15] Convert RovingTabIndex to Typescript Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../{RovingTabIndex.js => RovingTabIndex.tsx} | 87 +++++++++++++------ 1 file changed, 62 insertions(+), 25 deletions(-) rename src/accessibility/{RovingTabIndex.js => RovingTabIndex.tsx} (79%) diff --git a/src/accessibility/RovingTabIndex.js b/src/accessibility/RovingTabIndex.tsx similarity index 79% rename from src/accessibility/RovingTabIndex.js rename to src/accessibility/RovingTabIndex.tsx index b481f08fe2..32780629f2 100644 --- a/src/accessibility/RovingTabIndex.js +++ b/src/accessibility/RovingTabIndex.tsx @@ -22,9 +22,13 @@ import React, { useMemo, useRef, useReducer, + Reducer, + RefObject, + Dispatch, } from "react"; -import PropTypes from "prop-types"; + import {Key} from "../Keyboard"; +import AccessibleButton from "../components/views/elements/AccessibleButton"; /** * Module to simplify implementing the Roving TabIndex accessibility technique @@ -41,7 +45,19 @@ import {Key} from "../Keyboard"; const DOCUMENT_POSITION_PRECEDING = 2; -const RovingTabIndexContext = createContext({ +type Ref = RefObject; + +interface IState { + activeRef: Ref; + refs: Ref[]; +} + +interface IContext { + state: IState; + dispatch: Dispatch; +} + +const RovingTabIndexContext = createContext({ state: { activeRef: null, refs: [], // list of refs in DOM order @@ -50,16 +66,22 @@ const RovingTabIndexContext = createContext({ }); RovingTabIndexContext.displayName = "RovingTabIndexContext"; -// TODO use a TypeScript type here -const types = { - REGISTER: "REGISTER", - UNREGISTER: "UNREGISTER", - SET_FOCUS: "SET_FOCUS", -}; +enum Type { + Register = "REGISTER", + Unregister = "UNREGISTER", + SetFocus = "SET_FOCUS", +} -const reducer = (state, action) => { +interface IAction { + type: Type; + payload: { + ref: Ref; + }; +} + +const reducer = (state: IState, action: IAction) => { switch (action.type) { - case types.REGISTER: { + case Type.Register: { if (state.refs.length === 0) { // Our list of refs was empty, set activeRef to this first item return { @@ -92,7 +114,7 @@ const reducer = (state, action) => { ], }; } - case types.UNREGISTER: { + case Type.Unregister: { // filter out the ref which we are removing const refs = state.refs.filter(r => r !== action.payload.ref); @@ -117,7 +139,7 @@ const reducer = (state, action) => { refs, }; } - case types.SET_FOCUS: { + case Type.SetFocus: { // update active ref return { ...state, @@ -129,13 +151,21 @@ const reducer = (state, action) => { } }; -export const RovingTabIndexProvider = ({children, handleHomeEnd, onKeyDown}) => { - const [state, dispatch] = useReducer(reducer, { +interface IProps { + handleHomeEnd?: boolean; + children(renderProps: { + onKeyDownHandler(ev: React.KeyboardEvent); + }); + onKeyDown?(ev: React.KeyboardEvent); +} + +export const RovingTabIndexProvider: React.FC = ({children, handleHomeEnd, onKeyDown}) => { + const [state, dispatch] = useReducer>(reducer, { activeRef: null, refs: [], }); - const context = useMemo(() => ({state, dispatch}), [state]); + const context = useMemo(() => ({state, dispatch}), [state]); const onKeyDownHandler = useCallback((ev) => { let handled = false; @@ -171,19 +201,17 @@ export const RovingTabIndexProvider = ({children, handleHomeEnd, onKeyDown}) => { children({onKeyDownHandler}) } ; }; -RovingTabIndexProvider.propTypes = { - handleHomeEnd: PropTypes.bool, - onKeyDown: PropTypes.func, -}; + +type FocusHandler = () => void; // Hook to register a roving tab index // inputRef parameter specifies the ref to use // onFocus should be called when the index gained focus in any manner // isActive should be used to set tabIndex in a manner such as `tabIndex={isActive ? 0 : -1}` // ref should be passed to a DOM node which will be used for DOM compareDocumentPosition -export const useRovingTabIndex = (inputRef) => { +export const useRovingTabIndex = (inputRef: Ref): [FocusHandler, boolean, Ref] => { const context = useContext(RovingTabIndexContext); - let ref = useRef(null); + let ref = useRef(null); if (inputRef) { // if we are given a ref, use it instead of ours @@ -193,13 +221,13 @@ export const useRovingTabIndex = (inputRef) => { // setup (after refs) useLayoutEffect(() => { context.dispatch({ - type: types.REGISTER, + type: Type.Register, payload: {ref}, }); // teardown return () => { context.dispatch({ - type: types.UNREGISTER, + type: Type.Unregister, payload: {ref}, }); }; @@ -207,7 +235,7 @@ export const useRovingTabIndex = (inputRef) => { const onFocus = useCallback(() => { context.dispatch({ - type: types.SET_FOCUS, + type: Type.SetFocus, payload: {ref}, }); }, [ref, context]); @@ -216,8 +244,17 @@ export const useRovingTabIndex = (inputRef) => { return [onFocus, isActive, ref]; }; +interface IRovingTabIndexWrapperProps { + inputRef?: Ref; + children(renderProps: { + onFocus: FocusHandler; + isActive: boolean; + ref: Ref; + }); +} + // Wrapper to allow use of useRovingTabIndex outside of React Functional Components. -export const RovingTabIndexWrapper = ({children, inputRef}) => { +export const RovingTabIndexWrapper: React.FC = ({children, inputRef}) => { const [onFocus, isActive, ref] = useRovingTabIndex(inputRef); return children({onFocus, isActive, ref}); }; From a33717a475a74f9b4bd773169fb624de450ed509 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jul 2020 17:47:21 +0100 Subject: [PATCH 04/15] Wire up Room sublist show more/less as roving tabindex button using new helper Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/accessibility/RovingTabIndex.tsx | 10 ++++++++++ src/components/views/rooms/RoomSublist2.tsx | 10 +++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx index 32780629f2..388d67d9f3 100644 --- a/src/accessibility/RovingTabIndex.tsx +++ b/src/accessibility/RovingTabIndex.tsx @@ -259,3 +259,13 @@ export const RovingTabIndexWrapper: React.FC = ({ch return children({onFocus, isActive, ref}); }; +interface IRovingAccessibleButtonProps extends React.ComponentProps { + inputRef?: Ref; +} + +// Wrapper to allow use of useRovingTabIndex for simple AccessibleButtons outside of React Functional Components. +export const RovingAccessibleButton: React.FC = ({inputRef, ...props}) => { + const [onFocus, isActive, ref] = useRovingTabIndex(inputRef); + return ; +}; + diff --git a/src/components/views/rooms/RoomSublist2.tsx b/src/components/views/rooms/RoomSublist2.tsx index 3a12e99914..56ce631604 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 { RovingTabIndexWrapper } from "../../../accessibility/RovingTabIndex"; +import {RovingAccessibleButton, RovingTabIndexWrapper} from "../../../accessibility/RovingTabIndex"; import { _t } from "../../../languageHandler"; import AccessibleButton from "../../views/elements/AccessibleButton"; import RoomTile2 from "./RoomTile2"; @@ -531,12 +531,12 @@ export default class RoomSublist2 extends React.Component { if (this.props.isMinimized) showMoreText = null; // TODO Roving tab index / treeitem?: https://github.com/vector-im/riot-web/issues/14180 showNButton = ( - + {/* set by CSS masking */} {showMoreText} - + ); } else if (this.numTiles <= visibleTiles.length && this.numTiles > this.props.layout.defaultVisibleTiles) { // we have all tiles visible - add a button to show less @@ -548,12 +548,12 @@ export default class RoomSublist2 extends React.Component { if (this.props.isMinimized) showLessText = null; // TODO Roving tab index / treeitem?: https://github.com/vector-im/riot-web/issues/14180 showNButton = ( - + {/* set by CSS masking */} {showLessText} - + ); } From 28310cb64848713486bf4ac05ff4c83517cf1db1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 7 Jul 2020 17:48:39 +0100 Subject: [PATCH 05/15] remove TODOs Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/rooms/RoomSublist2.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/views/rooms/RoomSublist2.tsx b/src/components/views/rooms/RoomSublist2.tsx index 56ce631604..eefd29f0b7 100644 --- a/src/components/views/rooms/RoomSublist2.tsx +++ b/src/components/views/rooms/RoomSublist2.tsx @@ -529,7 +529,6 @@ export default class RoomSublist2 extends React.Component { ); if (this.props.isMinimized) showMoreText = null; - // TODO Roving tab index / treeitem?: https://github.com/vector-im/riot-web/issues/14180 showNButton = ( @@ -546,7 +545,6 @@ export default class RoomSublist2 extends React.Component { ); if (this.props.isMinimized) showLessText = null; - // TODO Roving tab index / treeitem?: https://github.com/vector-im/riot-web/issues/14180 showNButton = ( From e6b20088c0a1c87067f99444d3f90b693ad26cf4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 11:33:32 -0600 Subject: [PATCH 06/15] Try using requestAnimationFrame if available for sticky headers This might help performance, or it might not. Let's try it! --- src/components/structures/LeftPanel2.tsx | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/components/structures/LeftPanel2.tsx b/src/components/structures/LeftPanel2.tsx index b2a2384cb2..36012a1473 100644 --- a/src/components/structures/LeftPanel2.tsx +++ b/src/components/structures/LeftPanel2.tsx @@ -69,6 +69,7 @@ export default class LeftPanel2 extends React.Component { private listContainerRef: React.RefObject = createRef(); private tagPanelWatcherRef: string; private focusedElement = null; + private isDoingStickyHeaders = false; constructor(props: IProps) { super(props); @@ -113,6 +114,24 @@ export default class LeftPanel2 extends React.Component { }; private handleStickyHeaders(list: HTMLDivElement) { + // TODO: Evaluate if this has any performance benefit or detriment. + // See https://github.com/vector-im/riot-web/issues/14035 + + if (this.isDoingStickyHeaders) return; + this.isDoingStickyHeaders = true; + if (window.requestAnimationFrame) { + window.requestAnimationFrame(() => { + this.doStickyHeaders(list); + this.isDoingStickyHeaders = false; + }); + } else { + this.doStickyHeaders(list); + this.isDoingStickyHeaders = false; + } + } + + private doStickyHeaders(list: HTMLDivElement) { + this.isDoingStickyHeaders = true; const rlRect = list.getBoundingClientRect(); const bottom = rlRect.bottom; const top = rlRect.top; From baccabeae461048e1da8d9d52f4b51c33ab170ed Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 11:34:52 -0600 Subject: [PATCH 07/15] Remove extraneous true --- src/components/structures/LeftPanel2.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/structures/LeftPanel2.tsx b/src/components/structures/LeftPanel2.tsx index 36012a1473..7fac6cbff1 100644 --- a/src/components/structures/LeftPanel2.tsx +++ b/src/components/structures/LeftPanel2.tsx @@ -131,7 +131,6 @@ export default class LeftPanel2 extends React.Component { } private doStickyHeaders(list: HTMLDivElement) { - this.isDoingStickyHeaders = true; const rlRect = list.getBoundingClientRect(); const bottom = rlRect.bottom; const top = rlRect.top; From 7963ed6d047536dfa545a0ff11c332eba7476072 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 13:42:15 -0600 Subject: [PATCH 08/15] Mute "Unknown room caused setting update" spam See comment enclosed within. Fixes https://github.com/vector-im/riot-web/issues/14254 --- src/settings/handlers/RoomSettingsHandler.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index d8e775742c..00dd5b8bec 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.js @@ -43,11 +43,14 @@ export default class RoomSettingsHandler extends MatrixClientBackedSettingsHandl const roomId = event.getRoomId(); const room = this.client.getRoom(roomId); - // Note: the tests often fire setting updates that don't have rooms in the store, so - // we fail softly here. We shouldn't assume that the state being fired is current - // state, but we also don't need to explode just because we didn't find a room. - if (!room) console.warn(`Unknown room caused setting update: ${roomId}`); - if (room && state !== room.currentState) return; // ignore state updates which are not current + // Note: in tests and during the encryption setup on initial load we might not have + // rooms in the store, so we just quietly ignore the problem. If we log it then we'll + // just end up spamming the logs a few thousand times. It is perfectly fine for us + // to ignore the problem as the app will not have loaded enough to care yet. + if (!room) return; + + // ignore state updates which are not current + if (room && state !== room.currentState) return; if (event.getType() === "org.matrix.room.preview_urls") { let val = event.getContent()['disable']; From be1b2fddafd7a3588bbbd3e7709540b04a550a8c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 13:46:10 -0600 Subject: [PATCH 09/15] Ensure DMs are not lost in the new room list Fixes https://github.com/vector-im/riot-web/issues/14236 --- src/stores/room-list/algorithms/Algorithm.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index d5f2ed0053..41066fc14b 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -530,9 +530,11 @@ export class Algorithm extends EventEmitter { } if (!inTag) { - // TODO: Determine if DM and push there instead: https://github.com/vector-im/riot-web/issues/14236 - newTags[DefaultTagID.Untagged].push(room); - + if (DMRoomMap.getUserIdForRoomId(room.roomId)) { + newTags[DefaultTagID.DM].push(room); + } else { + newTags[DefaultTagID.Untagged].push(room); + } // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[DEBUG] "${room.name}" (${room.roomId}) is Untagged`); } From 2488520263d0097f6a08fefd98f1540de7bc8839 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 13:46:29 -0600 Subject: [PATCH 10/15] Clean up tag logging in setKnownRooms We don't need this anymore --- src/stores/room-list/algorithms/Algorithm.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 41066fc14b..e70d7d468e 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -501,13 +501,9 @@ export class Algorithm extends EventEmitter { // Split out the easy rooms first (leave and invite) const memberships = splitRoomsByMembership(rooms); for (const room of memberships[EffectiveMembership.Invite]) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[DEBUG] "${room.name}" (${room.roomId}) is an Invite`); newTags[DefaultTagID.Invite].push(room); } for (const room of memberships[EffectiveMembership.Leave]) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[DEBUG] "${room.name}" (${room.roomId}) is Historical`); newTags[DefaultTagID.Archived].push(room); } @@ -518,11 +514,7 @@ export class Algorithm extends EventEmitter { let inTag = false; if (tags.length > 0) { for (const tag of tags) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[DEBUG] "${room.name}" (${room.roomId}) is tagged as ${tag}`); if (!isNullOrUndefined(newTags[tag])) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[DEBUG] "${room.name}" (${room.roomId}) is tagged with VALID tag ${tag}`); newTags[tag].push(room); inTag = true; } @@ -535,8 +527,6 @@ export class Algorithm extends EventEmitter { } else { newTags[DefaultTagID.Untagged].push(room); } - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[DEBUG] "${room.name}" (${room.roomId}) is Untagged`); } } From 34ea8342fb3654f5a18ec07df5137a4159f9cc8a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 13:49:36 -0600 Subject: [PATCH 11/15] Remove comment claiming encrypted rooms are handled incorrectly Fixes https://github.com/vector-im/riot-web/issues/14238 The encrypted rooms are loaded on startup (eventually), so we don't need to worry about the problem described. --- src/stores/room-list/RoomListStore2.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index e5205f6051..60174b63c8 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -221,9 +221,6 @@ export class RoomListStore2 extends AsyncStore { } // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Decrypted timeline event ${eventPayload.event.getId()} in ${roomId}`); - // TODO: Verify that e2e rooms are handled on init: https://github.com/vector-im/riot-web/issues/14238 - // It seems like when viewing the room the timeline is decrypted, rather than at startup. This could - // cause inaccuracies with the list ordering. We may have to decrypt the last N messages of every room :( await this.handleRoomUpdate(room, RoomUpdateCause.Timeline); } else if (payload.action === 'MatrixActions.accountData' && payload.event_type === 'm.direct') { const eventPayload = (payload); // TODO: Type out the dispatcher types From a4ef5909f93cdb1685efe7673ae601b8b6746433 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 14:45:08 -0600 Subject: [PATCH 12/15] Respect and fix understanding of legacy options Fixes https://github.com/vector-im/riot-web/issues/14372 We read/use the options in multiple places, and those places were not in sync. Now when algorithms change and on initial load, both will come to the same conclusions about how to order & sort the rooms. --- src/settings/Settings.js | 4 +- src/stores/room-list/RoomListStore2.ts | 57 ++++++++++++++++---- src/stores/room-list/algorithms/Algorithm.ts | 2 + 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/settings/Settings.js b/src/settings/Settings.js index 58d9ed4f31..0011ecfccd 100644 --- a/src/settings/Settings.js +++ b/src/settings/Settings.js @@ -478,13 +478,13 @@ export const SETTINGS = { deny: [], }, }, - // TODO: Remove setting: https://github.com/vector-im/riot-web/issues/14231 + // TODO: Remove setting: https://github.com/vector-im/riot-web/issues/14373 "RoomList.orderAlphabetically": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, displayName: _td("Order rooms by name"), default: false, }, - // TODO: Remove setting: https://github.com/vector-im/riot-web/issues/14231 + // TODO: Remove setting: https://github.com/vector-im/riot-web/issues/14373 "RoomList.orderByImportance": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, displayName: _td("Show rooms with unread notifications first"), diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index e5205f6051..ac765b1e05 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -31,6 +31,7 @@ import RoomViewStore from "../RoomViewStore"; import { Algorithm, LIST_UPDATED_EVENT } from "./algorithms/Algorithm"; import { EffectiveMembership, getEffectiveMembership } from "./membership"; import { ListLayout } from "./ListLayout"; +import { isNullOrUndefined } from "matrix-js-sdk/src/utils"; interface IState { tagsEnabled?: boolean; @@ -321,6 +322,28 @@ export class RoomListStore2 extends AsyncStore { return localStorage.getItem(`mx_tagSort_${tagId}`); } + // logic must match calculateListOrder + private calculateTagSorting(tagId: TagID): SortAlgorithm { + const defaultSort = SortAlgorithm.Alphabetic; + const settingAlphabetical = SettingsStore.getValue("RoomList.orderAlphabetically", null, true); + const definedSort = this.getTagSorting(tagId); + const storedSort = this.getStoredTagSorting(tagId); + + // We use the following order to determine which of the 4 flags to use: + // Stored > Settings > Defined > Default + + let tagSort = defaultSort; + if (storedSort) { + tagSort = storedSort; + } else if (!isNullOrUndefined(settingAlphabetical)) { + tagSort = settingAlphabetical ? SortAlgorithm.Alphabetic : SortAlgorithm.Recent; + } else if (definedSort) { + tagSort = definedSort; + } // else default (already set) + + return tagSort; + } + public async setListOrder(tagId: TagID, order: ListAlgorithm) { await this.algorithm.setListOrdering(tagId, order); // TODO: Per-account? https://github.com/vector-im/riot-web/issues/14114 @@ -337,19 +360,35 @@ export class RoomListStore2 extends AsyncStore { return localStorage.getItem(`mx_listOrder_${tagId}`); } - private async updateAlgorithmInstances() { - const defaultSort = SortAlgorithm.Alphabetic; + // logic must match calculateTagSorting + private calculateListOrder(tagId: TagID): ListAlgorithm { const defaultOrder = ListAlgorithm.Natural; + const settingImportance = SettingsStore.getValue("RoomList.orderByImportance", null, true); + const definedOrder = this.getListOrder(tagId); + const storedOrder = this.getStoredListOrder(tagId); + // We use the following order to determine which of the 4 flags to use: + // Stored > Settings > Defined > Default + + let listOrder = defaultOrder; + if (storedOrder) { + listOrder = storedOrder; + } else if (!isNullOrUndefined(settingImportance)) { + listOrder = settingImportance ? ListAlgorithm.Importance : ListAlgorithm.Natural; + } else if (definedOrder) { + listOrder = definedOrder; + } // else default (already set) + + return listOrder; + } + + private async updateAlgorithmInstances() { for (const tag of Object.keys(this.orderedLists)) { const definedSort = this.getTagSorting(tag); const definedOrder = this.getListOrder(tag); - const storedSort = this.getStoredTagSorting(tag); - const storedOrder = this.getStoredListOrder(tag); - - const tagSort = storedSort ? storedSort : (definedSort ? definedSort : defaultSort); - const listOrder = storedOrder ? storedOrder : (definedOrder ? definedOrder : defaultOrder); + const tagSort = this.calculateTagSorting(tag); + const listOrder = this.calculateListOrder(tag); if (tagSort !== definedSort) { await this.setTagSorting(tag, tagSort); @@ -378,8 +417,8 @@ export class RoomListStore2 extends AsyncStore { const sorts: ITagSortingMap = {}; const orders: IListOrderingMap = {}; for (const tagId of OrderedDefaultTagIDs) { - sorts[tagId] = this.getStoredTagSorting(tagId) || SortAlgorithm.Alphabetic; - orders[tagId] = this.getStoredListOrder(tagId) || ListAlgorithm.Natural; + sorts[tagId] = this.calculateTagSorting(tagId); + orders[tagId] = this.calculateListOrder(tagId); } if (this.state.tagsEnabled) { diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index d5f2ed0053..91d156d0d9 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -109,6 +109,7 @@ export class Algorithm extends EventEmitter { } public getTagSorting(tagId: TagID): SortAlgorithm { + if (!this.sortAlgorithms) return null; return this.sortAlgorithms[tagId]; } @@ -125,6 +126,7 @@ export class Algorithm extends EventEmitter { } public getListOrdering(tagId: TagID): ListAlgorithm { + if (!this.listAlgorithms) return null; return this.listAlgorithms[tagId]; } From 774e32ecf0dd3f36b467b82df4ad377c6aa924c4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 7 Jul 2020 16:16:46 -0600 Subject: [PATCH 13/15] Fix DM handling in new room list --- src/stores/room-list/algorithms/Algorithm.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 361f42eac5..8c7bbc8615 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -524,7 +524,7 @@ export class Algorithm extends EventEmitter { } if (!inTag) { - if (DMRoomMap.getUserIdForRoomId(room.roomId)) { + if (DMRoomMap.shared().getUserIdForRoomId(room.roomId)) { newTags[DefaultTagID.DM].push(room); } else { newTags[DefaultTagID.Untagged].push(room); From 92dec8ddd81b326558a9032bce253138d3fcfd52 Mon Sep 17 00:00:00 2001 From: Jorik Schellekens Date: Wed, 8 Jul 2020 00:16:24 +0100 Subject: [PATCH 14/15] Fix gaps --- res/css/views/rooms/_RoomSublist2.scss | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/res/css/views/rooms/_RoomSublist2.scss b/res/css/views/rooms/_RoomSublist2.scss index 0e76152f86..a8de6dd409 100644 --- a/res/css/views/rooms/_RoomSublist2.scss +++ b/res/css/views/rooms/_RoomSublist2.scss @@ -203,15 +203,15 @@ limitations under the License. // Update the render() function for RoomSublist2 if these change // Update the ListLayout class for minVisibleTiles if these change. // - // At 24px high and 8px padding on the top this equates to 0.65 of + // At 28px high and 8px padding on the top this equates to 0.73 of // a tile due to how the padding calculations work. - height: 24px; + height: 28px; padding-top: 8px; // We force this to the bottom so it will overlap rooms as needed. // We account for the space it takes up (24px) in the code through padding. position: absolute; - bottom: 4px; // the height of the resize handle + bottom: 0; // the height of the resize handle left: 0; right: 0; From 0906da01baa1724df28814afc34d45d9e4fbeca9 Mon Sep 17 00:00:00 2001 From: Jorik Schellekens Date: Wed, 8 Jul 2020 00:18:58 +0100 Subject: [PATCH 15/15] Fix gaps --- res/css/views/rooms/_RoomSublist2.scss | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/res/css/views/rooms/_RoomSublist2.scss b/res/css/views/rooms/_RoomSublist2.scss index a8de6dd409..d08bc09031 100644 --- a/res/css/views/rooms/_RoomSublist2.scss +++ b/res/css/views/rooms/_RoomSublist2.scss @@ -203,10 +203,11 @@ limitations under the License. // Update the render() function for RoomSublist2 if these change // Update the ListLayout class for minVisibleTiles if these change. // - // At 28px high and 8px padding on the top this equates to 0.73 of + // At 24px high, 8px padding on the top and 4px padding on the bottom this equates to 0.73 of // a tile due to how the padding calculations work. - height: 28px; + height: 24px; padding-top: 8px; + padding-bottom: 4px; // We force this to the bottom so it will overlap rooms as needed. // We account for the space it takes up (24px) in the code through padding.