Fix sender header insertion with respect to membership changes #158
Conversation
Stebalien
changed the title
|
Thanks, I'll want to review and test this carefully before merging, so it might be a little while. I do appreciate your working on this, since I don't use the "Elemental" style very often. |
|
np. Take your time.
|
|
Hm. Actually, I've found one bug. The first sender header in the buffer stays the first sender header, even after loading new events. I believe
Then it needs to "fix" all headers in this range. As you suggested, simply deleting the sender headers first then re-inserting them is likely the best approach. |
|
I'll tackle this tomorrow. |
IIRC it already does that--or maybe I did that for the timestamp headers. I don't touch this code often.
My hesitation with that approach is that it will, of course, take longer to do so with every incoming message. Sometimes, e.g. I look way back in a room's history, loading a few thousand old events, which Ement handles just fine; but if this approach were used, it would likely perform poorly in that case. So I'd prefer to avoid that if possible. Something I haven't done is dig into the Matrix React SDK to see how it does it; but since it uses React, I don't know if any of its algorithms would be relevant here. Maybe something relatively simple would help us ensure that we cover a large enough range while avoiding scanning the whole buffer, like scanning back to the previous full-date timestamp, or something like that. Even that could be a performance issue in very busy rooms like #emacs:libera.chat or #matrix:matrix.org, but since the "Elemental" style isn't the default, maybe it would be acceptable. |
It shouldn't. I'm talking about fixing just the range of messages that were modified, not the entire buffer.
Ah, that's why I suggested scanning back/forward to the previous/next message (which we don't currently do for sender headers). |
| @@ -3034,7 +3020,7 @@ Search starts from node START and moves by NEXT." | |||
| ;; HACK: Insert after any read markers. | |||
| (cl-loop for node-after-node-before = (ewoc-next ewoc event-node-before) | |||
| while node-after-node-before | |||
| while (not (ement-event-p (ewoc-data node-after-node-before))) | |||
| while (read-marker-p (ewoc-data node-after-node-before)) | |||
There was a problem hiding this comment.
This change is optional, but it helps keep sender headers attached to their messages.
| (setf end-node (ement-room--ewoc-next-matching ewoc end-node #'message-event-p))) | ||
|
|
||
| (let* ((event-node start-node) prev-node) | ||
| (while (and event-node (not (eq event-node end-node))) |
There was a problem hiding this comment.
eq should be sufficient now, as far as I can tell.
There was a problem hiding this comment.
(I've tested this with simple printf debugging and it appears to work)
ement-room.el
Outdated
| ;; We have a previous sender, but it's wrong. Fix it. | ||
| ((ement-user-p (ewoc-data prev-node)) | ||
| (unless (equal sender (ewoc-data prev-node)) | ||
| (ewoc-set-data prev-node sender))) |
There was a problem hiding this comment.
This can happen if we insert an event after a header but before another message. The read-marker fix below should prevent this from happening (in most cases), but we might as well handle it.
Stebalien
force-pushed
the
fix/157
branch
2 times, most recently
from
b09619a to
1f22358
Compare
|
Ok, I think this should actually be good now. |
|
Deferring this to 0.12 so 0.11 can be released sooner. |
alphapapa
force-pushed
the
master
branch
4 times, most recently
from
a68abe0 to
9da7b8e
Compare
|
No problem, I'm the one asking for your time. If there's anything I can do to make the review easier (or any general feedback), please let me know. |
|
Thanks. I was hoping to have time for things like this last weekend, but I didn't. Probably I just need to start running this PR's code on my own system to test it "in the background." FWIW, have you been using it regularly yourself? |
|
Not every day, but I use this patch whenever I use ement.el and haven't seen any issues. |
|
🥺 |
Geez, you make me DDG that emoji to see what it's supposed to mean. :P You'll have to forgive me, as I have had a lot going on outside of my various Emacs packages. And each one of them has a list of issues and PRs that are waiting. In the past couple of days I just merged a couple that were 3 years old. In the meantime, I would still be grateful for more feedback from having tested this patch. But I'll go ahead and rebase it and install the PR branch to test it myself... |
|
I've installed the PR's branch. Seems to work well so far, but I'll have to remember to use the "Elemental" message format in order to see this code in action. If no problems are found, I'll plan to merge this for v0.14. |
|
Thank you ❤️. And I understand, I've merged some very old PRs. In terms of testing, I've been using this more frequently and still haven't run into any issues. |
alphapapa
force-pushed
the
fix/157
branch
2 times, most recently
from
9f608b9 to
83beebc
Compare
|
@Stebalien I think I'm ready to merge this. AFAIK you already signed the FSF CA, because I see some non-exempt commits from you in emacs.git, and of course you've contributed several here already. But I can't find where I've asked you for confirmation already, so allow me to ask for confirmation now, just to be sure. :) Have you signed it already? Thanks. |
|
Yep, the FSF officially owns all my Emacs work. |
Thanks. BTW, I encountered an error which I've pushed an attempted fix for here, as well as adding a small failsafe: b520bc6 Please let me know what you think about that change. |
|
Note as well that I discovered that the failsafe is unnecessary (which I should have realized from reading your comments in the code), so I removed it. Seems to be working correctly now, but I still think it needs more testing, because this PR's code isn't the first time I've found a bug in this kind of code that only manifested after a few days (e.g. re-syncing after a long interruption). |
|
Hm. That shouldn't be possible. |
You're right, the bug was caused by my "Tidy..." commit that incorrectly consolidated one of the clauses. I'll take care of it. Thanks. |
Bug was introduced in a8c8b08. See <alphapapa#158 (comment)>.
|
I'm going to go ahead and merge this to master now. Hopefully enough users use the "Elemental" format for it to get some testing. :) |
Fix: (ement-room--insert-sender-headers) After suspending, resuming, and syncing again, an error happened, failing here because PREV-NODE's data was an EMENT-USER rather than an EMENT-EVENT, so EMENT-EVENT-SENDER failed on it. Now we test PREV-NODE's data's type to ensure it's an event before testing the sender. Also, as a failsafe, if the PREV-NODE is something else (could be membership events, e.g.), we insert the sender header, to be sure a message event has one visible. Fix: Remove unneeded failsafe This was duplicating sender headers; Stebalien's design makes this unnecessary. Fix: (ement-room--insert-sender-headers) Bug was introduced in a8c8b08. See <alphapapa#158 (comment)>.
Only insert a sender header if the previous message is from a different sender, and there is no sender header between the current message and the previous message. While the previous version of this function explicitly skipped the read marker, this version searches for known events (messages and sender headers), skipping everything it doesn't understand. This should make it robust to adding new events, widgets, etc. fixes alphapapa#157 Fix new event insertion with respect to sender headers Only skip over the read marker, don't skip over sender headers. Make sender headers robust 1. Widen the "fixup" range to the next message after the end-node. 2. Simplify the loop logic. 3. Fixup incorrect sender headers.
For Emacs 29.
Fix: (ement-room--insert-sender-headers) After suspending, resuming, and syncing again, an error happened, failing here because PREV-NODE's data was an EMENT-USER rather than an EMENT-EVENT, so EMENT-EVENT-SENDER failed on it. Now we test PREV-NODE's data's type to ensure it's an event before testing the sender. Also, as a failsafe, if the PREV-NODE is something else (could be membership events, e.g.), we insert the sender header, to be sure a message event has one visible. Fix: Remove unneeded failsafe This was duplicating sender headers; Stebalien's design makes this unnecessary. Fix: (ement-room--insert-sender-headers) Bug was introduced in a8c8b08. See <alphapapa#158 (comment)>.
|
@Stebalien Thanks for your work on this and your patience. |
|
No, thank you! I realize you don't use this feature so I'm just glad I don't have to maintain it myself. |
I mean...if this breaks, I'll probably be pinging you to help fix it. ;) |
|
No problem, that's better than constantly rebasing.
|
Only insert a sender header if the previous message is from a different sender, and there is no sender header between the current message and the previous message.
While the previous version of this function explicitly skipped the read marker, this version searches for known events (messages and sender headers), skipping everything it doesn't understand. This should make it robust to adding new events, widgets, etc.
fixes #157