mark room as fully read on switching to its buffer

[saadjsct]

No description provided.

[alphapapa]

Hi,

I appreciate your sending this in, but there are a few issues.

1. Adding a function to post-command-hook is invasive and has the potential to slow down the entire Emacs process. Remember that virtually every keypress executes a command.
2. What is the purpose of the while-no-input and (redisplay)? Those seem like "code smells" and probably indicate taking a wrong approach.
3. It would probably be better and simpler to use something like buffer-list-update-hook to simply check whether the current buffer is a Matrix room buffer, and if so, mark its events as read if they have not already been marked read.
4. If possible, we should avoid tracking what the "last buffer" is ourselves. That is generally out-of-scope for this package. A simple rule of thumb is, "What if every package did that?" It would result in a lot of duplicated functionality, which indicates a better design is needed.
5. We should avoid sending unnecessary mark-read requests for rooms and events already marked as read.

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 buffer-list-update-hook could simply send a mark-read request whenever the current buffer is a Matrix room buffer. A little bookkeeping could easily avoid sending unnecessary requests to rooms already marked as read (you should probably integrate with the last-seen line in the buffer, which serves the purpose of marking new vs. old events). That could all probably be implemented with a single function and no additional variables.

What do you think?

Thanks.

[saadjsct]

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.
2- the following is from the post command hook documentation:

It is a bad idea to use this hook for expensive processing. If
unavoidable, wrap your code in ‘(while-no-input (redisplay) CODE)’ to
avoid making Emacs unresponsive while the user types.

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
i found also this file https://github.com/10sr/switch-buffer-functions-el/blob/master/switch-buffer-functions.el which uses also post-command-hook but i didn't want to introduce a new dependency and i think my function is faster for our purpose.

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.

[alphapapa]

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 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.

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,

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.

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 .

Yes, unfortunately it also seems slow sometimes.

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

You're right. Most of the ones I see are check-buffers functions which seem to be defined by the define-globalized-minor-mode macro. I'm not sure exactly what's going on with those, because those functions also seem to remove themselves from the hook, while another cmhh function adds it back to the hook. More digging would probably reveal the answer.

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.

Yes, that might be acceptable, at least for users who want the feature activated.

Some other possibilities that might help:

• Use window-configuration-change-hook to detect when a room buffer is selected.
• Use function advice to detect when a room buffer is selected. This might need to be applied to multiple functions, and might also need to be user-customizable.

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 frame-purpose and the room sidebar in this package, so you might look at that code for inspiration, too.

the following is from the post command hook documentation:

Thanks, I hadn't seen that. Good job being thorough there!

3- as i said i tried it and i found it has a noticeable effect on the overall responsiveness
i found also this file https://github.com/10sr/switch-buffer-functions-el/blob/master/switch-buffer-functions.el which uses also post-command-hook but i didn't want to introduce a new dependency and i think my function is faster for our purpose.

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.

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.

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.

but other than that is there any issues in the api function of actually marking it as read.

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 matrix-post instead of matrix-request-request, etc.

Thanks.

[saadjsct]

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.

[saadjsct]

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

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

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.

you should probably use matrix-post instead of matrix-request-request

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.
thanks.

[alphapapa]

Hi again,

I need to look at the code more closely, but here are some initial thoughts:

1. You can use the buffer-local variable matrix-client-room to detect whether a buffer represents a Matrix room. See https://github.com/alphapapa/matrix-client.el/blob/master/matrix-client-room.el#L722 and function buffer-local-value. The matrix-get-buffer-room function shouldn't be necessary.
2. Since the previous and current args are not used, they can be prefixed with _, which will prevent byte-compiler warnings.
3. I think this conditional might perform better in matrix-mark-buffer-fully-read:

(when (and matrix-client-mark-as-read-on-buffer-switch
           (buffer-local-value 'matrix-client-room))

4. Again, I need to consult the API docs, but anyway: is there a logical way to distinguish marking rooms as read and sending read receipts? From the user's point of view, these may seem like separate issues, i.e. some users might want to not send read receipts but might want to mark rooms as read for their own benefit. I don't know how the API handles this, but if it's possible to separate these actions, we probably should.
5. This is sort of a silly nitpick, but I think the slot should be called something like last-seen-event.
6. This might be better served with another PR, but it occurs to me that, to implement this feature correctly (and more like Riot does), we need to handle window scroll events, and we need to hook into when new events are displayed. That is, when new events arrive in an already-selected buffer, if the new events are off-screen, the event shouldn't be marked as read; but if the new events are visible, they should be (probably, anyway; we might want to wait for some kind of user input or action. This does start to get complicated...).

Thanks for your work on this!

[saadjsct]

points 1, 2, 3, 5 done

Again, I need to consult the API docs, but anyway: is there a logical way to distinguish marking rooms as read and sending read receipts?

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.

some users might want to not send read receipts but might want to mark rooms as read for their own benefit. I don't know how the API handles this, but if it's possible to separate these actions, we probably should.

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 .

[alphapapa]

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.

[saadjsct]

here are a few other things I've noticed. I'll use the line-by-line reviews for it

where can i find those, i don't see them.

[saadjsct]

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

[alphapapa]

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.

[saadjsct]

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

[saadjsct]

what do you think about it now, it's almost rewritten

[alphapapa]

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:

1. 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.
2. 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?

[saadjsct]

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.

[alphapapa]

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. :)

[saadjsct]

yeah but its the event object itself that i am comparing here.

[alphapapa]

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?

[saadjsct]

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.

[saadjsct]

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.

[alphapapa]

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.

[saadjsct]

that's great tip here thanks. i don't know if the new implementation is good enough or what.

[alphapapa]

here are a few other things I've noticed. I'll use the line-by-line reviews for it

where can i find those, i don't see them.

Oops, sorry. Got confused by the GitHub UI and forgot to "Submit" the review.

[saadjsct]

what do you think about the new implementation ?

[alphapapa]

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.

[seanfarley]

I just stumbled upon this issue and was wondering if there is any update to marking an entire buffer as read?

[alphapapa]

Closing issues and archiving repository. Please see https://github.com/alphapapa/ement.el for the new client.