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

Display favourite messages in a room-like view #9719

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Dec 7, 2022

Originally developed by @yaya-usman but remodelled quite a bit now by @andybalaam

This creates a new view that you can access in a similar way to a room:
image

It displays all your favourite messages.

Apologies for the huge review. Do ask me and I can walk you through it interactively.


Here's what your changelog entry will look like:

✨ Features

  • Display favourite messages in a room-like view (#9719). Contributed by @andybalaam.

@andybalaam
Copy link
Member Author

Work will continue on this in January

@andybalaam
Copy link
Member Author

Prerequisite: matrix-org/matrix-analytics-events#76

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

generally seems good, thank you!

lastRoomId = roomId!;
}

const resultLink = "#/room/" + roomId + "/" + mxEvent.getId();
Copy link
Member

Choose a reason for hiding this comment

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

this appears to be an assumption of our url routing which isn't guaranteed - can we convert it to a view_room dispatch instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into the code, it seems that EventTile does not currently support an onClick or similar event, so the only way to allow clicking a tile to jump somewhere else is with its highlightLink property as we have done here and in RoomSearchView.

Given how long this PR has festered, I'd like to argue that fixing this is outside of the scope of this PR, and something we can do later.

src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
src/dispatcher/actions.ts Outdated Show resolved Hide resolved
src/dispatcher/actions.ts Outdated Show resolved Hide resolved
src/dispatcher/actions.ts Outdated Show resolved Hide resolved
src/hooks/useFavouriteMessages.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@andybalaam
Copy link
Member Author

i18n test is failing with

fatal: couldn't find remote ref andybalaam/favourite-messages-rebased

which I don't understand

@andybalaam
Copy link
Member Author

OK, CI seems happy so requesting a re-review from @t3chguy

@andybalaam
Copy link
Member Author

OK, CI seems happy so requesting a re-review from @t3chguy

Scratch that - techguy is too busy today so I'll ask someone else.

@weeman1337
Copy link
Contributor

weeman1337 commented Jan 20, 2023

Played a bit around with it on the deployment here. I like it 👍
Some issues I found:

If you favourite a poll, it explodes with:

Uncaught (in promise) TypeError: f.content.body is undefined
    filterFavourites FavouriteMessagesView.tsx:72
    filterFavourites FavouriteMessagesView.tsx:71
    calcEvents FavouriteMessagesView.tsx:134
    recalcEvents FavouriteMessagesView.tsx:43
    FavouriteMessagesView FavouriteMessagesView.tsx:49
    fk React
    unstable_runWithPriority scheduler.production.min.js:18
    React 4
    unstable_runWithPriority scheduler.production.min.js:18
    React 6
    setState MatrixChat.tsx:414
    setPage MatrixChat.tsx:888
    viewFavouriteMessages MatrixChat.tsx:1051
    MatrixChat MatrixChat.tsx:848
    _invokeCallback Dispatcher.js:214
    dispatch Dispatcher.js:189
    setTimeout handler*dispatch dispatcher.ts:52
    onFavouriteClicked RoomList.tsx:716
    React 11
    unstable_runWithPriority scheduler.production.min.js:18
    React 11
    loadApp init.tsx:150
    start index.ts:228
    async* index.ts:239
    Webpack 3
[FavouriteMessagesView.tsx:72:9](webpack:///src/components/structures/FavouriteMessagesView/FavouriteMessagesView.tsx)

(same for broadcasts)


You cannot jump to the room message, if you favourite an image, voice message or location share (click the message in the favourites view).


The room separation in the favourite view could be improved.

image

Maybe just add more spacing above the room names.


If you favourite a thread root, the thread summary is shown in favourites. But clicking it does nothing.


Ctrl + F (enabled in options) doesn't trigger search in the favourites view.


It would be nice if the search field can get the focus if you click the 🔍


Room search with unexpected result, when hitting random keys on the keyboard.

image

@bwindels
Copy link
Contributor

If I read correctly, this will save all favorite message ids in a single account data entry (favourite_messages with element settings prefix). The downsides of this approach would be that the array could grow quite large, and also data loss in case of a race between multiple clients/devices (a pain currently with the DM rooms mapping). Curious what your thoughts are on these, and if storing a single favorite (or only favorites for one room) per account data entry was considered. I can see that would probably be harder to integrate with the element settings code though.
The reason I bring it up (of lesser importance) is also that a pattern like io.element.favourite_messages.<roomid>.<eventid> for the type with only 1 favourite per account data entry would make things easier in Hydrogen as well to only load in memory what is needed.

@andybalaam
Copy link
Member Author

If I read correctly, this will save all favorite message ids in a single account data entry (favourite_messages with element settings prefix). The downsides of this approach would be that the array could grow quite large, and also data loss in case of a race between multiple clients/devices (a pain currently with the DM rooms mapping). Curious what your thoughts are on these, and if storing a single favorite (or only favorites for one room) per account data entry was considered. I can see that would probably be harder to integrate with the element settings code though. The reason I bring it up (of lesser importance) is also that a pattern like io.element.favourite_messages.<roomid>.<eventid> for the type with only 1 favourite per account data entry would make things easier in Hydrogen as well to only load in memory what is needed.

Because this PR started out using local storage, I totally forgot when I converted it to use SettingsStorage that there is an MSC we are planning to follow: matrix-org/matrix-spec-proposals#2437

Note: that MSC proposes using Room Account Data, but I plan to use general Account Data because I think favourites should not be tied to rooms.

@bwindels I think that MSC also uses a single account data entry for everything - will that cause you similar problems?

@bwindels
Copy link
Contributor

bwindels commented Jan 20, 2023

If I read correctly, this will save all favorite message ids in a single account data entry (favourite_messages with element settings prefix). The downsides of this approach would be that the array could grow quite large, and also data loss in case of a race between multiple clients/devices (a pain currently with the DM rooms mapping). Curious what your thoughts are on these, and if storing a single favorite (or only favorites for one room) per account data entry was considered. I can see that would probably be harder to integrate with the element settings code though. The reason I bring it up (of lesser importance) is also that a pattern like io.element.favourite_messages.<roomid>.<eventid> for the type with only 1 favourite per account data entry would make things easier in Hydrogen as well to only load in memory what is needed.

Because this PR started out using local storage, I totally forgot when I converted it to use SettingsStorage that there is an MSC we are planning to follow: matrix-org/matrix-spec-proposals#2437

Thanks for the MSC link. I always forget about room account data :) The size of the array would indeed be more reasonable when using room account data, as well as the scope for races would be smaller. The race is indeed described as a limitation. The size of the array would indeed be more reasonable when using room account data). So yeah, the race is a discussion for the MSC.

Note: that MSC proposes using Room Account Data, but I plan to use general Account Data because I think favourites should not be tied to rooms.

But favourites are always for events, which can only occur within a room, no?

@bwindels I think that MSC also uses a single account data entry for everything - will that cause you similar problems?


.mx_FavMessagesHeader_cancelButton {
background-color: $alert;
mask: url("$(res)/img/cancel.svg");
Copy link
Contributor

@florianduros florianduros Jan 20, 2023

Choose a reason for hiding this comment

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

We are trying to get rid of mask in favour of the svg as a React component directly

Example of use:
https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/rooms/wysiwyg_composer/components/FormattingButtons.tsx#L21

}

.mx_FavMessagesHeader_sortButton::before {
mask-image: url("$(res)/img/element-icons/room/sort-twoway.svg");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about the mask

}

.mx_FavMessagesHeader_clearAllButton::before {
mask-image: url("$(res)/img/element-icons/room/clear-all.svg");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about the mask

color: $primary-content;
font-weight: $font-semi-bold;
font-size: $font-18px;
margin: 0 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use here $spacing-8

mask-position: center;
mask-size: 17px;
padding: 9px;
margin: 0 12px 0 3px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use $spacing-*

let lastInSection = true;
const nextEv = props?.timeline[j + 1];
if (nextEv) {
lastInSection =
Copy link
Contributor

Choose a reason for hiding this comment

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

const lastInSection = nextEv && 
                 ( dateSeparator(mxEv, nextEv) ||
                    mxEv.getSender() !== nextEv.getSender() ||
                    !shouldFormContinuation(mxEv, nextEv, context?.showHiddenEvents, threadsEnabled)
                    )

recalcEvents();
}, [searchQuery, recalcEvents]);

registerFavouritesChangedListener(() => recalcEvents());
Copy link
Contributor

@florianduros florianduros Jan 20, 2023

Choose a reason for hiding this comment

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

At every render, it will call the registerFavouritesChangedListener and add listener

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have an alternative suggestion?

Copy link
Contributor

@florianduros florianduros Jan 20, 2023

Choose a reason for hiding this comment

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

We can add the listener in a useEffect:

useEffect(() => {
     const handler = () => recalcEvents();
     registerFavouritesChangedListener(handler);
     return () => {
         // here we would like to unregister the handler
         unregisterFavouritesChangedListener(handler);
     }
}, [registerFavouritesChangedListener, recalcEvents]

If we want to avoid to repeat this behaviour every time we use the useFavourites hook. We can pass the handler as argument of the hook and make all the register/unregister in it.

const displayedFavourites: FavouriteStorage[] = filterFavourites(searchQuery, favouriteMessages);
const promises: Promise<MatrixEvent | null>[] = displayedFavourites.map((f) => fetchEvent(f, matrixClient));
const events = await Promise.all(promises);
return events.filter((e) => e !== null) as MatrixEvent[]; // force cast because typescript doesn't understand what `filter` 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 can use https://stackoverflow.com/questions/43118692/typescript-filter-out-nulls-from-an-array this kind of predicate to cleanly filter without casting the type

useFavouriteMessages(favouriteMessagesStore);
const [, forceRefresh] = useState([]);

registerFavouritesChangedListener(() => forceRefresh([]));
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be a bit hacky here, maybe useFavouriteMessages should return something to trigger the rerender ?

} {
const [, setX] = useState<string[]>();
const myListeners: (() => void)[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

At every render, myListeners is replaced by an empty array which seems to be unwanted behaviour.

It needs to be a useState or useRef

@andybalaam
Copy link
Member Author

Note: that MSC proposes using Room Account Data, but I plan to use general Account Data because I think favourites should not be tied to rooms.

But favourites are always for events, which can only occur within a room, no?

Yes, but I argue that:

a) When I want to view my favourites I don't want to iterate all rooms
b) When I leave a room I don't want to lose the information that I favourited a message inside it

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Some minor findings in the code.

align-items: center;
min-width: 0;
margin: 0 20px 0 16px;
padding-top: 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use spacing variables, where possible?
Like for instance here $spacing-8.

@@ -0,0 +1,57 @@
/*
Copyright 2022 The Matrix.org Foundation C.I.C.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already 2023 🍾

}

const FavouriteMessageTile: FC<IProps> = (props: IProps) => {
let context!: React.ContextType<typeof RoomContext>;
Copy link
Contributor

Choose a reason for hiding this comment

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

context doesn't seem to be set anywhere.

height={26}
resizeMethod="crop"
/>
<span>Favourite Messages</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

String missing translation.

(also strings in some other places)

);
};

export default ConfirmClearFavouritesDialog;
Copy link
Contributor

Choose a reason for hiding this comment

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

Applies to the entire PR: We should use named exports.

}

const FavouriteMessagesPanel: FC<IProps> = (props: IProps) => {
const favouriteMessagesPanelRef = useRef<ScrollPanel>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also solve the TS error:

Suggested change
const favouriteMessagesPanelRef = useRef<ScrollPanel>();
const favouriteMessagesPanelRef = useRef<ScrollPanel | null>(null);

* new instance, anyone who is listening to a different instane will not
* be notified about changes to this instance.
*/
public static get instance(): FavouriteMessagesStore {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new store. Can we directly make it available via the SdkContext and get rid of the static instance here?

content: IContent;
}

interface IState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface IState {
interface State {

as per new code style

@weeman1337
Copy link
Contributor

weeman1337 commented Jan 20, 2023

This is the last one for today. And maybe I am too picky here, but it hurts me 🙃 The header design for favourite message is not the same as for rooms:

header

@andybalaam
Copy link
Member Author

This is the last one for today. And maybe I am too picky here, but it hurts me upside_down_face The header design for favourite message is not the same as for rooms:

Not too picky! Well spotted.

@andybalaam
Copy link
Member Author

This is all awesome feedback - thank you @weeman1337 @florianduros @bwindels !

I was hoping to squeeze the work to get this PR into shape in between other things, and given how much work there is left to do, I don't think I can do that right now :-(

So, any of the following would be much appreciated:

  • Help with fixing some of the above comments
  • Suggestions for how to break this down into more manageable pieces so at least some of it can be merged

Thanks!

@t3chguy t3chguy removed their request for review April 27, 2023 10:12
@andybalaam
Copy link
Member Author

Not currently being worked on, so moving to draft.

@andybalaam andybalaam marked this pull request as draft September 6, 2023 07:31
@langleyd langleyd closed this Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants