alphapapa
Add rich reply support
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,
Hi Visuwesh,
Thanks, this looks good. Please rebase the branch on current master. Then I'll plan to merge this for the next release.
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.
@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?
(defun ement-room--format-quotation-text (quoted-event)
"Return text for QUOTED-EVENT."
(pcase-let* (((cl-struct ement-event sender (content (map body))) quoted-event)
((cl-struct ement-user (id sender-id)) sender))
(with-temp-buffer
(insert "> " "<" sender-id ">" body)
(goto-char (point-min))
(forward-line 1)
(while (not (eobp))
(insert "> ")
(forward-line 1))
(insert "\n")
(buffer-string))))
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,
diff --git a/ement-lib.el b/ement-lib.el
index 06c98ab..74b93fd 100644
--- a/ement-lib.el
+++ b/ement-lib.el
@@ -1092,7 +1092,10 @@ e.g. `ement-room-send-org-filter')."
(when filter
(setf content (funcall filter content room)))
(when replying-to-event
- (setf content (ement--add-reply content replying-to-event room)))
+ (setq content (cons `(m.relates_to (m.in_reply_to (event_id . ,(ement-event-id replying-to-event))))
+ content))
+ ;;(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
;; Data is added when calling back.
(Did my "review comments" not go through?)
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?
I pushed two more commits to quote prefix each line in the reply, and also a new user option to send 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.
Indeed, looks like I didn't do it. But anyway, all my comments are addressed now.
@vizs I rebased this branch onto master and pushed some small changes on top, including linting with makem.sh again. Please review the new commits I pushed and the current diff against master and let me know what you think. If it looks good to you, I'll merge to master and we can start testing it "in anger" in preparation for the next version release. Thanks.
Thanks, everything looks good! As for the FIXME you added,
diff --git a/ement-room.el b/ement-room.el
index 08d79db..8060ee6 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -3457,6 +3457,7 @@ If FORMATTED-P, return the formatted body content, when available."
:then (let ((room ement-room)
(session ement-session))
(lambda (fetched-event)
+ ;; FIXME: Do we need to use `when'? Shouldn't `fetched-event' always be present?
(when fetched-event
(pcase-let* ((new-event (ement--make-event fetched-event))
((cl-struct ement-room (local (map buffer))) room))
I simply cannot recall why I added the `when', if you want we can drop it and add it back when someone (TM) complains.
I simply cannot recall why I added the `when', if you want we can drop it and add it back when someone (TM) complains.
AFAIK, if this function is called, it means the request succeeded, which should mean that the value will be present; and if the value is nil, that would indicate a problem that we should probably allow to signal an error.
So if you remove the wrapping when form and the code still works, then I think we can safely remove it.
Very quick testing with the when removed tells me that everytrhing works fine, so we can remove it.
@vizs Ok, thanks. I pushed another commit doing that.
However, before merging this, I looked at the spec again: https://spec.matrix.org/v1.3/client-server-api/#rich-replies IIUC, to support it correctly, we also need to:
- [ ] Include fallback quotes. Probably we need to also call
ement--add-replyeven when sending rich replies. (And maybe we should move the setting of the relationship metadata into that function, and do it uncondtionally, maybe even removing the option.) - [ ] Strip fallback quotes from events that include rich replies. We already have the code to fetch the replies and re-render the event.
- [ ] Ensure that our rendered replies also handle message types other than
m.text(as in 1, 2, and 3).
What do you think? Thanks.
Include fallback quotes. Probably we need to also call ement--add-reply even when sending rich replies. (And maybe we should move the setting of the relationship metadata into that function, and do it uncondtionally, maybe even removing the option.)
Oh yes, this was a misunderstanding on my part on how rich replies work. I misread/misunderstood the situation and thought fallback quotes need not be added when you're sending rich-replies since this PR was sparked by messages that did not include fallback quotes. You're right, we need to add fallback quotes unconditionally and only need to add the m.relates_to part if we are concerned with sending rich-replies.
Strip fallback quotes from events that include rich replies. We already have the code to fetch the replies and re-render the event.
As of now, well behaved clients always include the fallback quotes but I don't find it necessary to strip them except for rare cases when clients tend to truncate the reply's body however, stripping should be easy for HTML messages but something deep in me says that plain text bodies will not be fun to handle. I say we don't bother about plain text bodies for now and worry about it later since it is always a second-class citizen in the matrix world anyway. :(
Ensure that our rendered replies also handle message types other than m.text (as in 1, 2, and 3).
Hmm... if we are sure that we don't want to deal with stripping fallback quotes, I think we can live without having support for these just yet.
However, if you think stripping is necessary, I can implement it but unfortunately I am kind of busy these days with a whole load of quizzes and generally busy semester (+ prep for my masters project 🙃).
Ok, let's defer this to v0.13. Since it seems that other clients are either not using rich replies or are always including the inline fallbacks, it doesn't seem urgent to support this, and there are other issues that are more important to solve sooner. Thanks.
I'm really sorry but I don't have the motivation to work on this PR and address the final issues.
You can close this issue if you'd like, or just leave it open in hopes that someone else will take over.
No problem. You've already done valuable work on this. There's no urgency to finishing it. I'll plan to finish it myself when I get time, or maybe you'll change your mind sometime. Thanks.
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?
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.
However, before merging this, I looked at the spec again: https://spec.matrix.org/v1.3/client-server-api/#rich-replies IIUC, to support it correctly, we also need to:
- [ ] Include fallback quotes. Probably we need to also call
ement--add-replyeven when sending rich replies. (And maybe we should move the setting of the relationship metadata into that function, and do it uncondtionally, maybe even removing the option.)
This is now done. I removed the defcustom and now unconditonally send rich replies. I don't think there's any harm here.
- [ ] Strip fallback quotes from events that include rich replies. We already have the code to fetch the replies and re-render the event.
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 "
What do you think? Thanks.
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 :upside_down_face: 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.
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'm not sure if it's relevant or not (I haven't tried to understand this feature), but ement--original-event-for is a relatively recent addition to Ement, and sounds like it might help with that? I've not read your code here, other than to search/confirm that you're not already calling that function.
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.
- https://github.com/9viz/ement.el/tree/feature/rich-replies-rebase
Hi @9viz. Thanks for your work on this, and sorry for the slow reply.
I'm not too confident that I actually did the rebase properly
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 feature/rich-replies is a bit of a mess, due to apparent confusion between merging and rebasing, meaning you ended up with 31 commits to rebase instead of 18, with the additional 13 commits being alternate-history variants of some of the other 18 resulting from occasions in which you merged a different version of this branch back into itself. All of that history for each of the multiple different versions is still in your branch, and it makes rebasing a pain.
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 feature/rich-replies at commit 62244c15535b821b19fa8fd2e4e6a40a845bc02a which was still based on ement version 0.9.3 commit 8b56efa9387262514daf63151d41c9e111e79567 contained all of the following history just for this feature.
* 62244c1 (9viz/feature/rich-replies, rr, feature/rich-replies) Fix: Quote event being replied to in original message for a reply (Visuwesh)
* be6ca18 Tidy: (ement-room--format-quotation-html) Remove debugging marker (Visuwesh)
* 6ec7d31 Change: Always send rich replies, remove ement-room-send-rich-replies (Visuwesh)
* e1b02bc Fix: Strip off fallback quotes from HTML bodies if rich reply event (Visuwesh)
* d6a6bdf Always include fallback quotes regardless of rich reply setting (Visuwesh)
* 3c1b73b Merge remote-tracking branch 'refs/remotes/origin/feature/rich-replies' into feature/rich-replies (Visuwesh)
|\
| * ca866fe Tidy: Remove WHEN wrapper (Adam Porter)
| * 4b89b7c Fix: (ement-room-send-rich-replies) Specify type (Adam Porter)
| * d137617 Tidy: Compiler warning (Adam Porter)
| * 3146201 Tidy: Use ement--put-event (Adam Porter)
| * f0a13cf Comment: Add FIXME (Adam Porter)
| * 787f537 Revert: Whitespace-only changes (Adam Porter)
| * 70b16bd Tidy: Indentation (Adam Porter)
| * 118d000 Change: (ement-room-send-rich-replies) Docstring, add link to spec (Adam Porter)
| * f67698a Change: (ement-send-message) Remove :rich-reply argument (Adam Porter)
| * cc0c97d Add: Support for sending rich replies (Visuwesh)
| * d275349 Change: (ement-room--format-quotation-text) Quote prefix each line (Visuwesh)
| * 6f625b8 Minor refactoring and tidying (Adam Porter)
| * 3581ef5 Add: Support for rich reply support (Visuwesh)
| * 5ef7417 Merge: Improve view-room prompt (Adam Porter)
| |\
| | * cad27f5 Docs: Update changelog (Adam Porter)
| | * e7ca33e Change: (ement-room-browse-url) Use `read-multiple-choice' (Phil Sainty)
| | * 21cc649 Fix: (ement-room-browse-url) Avoid recursive call (Phil Sainty)
| |/
| * 97be65a Meta: v0.15-pre (Adam Porter)
| * 0c725c6 (tag: v0.14) Release: v0.14 (Adam Porter)
| * a38eba1 Comment: Add FIXME (Adam Porter)
| * 08d5dad Merge: (ement-room-send-reaction) Check for event at point (Adam Porter)
| |\
| | * 27828a2 Docs: Update changelog (Adam Porter)
| | * d3154ca Fix: (ement-room-send-reaction) Bail out early when there's no event at point (Phil Sainty)
| | * 72a6399 Fix: (ement-room-with-highlighted-event-at) Use gensyms (Adam Porter)
| |/
| * 71ce17d Tidy (Adam Porter)
| * 2078ad4 Merge: Support for m.audio events (Adam Porter)
| |\
| | * a3cbeea Docs: Update changelog (Adam Porter)
| | * b692c4a Add basic support for m.audio events (Arto Jantunen)
| |/
| * d557519 Fix: (ement-notifications-bookmark-handler) Call command to restore (Adam Porter)
| * 63254f4 Docs: Update changelog (Adam Porter)
| * 2764c10 Merge: Cache room-list visibility concisely (Adam Porter)
| |\
| | * eabb725 (upstream/wip/256-visibility-cache-data) Cache visibility data concisely (Adam Porter)
| |/
| * 4a25cc0 Merge: (ement-room--insert-sender-headers) Refactor/fix (Adam Porter)
| |\
| | * eec3327 (upstream/fix/157) Docs: Update changelog (Adam Porter)
| | * a9d9d23 Tidy: Formatting, comments (Adam Porter)
| | * eed5f27 Tidy: Indentation (Adam Porter)
| | * 24320f2 Fix sender header insertion with respect to membership changes (Steven Allen)
| |/
| * c8fa944 Add: (ement-room-reaction-names-limit) (Adam Porter)
| * 91f8632 Tidy (Adam Porter)
| * 64424c1 Change: (ement-room--format-reactions) Show name if only one sender (Adam Porter)
| * b3a9487 Docs: Fix typos (Jonas Bernoulli)
| * 1579f78 Comment: Add TODO (Adam Porter)
| * e509dc9 Dev: (ement--sync-callback) Add debug call (Adam Porter)
| * 7b78c13 Tidy: Obsolete variable (Adam Porter)
| * 8e6e9cd Change/Fix: Clicking/jumping-to notifications (Adam Porter)
| * 4c87563 Change: (ement-room-mode-map) Bind tab/backtab to move between links (Adam Porter)
| * 26be559 Add: (ement-room-list-space-prefix) (Adam Porter)
| * 9bab00c Add: (defgroup ement-room-list) (Adam Porter)
| * f82d763 Docs: Update readme (Adam Porter)
| * 1dc5727 Meta: (test.yml) Update Emacs version matrix (Adam Porter)
| * 3dde860 Meta: v0.14-pre (Adam Porter)
| * 17ea7f6 (tag: v0.13) Release: v0.13 (Adam Porter)
| * 6b4ce57 Merge: (ement-room-image-show) Use frame parameters (Adam Porter)
| |\
| | * 2cfe8c4 Docs: Update changelog (Adam Porter)
| | * 6621c20 Change: (ement-room-image-show) Pass frame parameters (Nicholas Vollmer)
| |/
| * 9845201 Merge: Fixes for redactions of edited messages (Adam Porter)
| |\
| | * 63f3020 Docs: Update changelog (Adam Porter)
| | * cb24ae6 Fix: (ement-room-delete-message) Redact original messages (Adam Porter)
| | * 5d12e08 Docs: Update changelog (Adam Porter)
| | * 9a4bf42 Fix: ("m.room.redaction") Redacting edited events (Adam Porter)
| |/
| * 84787ed Merge: (ement--original-event-for) (Adam Porter)
| |\
| | * ec518a1 Docs: Update changelog (Adam Porter)
| | * f7e9630 Fix: (ement-room-write-reply) Reply to original event, not an edit (Adam Porter)
| | * bed6e41 Add/Change: (ement--original-event-for) (Adam Porter)
| |/
| * 0fad92d Change: (ement-room-flush-colors) Maintain point position (Adam Porter)
| * 99ee11a Fix: (ement-room-edit-message) Already-edited events (Adam Porter)
| * 688a5ff Meta: (bug_report.yml) Update (Adam Porter)
| * 3dbed13 Comment: Add TODOs (Adam Porter)
| * 52faf4a Merge: Reuse buffers for message formatting (Adam Porter)
| |\
| | * 64972dc (upstream/opt/single-buffer) Change: Disable undo in formatting buffers (Adam Porter)
| | * bb73293 Change: use dedicated buffer for message rendering, formatting (Nicholas Vollmer)
| |/
| * 79b0bd0 Docs: Update changelog (Adam Porter)
| * 0c93e14 Fix: (ement-room-username-display-property) Custom type (Adam Porter)
| * 351aea8 Fix: Writing reply from compose buffer from minibuffer (Adam Porter)
| * 01ec34b Comment: Add TODO (Adam Porter)
| * 9823d52 Comment: Add FIXME (Adam Porter)
| * 225e607 Fix/Tidy: Incorrect use of etypecase (Adam Porter)
| * 68504ed Change: (ement-room-mode-map) Bind "N" to end-of-buffer (Adam Porter)
| * 645537c Add: (ement-directory-define-key people-p) (Adam Porter)
| * 30bd78b Fix: (ement-directory-define-column "Name") Direct rooms (Adam Porter)
| * 886f5e2 Change: (ement-directory-define-column "Size") :align 'right (Adam Porter)
| * 2bfabaf Meta: v0.13-pre (Adam Porter)
| * a4fc3d1 (tag: v0.12) Release: v0.12 (Adam Porter)
| * b88e303 Tidy: Indentation of cl-labels forms (Adam Porter)
| * 8363bfc Add: (ement--savehist-save-hook) Workaround for savehist-mode (Adam Porter)
| * 358b125 Fix: Scroll commands in mentions buffer (Adam Porter)
| * 20f6982 Fix: (ement-notifications-log-to-buffer) Use save-mark-and-excursion (Adam Porter)
| * ea6e577 Fix: (ement-room-send-org-filter) Prevent subscripts (Adam Porter)
| * 726e81f Docs: Tidy changelog (Adam Porter)
| * 0941ff6 Change: (ement-room-goto-next/prev) Make more useful (Adam Porter)
| * 52fc8a4 Change: (defface ement-room-quote) Inherit font-lock-comment-face (Adam Porter)
| * 0632033 Add/Fix: Detection of end of quoted part (Adam Porter)
| * ba9af3a Tidy: Docstring (Adam Porter)
| * 15a07bd Change: (ement-room-define-event-formatter) Add debug form (Adam Porter)
| * 4419261 Change/Fix: (ement--json-parse-buffer) Fall back to json-read (Adam Porter)
| * c9183c0 Comment: Add FIXME (Adam Porter)
| * 8aea26a Add/Change: (ement--json-parse-buffer) Use Jansson functions (Adam Porter)
| * d10ac0a Change: (ement-room-mode-map) A few bindings (Adam Porter)
| * 6508b68 Add/Change: Don't apply body face to quoted parts (Adam Porter)
| * fd1a657 Merge: (ement-notifications) (Adam Porter)
| |\
| | * 1d8afe6 Docs: Update changelog (Adam Porter)
| | * 02f61e5 Comment: Improve commentary (Adam Porter)
| | * 71e628b Comment: Remove FIXME (Adam Porter)
| | * 075920e Comment: Add TODO (Adam Porter)
| | * 83b4c64 Change: Use switch-to-buffer (Adam Porter)
| | * 386103c Fix (Adam Porter)
| | * cca407f Tidy: Compiler warnings (Adam Porter)
| | * a83ab5c Fix (Adam Porter)
| | * 36db186 Change/Fix (Adam Porter)
| | * 4f7ef5d Tidy: Compiler warning (Adam Porter)
| | * 46afa1f Change: Integrate ement-notify and ement-notifications, and retro-load (Adam Porter)
| | * c6331a2 Add: (ement-notifications) (Adam Porter)
| |/
| * 9da7b8e Meta: Add bug report template (Adam Porter)
* | da037e5 Merge remote-tracking branch 'refs/remotes/origin/feature/rich-replies' into feature/rich-replies (Visuwesh)
|\ \
| * | f51f77a Fix: (ement-room-send-rich-replies) Specify type (Adam Porter)
| * | 6cd3178 Tidy: Compiler warning (Adam Porter)
| * | 8683a4e Tidy: Use ement--put-event (Adam Porter)
| * | 42a53e6 Comment: Add FIXME (Adam Porter)
| * | c6279bd Revert: Whitespace-only changes (Adam Porter)
| * | 8c8e7cf Tidy: Indentation (Adam Porter)
| * | 2d8f1a4 Change: (ement-room-send-rich-replies) Docstring, add link to spec (Adam Porter)
| * | 78c4234 Change: (ement-send-message) Remove :rich-reply argument (Adam Porter)
| * | 5d737d8 Add: Support for sending rich replies (Visuwesh)
| * | c8fd52a Change: (ement-room--format-quotation-text) Quote prefix each line (Visuwesh)
| * | 9012da9 Minor refactoring and tidying (Adam Porter)
| * | fad677a Add: Support for rich reply support (Visuwesh)
| |/
| * a3aad83 Meta: Update copyright years (Adam Porter)
| * a3e05a7 Fix: (ement-room-send-file) File size (Adam Porter)
| * f2b5831 Add/Change: History lists for user IDs in ement-connect (Adam Porter)
| * fbf40d6 Add/Change: Use separate history lists for reading from minibuffer (Adam Porter)
| * 2435d03 Fix: (ement-room--format-m.file) Don't assume size is present (Adam Porter)
| * 82f1186 Tidy: Indentation (Adam Porter)
| * 0c94dcc Add: ement-room-quote face (Adam Porter)
| * eefda9c Comment: Add TODO (Adam Porter)
| * b5e3626 Meta: v0.12-pre (Adam Porter)
| * d2b7a84 (tag: v0.11) Release: v0.11 (Adam Porter)
| * a8c8680 Tidy: Newline (Adam Porter)
| * abedcf0 Meta: Enable testing with Emacs 29.1 (Adam Porter)
| * 837991d Tidy: Lint warnings on CI (Adam Porter)
| * 63e22ba Meta: Install ispell for CI (Adam Porter)
| * 3a01d42 Docs: Update installation section (Adam Porter)
| * 8f6fef3 Docs: Update installation section (Adam Porter)
| * 284db78 Docs: Tidy (Adam Porter)
| * 7f82724 Docs: Tidy (Adam Porter)
| * 6fc2908 Tidy: Compilation warning (Adam Porter)
| * 25c4eab Tidy: Compilation warning (Adam Porter)
| * 06bb5df Tidy: Compilation warning (Adam Porter)
| * 54aac8c Tidy: Compilation warning (Adam Porter)
| * a2fc8a3 Tidy: Use ash instead of lsh (Adam Porter)
| * 13077e3 Change: (ement-connect) Show message when browsing externally (Adam Porter)
| * fc2748a Change: (ement-connect) Improve login flow errors (Adam Porter)
| * 3ddbd38 Change: (ement-connect) Omit unhandled login flows (Adam Porter)
| * 36677f4 Change: (ement-room-image-show) Enable image-mode (Adam Porter)
| |\
| | * 76e90dc Enable image-mode when displaying images in a new buffer (Steven Allen)
| |/
| * c217fe4 Docs: Add Guix package link (Adam Porter)
| |\
| | * 4ae7b74 Add Guix package search link (jgart)
| |/
| * c9bcea5 Fix: (ement-read-receipt-idle-timer) Could be duplicated (Adam Porter)
| * 565b999 Fix: (ement-notify--log-to-buffer) Select log buffer's window (Adam Porter)
| * 1e348a7 Comment: Add (Adam Porter)
| * eb0e0f8 Fix: (ement-room-notification-state) Push rules' actions order (Adam Porter)
| |\
| | * 89b3fc3 Update changelog (Adam Porter)
| | * 271cf52 Require seq library (Adam Porter)
| | * 2349b91 Fix room list display with empty notification actions (Steven Allen)
| |/
| * dbe2212 Fix: (ement-room-list-next-unread) Infinite loop (Adam Porter)
| * 60e9a50 Docs: Update changelog (Adam Porter)
| * 804dbcd Fix: (ement-connect) Cleanup timer (Adam Porter)
| * 765ec7f Change/Fix: (ement-connect) Use external browser for SSO (Adam Porter)
| * 8b830e0 Change: (ement-connect) Add content to SSO browser page (Adam Porter)
| * 3c50f77 Tidy: (ement--sync) Message (Adam Porter)
| * bbf052a Fix: (ement-room-edit-message) Already edited events (Adam Porter)
| * 909abd4 Fix: (ement-notify) Demote errors (Adam Porter)
| * 18b7ae7 Merge: Improvements to image viewing commands (Adam Porter)
| |\
| | * 5d54f2a Docs: Edit changelog entries (Adam Porter)
| | * e0510fc Update changelog for image view changes (Steven Allen)
| | * 1ae5ac2 Support RET/M-RET for viewing/scaling images (Steven Allen)
| |/
| * 50e426e Meta: v0.11-pre (Adam Porter)
| * fba18f9 (tag: v0.10) Release: v0.10 (Adam Porter)
| * 33f0129 Docs: Mention security fix in changelog (Adam Porter)
| * 91c9821 Fix: (ement--event-replaces-p) (Adam Porter)
| * 30dda74 Change/Fix: (-room-send-file) Pass filename for upload (Adam Porter)
| * c7415f1 Change: (ement-room-mode-map) Bind "m" to ement-room-mark-read (Adam Porter)
| * cff1c9b Change: (ement-room-scroll-up-mark-read) Move f-r marker to TOW (Adam Porter)
| * 995f072 Docs: Update changelog (Adam Porter)
| * 4d28436 Fix: (ement--hostname-uri) Don't assume JSON has needed value (Adam Porter)
| * 03d411a (upstream/wip/sso) Add: SSO support (Adam Porter)
| * d890bf0 Fix: (ement-room-mwheel-scroll) Don't fetch at eob (Adam Porter)
| |\
| | * e3d98c7 Update changelog (Adam Porter)
| | * 4f399a6 fix: only autoload when scrolling past the top (Steven Allen)
| |/
| * b835641 Revert "Fix: (ement-room--pp-thing) Timestamp with date headers" (Adam Porter)
| * b821888 (upstream/wip/146) Fix: (ement-room-list) Don't reinitialize major mode (Adam Porter)
| * ee0eb6f Fix: (-widget ement-room-membership) (Adam Porter)
| |\
| | * 817ed15 Update changelog, formatting (Adam Porter)
| | * cdd35a0 Fix: avoid spurious indent in room membership changes (Steven Allen)
| |/
| * ec31709 Fix: Customization group for two faces (Adam Porter)
| * 94547e9 Fix: (ement-room--pp-thing) Timestamp with date headers (Adam Porter)
| * 4a8519b Docs: Improve changelog (Adam Porter)
| * 524bf5f Change: (ement-room--buffer) Add footer for spaces (Adam Porter)
| * b6a491a Change: (ement-view-space) Show space name and alias (Adam Porter)
| * 6468027 Change: (ement--space-p) Rename function (Adam Porter)
| * e3b4240 Tidy: Compilation warning (Adam Porter)
| * e10d37d Change: (ement-room-list-RET) Use ement-view-space for spaces (Adam Porter)
| * 3965a1d Change: (ement-describe-room) Show whether room or space (Adam Porter)
* | 18a3035 Add: Support for sending rich replies (Visuwesh)
* | 3e5f095 Change: (ement-room--format-quotation-text) Quote prefix each line (Visuwesh)
* | e4e71ad Minor refactoring and tidying (Adam Porter)
* | 6976137 Add: Support for rich reply support (Visuwesh)
|/
* 8b56efa Merge: v0.9.3 (Adam Porter)
In particular, you want to have a good understanding of what was going on in the two merge commits Merge remote-tracking branch 'refs/remotes/origin/feature/rich-replies' into feature/rich-replies, where you merged an alternative history of your own branch back into your own branch, which left your branch containing many different versions of some of your commits. (And equally importantly, you want to understand how that happened, and what you could have done differently to get a simpler branch history.)
Your feature/rich-replies-rebase has ended up like this (where I've coloured commits which have 'duplicates', on account of the merge history):
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:
* 1e18f88 Fix: Quote event being replied to in original message for a reply (Visuwesh)
* f3a9c6c Tidy: (ement-room--format-quotation-html) Remove debugging marker (Visuwesh)
* 5c63931 Change: Always send rich replies, remove ement-room-send-rich-replies (Visuwesh)
* c1bb30a Fix: Strip off fallback quotes from HTML bodies if rich reply event (Visuwesh)
* 3a5b7e1 Always include fallback quotes regardless of rich reply setting (Visuwesh)
* 3e73d9b Tidy: Remove WHEN wrapper (Adam Porter)
* 37d9edb Fix: (ement-room-send-rich-replies) Specify type (Adam Porter)
* ee77b0a Tidy: Compiler warning (Adam Porter)
* 15a7b76 Tidy: Use ement--put-event (Adam Porter)
* 2a1a6a3 Comment: Add FIXME (Adam Porter)
* 10b4397 Revert: Whitespace-only changes (Adam Porter)
* b39e4ac Tidy: Indentation (Adam Porter)
* 9c47fc1 Change: (ement-room-send-rich-replies) Docstring, add link to spec (Adam Porter)
* 2144647 Change: (ement-send-message) Remove :rich-reply argument (Adam Porter)
* a092da3 Add: Support for sending rich replies (Visuwesh)
* e5498e5 Change: (ement-room--format-quotation-text) Quote prefix each line (Visuwesh)
* 2b59533 Minor refactoring and tidying (Adam Porter)
* b4e183c Add: Support for rich reply support (Visuwesh)
* 3df2371 (upstream/master, master) Fix: Reply to original messages (Adam Porter)
I.e. one version of each commit.
That's what I've pushed to https://github.com/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 ()
Coming back to this after a long hiatus to review that diff...
Obviously we don't actually want the trailing whitespace here, but removing it wasn't part of the original branch. It was the merge commit https://github.com/9viz/ement.el/commit/da037e586fd615e8c2c5c22b39a1fb2525bb2f40 which took it out.
modified ement-lib.el
@@ -32,7 +32,7 @@
(require 'ewoc)
(require 'pcase)
(require 'subr-x)
-
+
(require 'taxy-magit-section)
(require 'ement-macros))
This next part was from commit https://github.com/alphapapa/ement.el/commit/6ec7d314a061db82d0d9caf4a4a665e728477286 and so I believe we want this change:
modified ement-lib.el
@@ -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
The original commit for this next bit was https://github.com/9viz/ement.el/commit/f67698ac6fd6a4a712580a8c3e4712a82e059cc2 so it was a rebase accident that these lines were removed in the rebased commit https://github.com/9viz/ement.el/commit/80a0bf3820204c3a656e21687257668c0e27a1ed, and so we do want this change:
@@ -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
And I believe this last part is good too:
modified ement-room.el
@@ -2256,14 +2256,16 @@ ement-room-write-reply
(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 ()
The only two commits touching this code in the original rich-replies branch were commits https://github.com/9viz/ement.el/commit/cc0c97da86fc6702976f9f48748a28fb1c5713a8 and https://github.com/9viz/ement.el/commit/f67698ac6fd6a4a712580a8c3e4712a82e059cc2 with the latter reverting the change from the former.
The initial rebase wasn't taking commit https://github.com/alphapapa/ement.el/commit/3df237176262ecf6def8f0766154fea066009a97 into account, and it looks correct for us to end up with that code given that the other two commits cancelled each other out (for this bit of code), so I believe this is correct too:
@@ -2262,9 +2262,10 @@ ement-room-write-reply
(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)))))
+ 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)))))
All in all, I think my rebase is good, so I suggest doing a hard-reset of 9viz/feature/rich-replies to that state, and pushing that back for this pull request:
git checkout feature/rich-replies
git remote add phil-s https://github.com/phil-s/ement.el.git --track phil-s/rich-replies --fetch
git reset --hard phil-s/phil-s/rich-replies
git push --force-with-lease origin feature/rich-replies
n.b. I've also rebased my branch over master again, which happened without conflicts.