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

Reply shows up in thread #19678

Closed
babolivier opened this issue Nov 10, 2021 · 12 comments · Fixed by matrix-org/matrix-js-sdk#2305
Closed

Reply shows up in thread #19678

babolivier opened this issue Nov 10, 2021 · 12 comments · Fixed by matrix-org/matrix-js-sdk#2305
Assignees
Labels
A-Threads 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 Z-Labs

Comments

@babolivier
Copy link
Contributor

A reply was sent to a message that someone else created a thread from. Here's the reply's source:

{
  "content": {
    "body": "> <@andrewm:amorgan.xyz> That's pretty cool! A class around shell scripting specifically sounds really useful. Usually people just have to (barely) learn about that on the spot out in the field from what I've experienced.\n\nthe first rule of shell scripting.. don't use shell scripting ;) portability is awful (flags vary on even common tools like `grep`), the programming defaults are often awful (you should generally always be using `#!/bin/bash -euo pipefail` unless you're damn sure otherwise) and escaping things is a complete pain in the ass",
    "format": "org.matrix.custom.html",
    "formatted_body": "<mx-reply><blockquote><a href=\"https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$cSP4DENqGsF9JWBIFjano09veTj_6OA6F6xgy0Y2WuQ?via=matrix.org&via=vector.modular.im&via=jki.re\">In reply to</a> <a href=\"https://matrix.to/#/@andrewm:amorgan.xyz\">@andrewm:amorgan.xyz</a><br>That&#39;s pretty cool! A class around shell scripting specifically sounds really useful. Usually people just have to (barely) learn about that on the spot out in the field from what I&#39;ve experienced.</blockquote></mx-reply>the first rule of shell scripting.. don't use shell scripting ;) portability is awful (flags vary on even common tools like <code>grep</code>), the programming defaults are often awful (you should generally always be using <code>#!/bin/bash -euo pipefail</code> unless you're damn sure otherwise) and escaping things is a complete pain in the ass",
    "m.relates_to": {
      "m.in_reply_to": {
        "event_id": "$cSP4DENqGsF9JWBIFjano09veTj_6OA6F6xgy0Y2WuQ"
      }
    },
    "msgtype": "m.text"
  },
  "origin_server_ts": 1636540473839,
  "sender": "@kegan:matrix.org",
  "type": "m.room.message",
  "unsigned": {
    "age": 671
  },
  "event_id": "$9sgPalmGeXLTVO9fxcABe8LSTvnr8VtoLEKtrV_uj5s",
  "room_id": "!SGNQGPGUwtcPBUotTL:matrix.org"
}

However, Element web shows it in the thread:

image

Worth nothing that it seems to have caused a bit of a mess because there's a message sent in the thread (the one actually sent in the thread) between anoa's first two messages that apparently went missing.

Element version: 28f0049-react-71d894664113-js-b07d44a6c057
Olm version: 3.2.3

@babolivier
Copy link
Contributor Author

For additional context, it indeed looks like a bug as Kegan then said in the room he's not using threads.

@anoadragon453
Copy link
Member

anoadragon453 commented Nov 10, 2021

Yes, Kegan then sent another message which was related to the reply, but because it itself wasn't directly a reply, it wasn't contained in the thread. So now conversation was split between the thread and the rest of the room.

image

I am using Element stable version: 1.9.3, Olm version: 3.2.3 on Arch Linux.

@kegsay
Copy link
Contributor

kegsay commented Nov 10, 2021

Element version: 1.9.2
Olm version: 3.2.3

on my end, Mac.

@novocaine novocaine added A-Threads T-Defect 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 Nov 10, 2021
@germain-gg
Copy link
Contributor

Having the reply showing up in the thread is the expected behaviour.

For a message to be included in the thread from a client that does not support threads, it is essential to hit "reply" on a message that is part of the thread.
Looking at the event source attached, it is all working as expect (minus the UI defect of how the reply chain is displayed).

Yes, Kegan then sent another message which was related to the reply, but because it itself wasn't directly a reply, it wasn't contained in the thread. So now conversation was split between the thread and the rest of the room.

This is a known short coming, and there are not a lot of ways around that. We expect Threads to be a big enough feature that a lot of clients in the ecosystem will want to support that fairly promptly.
If you have ideas on how to mitigate this, I'd love to hear about them

@kegsay
Copy link
Contributor

kegsay commented Nov 11, 2021

Having the reply showing up in the thread is the expected behaviour.

Were there any UX stories done for this, or was this just decided that due to the semantics of in reply to being similar to threads, to have this form of fallback? The current expected behaviour is quite confusing and seems to fragment clients, enough to cause people (in this case Erik) to comment on it. I don't think we have the luxury to say:

We expect Threads to be a big enough feature that a lot of clients in the ecosystem will want to support that fairly promptly.

as it's non-trivial to implement the UI correctly for threads, especially under challenging environments (e.g terminals).

@germain-gg
Copy link
Contributor

Were there any UX stories done for this, or was this just decided that due to the semantics of in reply to being similar to threads, to have this form of fallback?

We have discussed this within the team and found that this was the most sensible approach. Jano has been doing some UX research in all of this yes.

I do understand how this can be jarring, and would be keen to hear some ideas or suggestions on how this could be approached otherwise. You need some sort of signal that a message should be included in a thread when that event comes from a client that does not support threads. Replies seems like a natural signal that is widely available in the ecosystem. But the second event you sent was lacking any signaling, so I am not sure how a client would decide to render that event in a thread or not if that's what you're hinting at

@kegsay
Copy link
Contributor

kegsay commented Nov 11, 2021

I don't think any such signal will exist as it's entirely a social problem unfortunately. I would personally prefer it if clients didn't guess that I want to start/reply on a thread when my client has no concept of it. This would keep the existing user expectations and allow a proper UX when clients do support threading. If threading is widely adopted quickly then the ecosystem will be full with threads soon anyway, so to have this fallback seems contradictory to the expectations of levels of adoption imo.

@germain-gg
Copy link
Contributor

It's good feedback thanks,

It would be interesting to capture this conversation and your expectations as this is something that I touched on in the MSC. https://github.com/matrix-org/matrix-doc/pull/3440/files#diff-113727ce0257b4dc0ad6f1087b6402f2cfcb6ff93272757b947bf1ce444056aeR195

@germain-gg
Copy link
Contributor

I am re-reading this conversation, @babolivier and @anoadragon453 I'd be interested in hearing what a good resolution would look like for you here?

This is working as intended, and the split conversation between is a known limitation that we could not find a better solution for given the discrepency of Element versions out there

@HarHarLinks

This comment was marked as off-topic.

@t3chguy t3chguy added the X-Needs-Product More input needed from the Product team label Mar 4, 2022
@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2022

@HarHarLinks what you described is different and has been fixed since.

@t3chguy t3chguy self-assigned this Apr 19, 2022
@t3chguy
Copy link
Member

t3chguy commented Apr 19, 2022

This changed in the MSC since the time of the responses

A rich reply without rel_type: m.thread targeting a thread relation must be rendered in the main timeline. This will allow users to advertise threaded messages in the room.

@t3chguy t3chguy removed the X-Needs-Product More input needed from the Product team label Apr 19, 2022
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue May 10, 2022
* Live location sharing: handle encrypted messages in processBeaconEvents ([\matrix-org#2327](matrix-org#2327)).
* Fix race conditions around threads ([\matrix-org#2331](matrix-org#2331)). Fixes element-hq/element-web#21627.
* Ignore m.replace relations on state events, they're invalid ([\matrix-org#2306](matrix-org#2306)). Fixes element-hq/element-web#21851.
* fix example in readme ([\matrix-org#2315](matrix-org#2315)).
* Don't decrement the length count of a thread when root redacted ([\matrix-org#2314](matrix-org#2314)).
* Prevent attempt to create thread with id "undefined" ([\matrix-org#2308](matrix-org#2308)).
* Update threads handling for replies-to-thread-responses as per MSC update ([\matrix-org#2305](matrix-org#2305)). Fixes element-hq/element-web#19678.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Threads 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 Z-Labs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants