Add rich reply support #150
Conversation
|
Hi Visuwesh, Thanks, this looks good. Please rebase the branch on current master. Then I'll plan to merge this for the next release. |
* ement-room.el (ement-room--rich-reply-callback): Function to modify the message event when reply event is finally fetched. (ement-room--rich-reply-text, ement-room--rich-reply-html): Helper functions for formatting body text with no reply message. (ement-room--format-message-body): Use above to support rich replies. Closes alphapapa#57. Ref. https://spec.matrix.org/v1.4/client-server-api/#rich-replies
9viz
force-pushed
the
feature/rich-replies
branch
from
fd28166
to
6976137
Compare
|
Oops, forgot that you prefer rebase over merge, now done. |
|
Hi @Vizs, I pushed another commit to your branch that makes some minor changes. It also adds two FIXMEs that should probably be addressed before merging. Please review my changes and let me know what you think. Thanks. |
alphapapa
force-pushed
the
feature/rich-replies
branch
2 times, most recently
from
5069efd
to
e4e71ad
Compare
|
Rest of the changes look fine to me, thanks for the cleanup! |
|
@Vizs Thanks. Did you see the two FIXME comments? How do you think we should handle them? Also, do you know of a good way to test this new code? e.g. could you set up a room with a few "rich reply" events that we could view in Ement to ensure that it works correctly? |
|
(Did my "review comments" not go through?) I say we leave the first FIXME in until people start to complain. I haven't encountered the situation you highlight in my testing period and I encounter rich-replies practically all the time when I use matrix since the t2bot's discord bridge uses rich replies. As for the second FIXME, what do you think about the following function instead? As for a room with rich replies, I invited you to one named "testytestytest". Please have a look at message events after "TEST FOR RICH REPLY". I used the following patch to make ement.el produce rich-replies, |
I can't find any. Maybe you forgot to press the "Finish review" button. I've done that before, because the UI is non-obvious there. |
|
Thanks, I agree with those solutions. One more thing: I think we should have an option to make Ement send rich replies too. Would you be interested in adding that feature as well? |
* ement-room.el (ement-room-send-rich-replies): Add user option to control sending rich replies. (ement-room-write-reply): Adjust to use above. * ement-lib.el (ement-send-message): Change to send rich replies.
|
I pushed two more commits to quote prefix each line in the reply, and also a new user option to send rich replies.
Indeed, looks like I didn't do it. But anyway, all my comments are addressed now. |
alphapapa
force-pushed
the
feature/rich-replies
branch
2 times, most recently
from
2c23e7e
to
18a3035
Compare
* ement-room.el (ement-room--rich-reply-callback): Function to modify the message event when reply event is finally fetched. (ement-room--rich-reply-text, ement-room--rich-reply-html): Helper functions for formatting body text with no reply message. (ement-room--format-message-body): Use above to support rich replies. Closes alphapapa#57. Ref. https://spec.matrix.org/v1.4/client-server-api/#rich-replies
* ement-room.el (ement-room-send-rich-replies): Add user option to control sending rich replies. (ement-room-write-reply): Adjust to use above. * ement-lib.el (ement-send-message): Change to send rich replies.
alphapapa
force-pushed
the
feature/rich-replies
branch
from
befb23c
to
ca866fe
Compare
|
Rebased on master. |
…s' into feature/rich-replies
|
I was just chatting with a friend and he noticed he couldn't see a couple of my replies in Element on Android: But renders correctly on Element Desktop, and every other client we tried, Element X Android, FluffyChat Android, and of course Ement.el: Maybe it's an issue on the Element side? |
Yes, "Element Android" has known problems with rendering of non-plain-text messages. They will only be resolved by switching to Element X. However, with regard to this PR, we should ensure that we're sending events according to the spec. |
* ement-lib.el (ement-send-message): The spec asks to include fallback quotes even if rich replies are being used.
* ement-room.el (ement-room--format-message-body) (ement-room--format-quotation-html): Strip off fallback quotes from HTML body, and use the quoted event's body always. Currently, stripping off is not done for plain text bodies since it is fragile: it is not a given that all clients will follow the spec for the fallback quote in plain text bodies. So more harm than good will be done by a poor attempt at stripping fallback quotes.
* ement-room.el (ement-room-send-rich-replies): * ement-lib.el (ement-room-send-rich-replies, ement-send-message): Always send rich replies.
This is now done. I removed the defcustom and now unconditonally send rich replies. I don't think there's any harm here.
This is partially done. I only do it for HTML bodies since plain text bodies tend to be fragile: if the client doesn't follow the spec word-to-word, then the quoted part will not be in the format given in the spec which will need some complex machinery to check if the quote is actually in the body... this sounds brittle to me so I would rather not attempt at stripping fallback quotes for plain text bodies (and anyway, apart from me, I don't know of any other user who uses plain text bodies). TBH I disagree with the spec here. They make the quote unhelpful: it is hard already to figure out which image is being replied to. If the quote just says " sent an image," that is unhelpful. So I am leaving the multi-media ones aside. As for the rest, I would have to look closer.
I hope the changes I pushed now are good. If you have any further comments, I will answer them in May (semester finals from next week :)). |
|
Editing a reply still breaks everything though. See the message https://matrix.to/#/!QdMjOBGcNMjmTPvAAS:matrix.org/$17134583412681GagSj:matrix.org?via=matrix.org&via=bpulse.org&via=disrule.us in Element. This seems to be due to us not setting m.new_content's body correctly. Not sure if this falls under the radar of this PR. EDIT2: Edits done in Element is not rendered properly in Ement 🙃 Ement loses the quotation because we only look at m.new_content's body whereas it seems to me that we need to try harder. EDIT3: Ugghhh this happens because the edit event yields nil replied-to-event-id so we don't go in the rich-reply path. Looks like we need some kind of deep traversal for certain event types? I will leave this to the future me to deal with. EDIT4: This is fixed (though I messed up the commit message badly :( it is 11:30 pm, I should sleep). However, this doesn't include edits to the message being replied to in the quotation... I have no idea how to get edits of a given event so this will have be left unsolved for a while. |
If message A replies to message R, and message A is edited to message B, now correctly include message R in message B too. * ement-room.el (ement-room--format-message-body): Check for original event's m.in_reply_to event ID for potential quotation. (ement-room--rich-reply-callback): Factor out the callback function again.
I'm not sure if it's relevant or not (I haven't tried to understand this feature), but |
|
The "return an ersatz one that has the expected ID and same sender" makes it a bit hard to use since the dummy one won't have the body required to form the quotation. If it returns nil if the event is not already fetched, that would make it employable here. |
|
I just ran into the "Ement loses the quotation because we only look at m.new_content's body" scenario, and remembered this MR. 9viz: If you could rebase this over master, I'd be happy to start using it if it's ready for wider testing? |
|
@phil-s can you try the feature/rich-replies-rebase branch in my fork [1]? I'm not too confident that I actually did the rebase properly so I haven't pushed to the feature/rich-replies branch. |
|
Hi @9viz. Thanks for your work on this, and sorry for the slow reply.
Because of that I decided to perform the rebase independently to see whether or not we got the same result, and after a few conflict resolutions I found myself dealing with a commit I'd already dealt with, and then I realised why there were so many commits to be rebased... I'm afraid the git history for If I can give you some homework, it would be to spend some time making sure you understand in detail what rebasing and merging each do to a git history, until you're confident that you understand how your branch In particular, you want to have a good understanding of what was going on in the two merge commits Your
The critical thing is to never blindly merge a branch back into itself (or at least not unless that is specifically the result you are wanting, but I would consider that very unusual). Always check the result after merging or rebasing, and make sure you don't have a graph showing something more complicated than you expect. Here we really want to end up with a rebase that looks like this: I.e. one version of each commit. That's what I've pushed to https:/phil-s/ement.el/commits/phil-s/rich-replies/ after working through all of that last night, but I've done no testing, and I am not completely confident about all changes. The code differences between our rebases are as follows, and need to be reviewed. (I'll take another look at it myself, but not this weekend.) 2 files changed, 10 insertions(+), 9 deletions(-)
ement-lib.el | 9 ++++-----
ement-room.el | 10 ++++++----
modified ement-lib.el
@@ -32,7 +32,7 @@
(require 'ewoc)
(require 'pcase)
(require 'subr-x)
-
+
(require 'taxy-magit-section)
(require 'ement-macros))
@@ -1143,10 +1143,9 @@ ement-send-message
(setf content (funcall filter content room)))
(when replying-to-event
(setf replying-to-event (ement--original-event-for replying-to-event session))
- (when ement-room-send-rich-replies
- ;; TODO: Submit a patch to Emacs to enable `map-nested-elt' to work as a generalized variable.
- (setf (map-elt (map-elt (map-elt content 'm.relates_to) 'm.in_reply_to) 'event_id)
- (ement-event-id replying-to-event)))
+ ;; TODO: Submit a patch to Emacs to enable `map-nested-elt' to work as a generalized variable.
+ (setf (map-elt (map-elt (map-elt content 'm.relates_to) 'm.in_reply_to) 'event_id)
+ (ement-event-id replying-to-event))
(setf content (ement--add-reply content replying-to-event room)))
(ement-api session endpoint :method 'put :data (json-encode content)
:then (apply-partially then :room room :session session
modified ement-room.el
@@ -2256,14 +2256,16 @@ ement-room-write-reply
(ement-room-with-highlighted-event-at (point)
(pcase-let* ((room ement-room)
(session ement-session)
+ (prompt (format "Send reply (%s): " (ement-room-display-name room)))
+ (ement-room-read-string-setup-hook
(lambda ()
(setq-local ement-room-replying-to-event event)))
(body (ement-room-with-typing
(ement-room-read-string prompt nil 'ement-room-message-history
- nil 'inherit-input-method)))
- (replying-to-event (ement--original-event-for event ement-session)))
- (ement-room-send-message room session :body body :replying-to-event replying-to-event
- :rich-reply ement-room-send-rich-replies))))
+ nil 'inherit-input-method))))
+ ;; NOTE: `ement-room-send-message' looks up the original event, so we pass `event'
+ ;; as :replying-to-event.
+ (ement-room-send-message room session :body body :replying-to-event event)))))
(when (assoc "emoji" input-method-alist)
(defun ement-room-use-emoji-input-method () |
This PR fixes #57. I posted a preliminary patch in #57 and daily drove it ever since I posted it. Although I haven't used matrix much these past three--four weeks, I tested this current PR with both plain text and HTML body rendering. AFAIK I haven't broke anything, yet.
Hoping to hear from you,