-
Notifications
You must be signed in to change notification settings - Fork 20
mark room as fully read on switching to its buffer #84
Conversation
Hi, I appreciate your sending this in, but there are a few issues.
I'd suggest backing up a bit and trying to make a simple prototype that could be added to your personal Emacs config. For example, a function that runs in What do you think? Thanks. |
first of all i appreciate your experience in elisp. i learned alot from your code as i am still a novice in elisp, and found the way you are extending the language is very fascinating. thanks for your review, here is what i think 1- i agree with you that hooking a function to post-command-hook is invasive but apparently there is no switch-buffer-hook in emacs, but there is two ways i know of, one way is like you said hooking to buffer-list-update-hook which i tried first and i found it so slow . the other way is to hook to post-command-hook which if you see its value on your emacs there is lots of packages are hooking to it to check buffers, and the function itself does not run on every key press just the (eq (current-buffer) matrix--last-buffer)) which if false it doesn't execute any other line.
i think in this case its unavoidable although i didn't need it at all and i didn't notice any overhead but just in case if something happened it wont happen while the user is inputting text 3- as i said i tried it and i found it has a noticeable effect on the overall responsiveness 4- i agree with you on this point but as you said if possible i don't know a built in function or another package which provides this functionality. 5- yeah i thought about adding a slot in the room class for the last read message like end-token but i forgot about this issue. but yeah its easily can be added. but other than that is there any issues in the api function of actually marking it as read. if so i can work on the other stuff you mentioned and maybe think of a better way to integrate it. |
Thanks for the kind words. I could see that you are new to elisp, and I was impressed by your code, because it's clear that you studied the way that so many macros are used in this package (some would say that there are too many, but I've enjoyed using the package as a place to experiment and learn). Good job.
Yes, unfortunately there is no such hook in Emacs. Maybe we need to propose one, although I'm guessing it's been proposed before, and I wonder why it would have been rejected.
Yes, unfortunately it also seems slow sometimes.
You're right. Most of the ones I see are
Yes, that might be acceptable, at least for users who want the feature activated. Some other possibilities that might help:
Either of those might work, but I don't know enough of the details, so it would require some research and testing. Something we should probably keep in mind is that, AFAIK it doesn't matter when a buffer is made current; what matters is when a buffer's window is selected by the user. I think this is an important distinction. As one of the docstrings I saw when looking into this reminded me, buffers can be selected by code that runs between redisplays and without the user's awareness; so if we marked rooms as read when their buffer became current, that might not mean that the user had actually seen the room's buffer. This is similar to issues I had to deal with when working on
Thanks, I hadn't seen that. Good job being thorough there!
Good job finding that! You are thorough, indeed. It is good to avoid dependencies, but that package is in MELPA and seems very simple and purposeful, so it might be better to use it than reimplementing it ourselves. If every package that needs this functionality used it, it would reduce the number of functions called and tests run when the buffer changes.
That would work, yes. You could also use the last-seen overlay to detect whether there are unread messages in a room. Whichever is simplest and fastest would probably be the best way.
Yes, there are some changes I would prefer to that part of the code, but I would have to study the API docs and think about it a bit first. I haven't looked into that part of the API before. Other than that, there are minor issues like docstring typos, and you should probably use Thanks. |
ok i have a better insight now about the problem, certainly i will spend more time on it and rewrite it again. thanks for you detailed review and comments. |
what do you think about the last changes, now we are using switch-buffer-functions and we don't send unnecessary mark-read requests using a new slot holding the last read event (i found in my opinion the slot solution is way simpler than using the last-seen overlay). regarding this
that was one of the reason i decided to go with post-command-hook in the first place, the post-command-hook runs after interactive commands, so switching buffers using with-current-buffer or with set-buffer inside lisp code will not trigger switch-buffer-functions. and also i tested it on switching windows and even frames as those are interactive commands which change the current buffer and trigger switch-buffer-functions.
first i used matrix-post, but i found request is more reliable than url-retrieve which seemed to be not always asynchronous for some reason. if there is more issues i don't mind at all taking another iteration and refine it more. |
Hi again, I need to look at the code more closely, but here are some initial thoughts:
(when (and matrix-client-mark-as-read-on-buffer-switch
(buffer-local-value 'matrix-client-room))
Thanks for your work on this! |
points 1, 2, 3, 5 done
yeah they are different things but what they mean is a bit different than what you are pointing to in this point. i found the spec is so misleading and ambiguous on the difference between those markers, but after some searching and testing i think i got it now. m.read points to the latest message that you've read but m.fully_read indicates that you've read (or ignored) all messages up to and including that message. For example, if you're in a room and go away for a bit, and receive 20 new messages, but only 10 messages will fit on the screen. When you come back, m.read will point to the last of the 20 messages, and m.fully_read will point to the last message before the 20 new messages, to indicate that there are new messages that haven't been read yet. When you scroll up to read those messages, then the client can set m.fully_read to point to the last message. so in order to treat them differently we have to implement exactly what you said in point 6. which i agree with you its better if we work on it in another PR.
unfortunately the api does not support yet informing other client that the user have read certain messages without informing other participants in the room (sending a read receipt). but i asked on matrix HQ and they said it will be supported soon (maybe by introducing a new marker). we can track this issue at element-hq/element-web#2527 and element-hq/riot-meta#66 . |
Thanks, you're making great progress on this PR. If you're still interested in improving it before merging, here are a few other things I've noticed. I'll use the line-by-line reviews for it; I didn't think to do that before. |
where can i find those, i don't see them. |
i updated matrix-mark-fully-read to use matrix-post as you said, but opened #87 to update matrix-post and its sister functions to use matrix-request-request |
matrix-api-r0.3.0.el
Outdated
@@ -1303,6 +1305,27 @@ TYPING-P should be t or nil." | |||
(matrix-log (a-list 'fn 'matrix-typing-error-callback | |||
'args args)))))) | |||
|
|||
(cl-defun matrix-mark-fully-read (room) |
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.
Can we generalize this API-side function a bit? e.g. something like matrix-room-read-markers
, which could accept arguments room
, fully-read
and read
. The client-side code should probably specify the event IDs.
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.
shouldn't we also rename last-seen-event back to last-read and add last-fully-read slot to room, that also will be beneficial when we treat them differently
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.
what do you think about it now, it's almost rewritten
matrix-api-r0.3.0.el
Outdated
@@ -1303,6 +1305,27 @@ TYPING-P should be t or nil." | |||
(matrix-log (a-list 'fn 'matrix-typing-error-callback | |||
'args args)))))) | |||
|
|||
(cl-defun matrix-mark-fully-read (room) |
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.
I looked briefly at the spec, and the read_markers
endpoint is in version 0.4.0 of the client-server API. I'm not sure whether I've maintained any kind of version-specific purity in this code, but the explicitly targeted version at the moment is 0.3.0, and I'm reluctant to mix in code for newer versions silently. So there are a few options here:
- Target 0.4.0. However, that may also require making numerous other changes: https://matrix.org/docs/spec/client_server/r0.4.0.html#changelog Although, they are supposed to be backward-compatible, so maybe we could implement them as needed.
- Don't use
read_markers
yet; use 0.3.0 spec only.
I want to do whatever will make the code easier to maintain in the long run. What do you think?
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.
why not gradually moving to 0.4 until we can fully target it ?. most synapse servers now supports 0.4 (and most importantly matrix.org). for new functions we can add a note in their documentation that they implement a newer version of the api than the explicitly targeted version.
matrix-api-r0.3.0.el
Outdated
(txn-id (cl-incf txn-id)) | ||
(room-id (url-hexify-string id)) | ||
(endpoint (format$ "rooms/$room-id/read_markers"))) | ||
(unless (eq event last-seen-event) |
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.
The event IDs are strings, so you should use string=
rather than eq
. Remember that eq
, eql
, equal
, and functions like string=
are all different in Lisp. :)
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.
yeah but its the event object itself that i am comparing here.
matrix-api-r0.3.0.el
Outdated
(room-id (url-hexify-string id)) | ||
(endpoint (format$ "rooms/$room-id/read_markers"))) | ||
(unless (eq event last-seen-event) | ||
(matrix-request-request session endpoint |
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.
You mentioned that matrix-post
seemed to have some problems, so you use matrix-request-request
instead. I'm fine with using that if necessary, but only if absolutely necessary, because I want to avoid implementation-specific code that might need to be changed in the future. I only added it when I had no other choice.
So can you be more specific about the problems you had with using matrix-post
here?
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.
it makes emacs unresponsive until it returns (and sometimes it doesn't return). this problem i know its because of matrix-post because when i used matrix-request-request it worked perfectly fine (and i called it explicitly to ensure that it is causing this problem), but i reverted back to matrix-post because its not something specific to this implementation if the problem is in my system then matrix-post is ok and if not matrix-post itself should be fixed.
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.
the last commit fixed the issue in matrix-post, it was something related to the callback being nil. but for some reason matrix-request-request handled this case better. i didn't investigate deeper but it works fast now.
matrix-client.el
Outdated
room) | ||
(matrix-mark-fully-read room)))) | ||
|
||
(add-hook 'switch-buffer-functions 'matrix-mark-buffer-fully-read) |
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.
To be thorough, we should add and remove this hook in the defcustom
depending on whether the feature is enabled; that way it won't run when it's disabled. For an example of doing something similar, see the option matrix-client-room-avatar-in-buffer-name-size
and how the :set
argument to defcustom
is used.
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.
that's great tip here thanks. i don't know if the new implementation is good enough or what.
Oops, sorry. Got confused by the GitHub UI and forgot to "Submit" the review. |
what do you think about the new implementation ? |
Hi, I'm sorry I forgot about this PR. I haven't been using Matrix as much since, unfortunately, some of my friends stopped using it. But I'm trying to remember to "jack in" more often. :) Are you still using this client? If so, have you been using this the code from this PR? I don't want to keep your PR in limbo, but I am still reluctant to split the API targets, like we talked about. Have you had any more thoughts since we talked last? I saw your Python CLI Matrix client just now. Very cool! Let me know what you think, and I'll review the code again. Thanks. |
9db66ac
to
9003011
Compare
I just stumbled upon this issue and was wondering if there is any update to marking an entire buffer as read? |
Closing issues and archiving repository. Please see https://github.com/alphapapa/ement.el for the new client. |
No description provided.