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

slash command to update ignored rooms list #8983

Closed
wants to merge 3 commits into from

Conversation

Gnuxie
Copy link
Contributor

@Gnuxie Gnuxie commented Jul 4, 2022

Based on matrix-org/matrix-js-sdk#2483
Doesn't actually ignore anything, just updates the list in the above PR


This PR currently has none of the required changelog labels.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Based on matrix-org/matrix-js-sdk#2483
Doens't actually ignore anything yet though...
Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

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

I think that the syntax should be either

/ignore-invite

or

/ignore invites

Otherwise, it's going to be very confusing, since /ignore is used for ignoring users entirely.

Also, as you probably know, I cannot approve patches on matrix-react-sdk, so you'll have to find someone from that team to do it once you're ready!

@@ -428,6 +428,8 @@
"Stops ignoring a user, showing their messages going forward": "Stops ignoring a user, showing their messages going forward",
"Unignored user": "Unignored user",
"You are no longer ignoring %(userId)s": "You are no longer ignoring %(userId)s",
"Ignored room": "Ignored room",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we're not ignoring the room, only the invites from this room!

Copy link
Contributor

Choose a reason for hiding this comment

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

...actually, you are right, we may be ignoring the room entirely. But to do so, the slash command needs to leave the room immediately, no?

src/SlashCommands.tsx Outdated Show resolved Hide resolved
src/SlashCommands.tsx Outdated Show resolved Hide resolved
@@ -815,6 +815,28 @@ export const Commands = [
}),
);
}
const roomMatches = args.match(/^([!][^:]+:\S+)$/);
if (roomMatches) {
Copy link
Contributor

@Yoric Yoric Jul 5, 2022

Choose a reason for hiding this comment

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

Our main usecase is ignoring the current room, so we should support a syntax such as

/ignore-invites room

(where room is an actual keyword) to ignore invites to the current room.

Feel free to improve that syntax :)

Copy link
Member

Choose a reason for hiding this comment

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

But you can't write slash commands in a room in which you are holding an invite, you'd need to join the room to the ignore invites and then leave? That doesn't sound safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, user feedback suggests that this is actually a common behavior:

  1. Get tricked into a room.
  2. Realize that you have been tricked.
  3. Decide never to be invited in this room again.
  4. ...but we don't have tools for that yet.

We definitely want to add the ability to ignore invites before joining the room, but we're planning to work on that as a later stage, as this will require deeper changes in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

But if you leave before you realise "oh maybe I should ignore future invites" then it'll be too late, you'll have to wait for a second invite, accept it, then finally you can ignore it.

Copy link
Contributor

@Yoric Yoric Jul 5, 2022

Choose a reason for hiding this comment

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

Absolutely.

Right now, we're trying to reach the following objectives:

  • handle at least one common case with minimal UI, hoping to get to a stage where we can actually put it in the hands of power users for testing;
  • lay the foundations for handling the other cases once we have a proper UI design (which we're working on in the background, but with no ETA as of yet).

The command is much the same but the actual implementation
matrix-org/matrix-js-sdk#2496 will now only
hide invitations.

We now have a command to unignore invitations from a room.
@Yoric
Copy link
Contributor

Yoric commented Jul 7, 2022

@Gnuxie Heads up: the type of getIgnoredInvites/setIgnoredInvites is changing.

if (Boolean(targetRoomId)) {
const ignoredInvites = cli.getIgnoredInvites();
if (ignoredInvites.ignored_rooms === undefined) {
ignoredInvites.ignored_rooms = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest version of the PR, ignored_rooms has become readonly.

ignoredInvites.ignored_rooms = [];
}
const isAlreadyIgnored = Boolean(ignoredInvites.ignored_rooms
.find(ignoredRoom => ignoredRoom.room_id === targetRoomId));
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest version of the PR, this is an object, rather than an array.

if (!isAlreadyIgnored) {
ignoredInvites.ignored_rooms.push({
room_id: targetRoomId,
ts: Date.now(), // TODO: Check this is the timestamp we want?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is.

if (roomMatches) {
const roomId = roomMatches[1];
const ignoredInvites = cli.getIgnoredInvites();
if (ignoredInvites.ignored_rooms === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remarks.

@Yoric Yoric mentioned this pull request Sep 7, 2022
3 tasks
@andybalaam
Copy link
Member

@Gnuxie is this abandoned? Will close if so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants