Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"$foo mentioned you" notifications generally lie. #2397

Closed
ara4n opened this issue Jan 27, 2024 · 12 comments · Fixed by #3142
Closed

"$foo mentioned you" notifications generally lie. #2397

ara4n opened this issue Jan 27, 2024 · 12 comments · Fixed by #3142
Assignees
Labels
A-Notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Blocked X-Needs-Design X-Needs-Product

Comments

@ara4n
Copy link
Member

ara4n commented Jan 27, 2024

Steps to reproduce

  1. You receive a notification saying "$foo mentioned you: $msg"
  2. You wince because typically $foo didn't mention you. instead they either:
  • replied to a message you sent
  • used @room
  • or otherwise didn't actually intentionally mention you.

Outcome

What did you expect?

imo, notifications shouldn't waste space with "$foo mentioned you" at all. If we really want do to this, they certainly shouldn't include "$foo mentioned you" unless $foo really did intentionally mention you.

What happened instead?

All sorts of things seem to trigger this. I can't actually remember seeing this notification when someone actually intentionally mentioned me, in fact.

Your phone model

No response

Operating system version

No response

Application version

508

Homeserver

No response

Will you send logs?

No

@ara4n ara4n added the T-Defect label Jan 27, 2024
@Velin92 Velin92 added X-Needs-Rust This issue needs a Rust SDK change. It must have a link to a Rust SDK issue X-Needs-Design X-Needs-Product S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Occasional Affects or can be seen by some users regularly or most users rarely labels Feb 2, 2024
@Velin92
Copy link
Member

Velin92 commented Feb 2, 2024

So the hasMention check coming from the SDK notification obeys to some push rules that recognise as a mention also non intentional ones (if they are not supported from the sender), replies and room mentions. Probably we want at least for the reply case to distinguish between them.
However maybe is worth also discussing from a design or product point of view if we want to keep the additional copy or change it, or what should be intended behaviour for replies or for all room mentions.
@VolkerJunginger

@jmartinesp
Copy link
Member

jmartinesp commented Jul 9, 2024

I see we never made a decision. Should we remove the different text or try to detect intentional mentions only?

@manuroe
Copy link
Member

manuroe commented Jul 9, 2024

What will happen if we remove false positives for intentional mentions and fix the computation of hasMention in the SDK?

@jmartinesp
Copy link
Member

Then it should be working as expected: replies won't say you've been mentioned, and neither will messages with @room. Non-intentional mentions however, won't say you've been mentioned, but I think that's expected.

@manuroe
Copy link
Member

manuroe commented Jul 9, 2024

yes, this is right.

@jmartinesp
Copy link
Member

jmartinesp commented Jul 10, 2024

So we discussed this with several devs in the Rust SDK Development room and it seems like there's no good way to distinguish between mentions and replies given the current state of the spec:

  1. We have intentional mentions, but the spec says for backwards compatibility clients should add the sender of the replied to message as an intentional mention when replying. There's no way to distinguish between a reply to you that mentions another user and a reply to other user that mentions you given the current data.
  2. There is this MSC, and either the proposed implementation or the alternative one would help us with this problem.
  3. If we don't follow the spec's suggestion (as it's just a suggestion) other clients may have unexpected behaviour and we'd still display 'mentioned you' for messages of users who replied to some message of yours in one of those clients.
  4. We could technically try to check the message contents for actual mentions, but that's a 🐰 hole I'd rather not jump into as it has many possible edge cases.

@manuroe
Copy link
Member

manuroe commented Jul 10, 2024

To illustrate it, below is how a notification for a reply looks like on EW on EXI

EW notification

image

EXI / EXA notification

IMG_1484

EXI timeline

For reference, the actual message is displayed like this in the EX timeline:
image

@manuroe manuroe removed the X-Needs-Rust This issue needs a Rust SDK change. It must have a link to a Rust SDK issue label Jul 11, 2024
@manuroe manuroe assigned amshakal and unassigned jmartinesp Jul 11, 2024
@manuroe
Copy link
Member

manuroe commented Jul 11, 2024

@amshakal , I assigned this to you as it seems we are going to review the copy instead of doing a MSC.

@amshakal
Copy link

Makes sense to change the copy. Can we have different copy for different scenarios? One for replies and one for @room? Is there a third scenario?

@ara4n
Copy link
Member Author

ara4n commented Aug 30, 2024

the current behaviour is now to say "Foo mentioned or replied to you" on these msgs, given we can't differentiate them otherwise. This is technically correct, but exposes the bug that we can't differentiate them, which is just really weird UX.

i suggest we remove the "mentioned or replied" entirely and just use the name until we have fixed the API to be able to distinguish between them.

cc @amshakal

@ara4n ara4n reopened this Aug 30, 2024
@mxandreas
Copy link

This is technically correct, but exposes the bug that we can't differentiate them, which is just really weird UX.

Agreed.

However, this was known to be workaround, not the desired behaviour (which is much more costly). The small upside of the current message is that it is clear (e.g. you know at least the source(s) for these notifications). I would say that if the desired behavior is critical (e.g. causing significant damage), then we should prioritize implementing that solution, instead of fine-tuning the workaround.

@manuroe
Copy link
Member

manuroe commented Sep 4, 2024

I am closing this issue to faciliate the project management for the matrix conf as the current solution is good enough for the moment. The next iteration will happen in element-hq/element-meta#2520.

@manuroe manuroe closed this as completed Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Blocked X-Needs-Design X-Needs-Product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants