-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
[feature] Ephemeral messages (MSC2228) partial support #660
[feature] Ephemeral messages (MSC2228) partial support #660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some awesome work! One minor code-related question.
I was also wondering about the placement of the button/link to access the dialogue. Placing it down by the composer feels more natural IMO; perhaps as a timer icon next to the send button (very explicit) or as an option on holding down the send button for instance (less explicit, probably preferable given limited composer realestate, though that is currently reserved for SMS I believe)? Bear in mind though, that I'm no designer :P
(I realise this is still a draft but I figured it's best to suggest design changes early)
😁 Thank you
Funnily enough, this was my first instinct, too. I decided against it in the end, because I don't forsee needing to change it regularly enough to remove that click and clutter the bar. In e.g. Signal I just set and forget (apart from very odd occasions!) because it's as much housekeeping as anything. Bear in mind, this is an unstable feature which is "hidden" in the Advanced Settings in the first place. I also think this is the first client implementation... |
Yeah, that's reasonable. I'd say it's definitely fine to keep it there while it's unstable then, and possibly leave it there if we don't get too many questions about where it is once it becomes stable.
I think so too; part of the reason I'm so impressed at how fast we're going :P |
Move fast and... er... implement things :D |
Awesome work here! 🚀 Let's change the name to Disappearing Messages in the UX, though we can continue to reference it as Ephemeral in the code since that's what the spec has. When it doubt / as a general rule in Syphon, prefer on using Plain Language in the UX as much as possible even when it defies what Matrix names something. |
Thank you 😊 I understand the need for simple language, but I'm just a little hesitant that people will assign more "clout" to it being called that than the current which is, basically, asking pretty please to servers & clients. I'm thinking I'll create a dialog which goes into the details of the limitations, and I'll be happier with changing the name... Just to be clear, I'm particularly skittish on this one, purely because we're the first implementation AFAIK |
That makes sense and I'm willing to keep it ephemeral, but I think the dialog you mentioned is a good idea. If there's a confirmation dialog after attempting to toggle it in advanced settings describing what it is and isn't for now, I think it will temper peoples expectations and we could call it Disappearing Messages with the understanding that it takes several parties respecting it. I'll try to push the "Advanced Settings" toggle out to dev today to better isolate these features from the typical user. |
@ereio if you could give me some guidance on how I can use the async method for checking server support I'd appreciate it. It was simple for the Hidden Messages because changing the value is an async operation there in the first place... |
Sorry in advance if I'm misunderstanding the ask here. You should be able to use the async / future based check you have here in either of the following:
otherwise
Lmk if that answers your question. Happy to review this a bit more and add comments to guide you later today too! |
I think it does 😅 Any code comments are more than welcome. I'd rather get this right 🙂 The support is per-homeserver rather than room. Right now, I'm planning to store the wanted per-message expiration millis in the |
I'm wondering if we're going to need something like the Mute Handler to expire messages in the client without manually refreshing rooms... Don't think we can put it in the |
30c0e0c
to
5821406
Compare
It occurs to me that we could even check the participating homeservers in a given room for their support, too. That way we can be even more explicit about support in a given room... |
Great idea, but I don't think it's practical with the current state of the spec:
So, while I would love to have something like that, I think we'd have to write an MSC for it first (something like "query server capabilities over federation") which solves at least some of these issues. On a separate note, I was wondering how well this works with MSC2285 (Hidden read receipts), because MSC2228 (Self destructing events) depends on read receipts to redact events after they are viewed. It seems neither MSC mentions the other which makes it difficult to see how they interact. Should I comment on one of them for clarification? |
Damn. You're quite right, of course!
That's an interesting point, given the refactoring has split the endpoint off for hidden RRs. There's going to have to be a lot of testing on all of this... |
Another thing to consider, as suggested by @notramo in the Dev #, is not hiding the toggle but throwing a "your homeserver does not support this" dialog when trying to click but it's... well... unsupported by your HS |
I definitely see the upside. Just hiding the feature can cause confusion if one user can see features and another can't. This would also stop feature requests from users who can't find the feature because their HS does not support it. On the other hand, lots of disabled features clutter the UI. I don't have any strong preference personally. We could make it a setting but having settings for settings feels like a bit much :P |
Well @ereio is moving |
Looks good! I like it :D |
Unless I'm losing the plot, there's actually no way to interrogate the homeserver as to whether or not it has the *The other option is to copy the test case of trying to expire a message in the past and see whether the hs honours it, but I can't think of a "nice" way to implement the failure case |
5983303
to
cb1e691
Compare
@ereio I've reverted the stuff to do with the |
possibly broken in the reverted commits
And now I'm back to chasing my tail again. On my local branch I've started adding |
We should probably get upstream exposing it @ {"capabilities":{"m.room_versions":{"default":"9","available":{"1":"stable","2":"stable","3":"stable","4":"stable","5":"stable","6":"stable","org.matrix.msc2176":"unstable","7":"stable","8":"stable","9":"stable","org.matrix.msc2716v3":"unstable"},"org.matrix.msc3244.room_capabilities":{"knock":{"preferred":"7","support":["7","8","9","org.matrix.msc2716v3"]},"restricted":{"preferred":"9","support":["8","9"]}}},"m.change_password":{"enabled":true},"m.set_displayname":{"enabled":true},"m.set_avatar_url":{"enabled":true},"m.3pid_changes":{"enabled":true}}} |
matrix-org/synapse#12524 (comment)
|
Unless and until someone has the bandwidth to deal with the verdict from the Matrix team, with immense sadness I'm going to close this attempt. |
Types
Changes
🔮 Features
Add support for synapse's partial MSC2228 implementation
🐛 Fixes
🔒 Security
🛠 Performance
📐 Refactoring
Media (if applicable)
QA
Final Checklist