Fix: (ement-room-list) Prevent window position being set via markers

[phil-s]

Any markers in the room list buffer will be set to position 1 when erase-buffer is called each time the buffer is rebuilt, which caused the window point to be forcibly moved to the start of the buffer in many situations.

See #175

[phil-s]

Ah, not quite done yet...

set-window-point(1): (set-window-point@my-debug apply set-window-point set-window-buffer-start-and-point switch-to-prev-buffer replace-buffer-in-windows kill-buffer funcall-interactively call-interactively command-execute)

[phil-s]

Sigh, this looks like an Emacs bug...

switch-to-buffer only uses a marker from window-prev-buffers when switch-to-buffer-preserve-window-point is non-nil, but both switch-to-prev-buffer and switch-to-next-buffer call set-window-buffer-start-and-point using data from window-prev-buffers, and neither of those cases are conditional on that variable.

(I've logged a bug report and CC'd you, Adam.)

[alphapapa]

Sigh, this looks like an Emacs bug...

switch-to-buffer only uses a marker from window-prev-buffers when switch-to-buffer-preserve-window-point is non-nil, but both switch-to-prev-buffer and switch-to-next-buffer call set-window-buffer-start-and-point using data from window-prev-buffers, and neither of those cases are conditional on that variable.

(I've logged a bug report and CC'd you, Adam.)

Thank you, Phil. This problem was driving me crazy. I'm relieved to know that it's an Emacs bug after all.

Could you link the report here please?

[phil-s]

The upstream bug report is https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64911

Were it not for that, I suspect this PR would have been sufficient to resolve the issue entirely. As it is the current code will resolve it partially. I'm sure I can work around the remaining aspects in a similar fashion to what I've done for the quit-restore markers, using window-prev-buffers and set-window-prev-buffers. My initial look suggested that these values may need to remain markers (I think I saw code making that assumption), so it'd be a case of finding and restoring them around each regeneration.

Alternatively, at this point I think using replace-buffer-contents may be worth exploring. I thought about that in #175 and Eli suggested it in one of his replies. It had seemed slightly less appealing to me initially for a few reasons, but at this stage it might be the best option. I think that will mean replacing this:

            (delete-all-overlays)
            (erase-buffer)
            <generate new contents>

with this:

            (delete-all-overlays)
            <generate new contents in newbuffer>
            (replace-buffer-contents <newbuffer>)
            (kill-buffer <newbuffer>)

In principle it's not that simple if the optional MAX-SECS / MAX-COSTS args and behaviours are to be accounted for robustly, but maybe this will be fast enough that we don't need to worry in practice.

(That might not be good enough on its own, though. It really depends on how the replace-buffer-contents algorithm copes with the ways in which the buffer contents can be rearranged. I imagine it will avoid the markers being set back to 1, but I don't know whether it would reliably move each marker to a desirable position. It might work out well, but I can envisage the kinds of buffer content changes in play here causing some problems.)

[alphapapa]

Maybe I'm missing something, but AIUI, the problem is that the window-point stuff uses markers internally, which I didn't realize; I expected that those were using integer positions. So could we work around the problem by recording the point markers' positions and resetting from those (rather than using the actual markers)?

It might also be good to save the magit-section identity before erasing the buffer and try to go back to the same section after updating the buffer, if it's still present, and then fall back to positions.

[phil-s]

could we work around the problem by recording the point markers' positions and resetting from those

Yes indeed -- and in fact I've done exactly that in the most recent commit.

It might also be good to save the magit-section identity before erasing the buffer and try to go back to the same section after updating the buffer, if it's still present, and then fall back to positions.

Yep, I'd noted your section-ident code and had also figured that would be the next thing to do. Completely on the same page.

[alphapapa]

Thanks. A final question, then: Do we still need to walk the windows and do things with their markers, or can we just record the marker positions for the room list window and use that?

[phil-s]

We do still need to capture and reposition the markers, because we're not in control of the code which uses them to set the window point (in certain scenarios). So we need to ensure that the markers are positioned the way we want them, otherwise they'll still wind up being back at position 1 when that other code uses them.

[phil-s]

Can you confirm whether the pre-existing section-ident memory/restore is actually working for you?

I was just failing to make the related change to my code, and when I test the following interactively I get nil, even though I'm definitely getting a section identifier from the first line, and the room is still valid:

• M-: (setq si (magit-section-ident (magit-current-section)))
• refresh room list
• M-: (magit-get-section si)

[phil-s]

I think I was missing the fact that the section identifier varies with the hierarchical structure, so when a room moves from, say, "Unspaced" to "Buffer" its ID has different taxy-magit-section-section components and no longer matches. I guess the behaviour I was after would need to be based on unique room ID.

I see I can obtain room IDs at a position like so:

(ement-room-id (ement--room-at-point))

What would be the most efficient way to do the inverse -- locate a line in the room list buffer based on a room ID?

[alphapapa]

This may be relevant:

magit-section-ident-value is a byte-compiled Lisp function in
‘magit-section.el’.

(magit-section-ident-value OBJECT)

Return OBJECT’s value, making it constant and unique if necessary.

This is used to correlate different incarnations of the same
section, see ‘magit-section-ident’ and ‘magit-get-section’.

Sections whose values that are not constant and/or unique should
implement a method that return a value that can be used for this
purpose.


This is a generic function.

Implementations:

((section taxy-magit-section-section)) in ‘taxy-magit-section.el’.

Undocumented

((section magit-unpushed-section)) in ‘magit-log.el’.

"..@{push}" cannot be used as the value because that is
ambiguous if ‘push.default’ does not allow a 1:1 mapping, and
many commands would fail because of that.  But here that does
not matter and we need an unique value so we use that string
in the pushremote case.

((section magit-unpulled-section)) in ‘magit-log.el’.

"..@{push}" cannot be used as the value because that is
ambiguous if ‘push.default’ does not allow a 1:1 mapping, and
many commands would fail because of that.  But here that does
not matter and we need an unique value so we use that string
in the pushremote case.

((object eieio-default-superclass)) in ‘magit-section.el’.

Simply return the object itself.  That likely isn’t
good enough, so you need to implement your own method.

((section magit-section)) in ‘magit-section.el’.

Return the value unless it is an object.

Different object incarnations representing the same value then to
not be equal, so call this generic function on the object itself
to determine a constant value.

e.g. (magit-section-ident-value (magit-current-section)) evaluates to the [room session] vector that is the value of the section at point. That could probably be used to iterate over sections to find the one that matches the one that was at point before refreshing the buffer.

[alphapapa]

e.g. (magit-section-ident-value (magit-current-section)) evaluates to the [room session] vector that is the value of the section at point. That could probably be used to iterate over sections to find the one that matches the one that was at point before refreshing the buffer.

Regarding that, this commit I merged changes the section idents from pointing to the actual room and session structs to using just their ID strings: 2764c10

[alphapapa]

@phil-s I'm tidying up the list of issues for the v0.15 release. Do you think this should be merged for v0.15 or deferred for v0.16? (I generally try to keep down the number of changes that might introduce unexpected breakage, including ones I don't understand well.)

[phil-s]

Let's leave this for now. By accident it's ended up not being one of the patches I've been running locally, so I've not tested it recently and I don't want to have to review it at the moment.

[alphapapa]

Sure, I'll mark it for v0.16 then. Thanks.