Ship Mints <shipmints@gmail.com> writes:
>> From: Thierry Volpiatto <thievol@posteo.net>
>> Cc: Thierry Volpiatto <thievol@posteo.net>, 75354@debbugs.gnu.org
>> Date: Wed, 08 Jan 2025 13:52:32 +0000
>>
>> > This is contrary to what you originally wrote (with which I agree):
>>
>> Yes, after deeper search I found that `bookmark--jump-via` is behaving
>> like this AFAIU:
>> - It calls the handler which creates a new buffer related to bookmark.
>> - It then displays the current-buffer (the one before jumping to bmk) in
>> a window according to DISPLAY-FUNCTION and make the bookmark buffer current.
>>
>> This approach is OK as long as the handler fn doesn't try do do one part
>> of the job (window handling) itself, which is not the case at least with
>> eww and w3m. It is as well counter intuitive, DISPLAY-FUNCTION should
>> be called on the buffer generated by bookmark and not the contrary, so
>> this change makes the code simpler and easier to understand.
>>
>> > By contrast, the change you propose now will affect all the uses of
>> > bookmarks, everywhere,
>>
>> Yes, this is intended, in addition of fixing eww, it fixes w3m and also
>> the quit function of eww (actually broken).
>>
>> > which is riskier, given how many different variants of bookmark usage
>> > are out there.
>>
>> Tested here on many different kinds of bookmarks and work as expected
>> unlike the current code.
>
> It took me a while to find time to focus on some bugs with Emacs 31 in
> my workflow (reverting to Emacs 30 until I had time).
>
> I discovered that this change breaks all bookmark handlers that rely
> on managing window-state or window-configuration.
Which bookmark handlers?
At least these packages rely on bookmarks that store window state and their handlers restore window state. There are workarounds I have considered but they are distasteful, to say the least.
bufferlo
activities
burly
I help maintain bufferlo and this change broke bookmarks for all our users who have tried master, including me.
> I have no workaround yet this until we fix it, I have to do screwy
> things to make 'bookmark--jump-via' ignore 'save-window-excursion'.
>
> What's the rationale for wrapping 'bookmark-handle-bookmark' with
> 'save-window-excursion'?
1) The bookmark handler function setup a buffer.
2) When this buffer is ready we jump to this buffer with
DISPLAY-FUNCTION.
If you don't save the window excursion in 1) and buffer is not ready,
the DISPLAY-FUNCTION may run from the buffer that was current before
calling the handler function.
Emacs is single threaded. What does it mean for a buffer to "not be ready?"
Previously we were calling DISPLAY-FUNCTION on current-buffer, assuming
the handler fn had switched to a new buffer which should be current,
when the new buffer was not current we were swicthing to the
current-buffer the one that was current just before as explained above.
So this change has fixed not only eww but some other bookmarks (like
w3m).
Yes, you said in the original bug report.
> If this behavior is truly necessary (I'd like understand the use
> case), I suggest binding a new defvar to bind and trigger this
> behavior rather than affect all bookmark handlers. Unless eww can be
> fixed. It could handle window configuration management in its own
> bookmark handler.
That's the point, you should not handle window management in your
bookmark handler, this defeat bookmark--jump-via DISPLAY-FUNCTION.
Yes, we should handle window configurations. Bookmarks are not limited to just files or URLs. Any new limitations imposed on bookmarks that assume so makes bookmarks worse, not better.
This change also fixes old bugs where people were trying various things
in their bookmark handlers to get things right including polling or
running code under a timer etc...
Right. Those are some of the distasteful workarounds that Adam Porter had to resort to for activities in an effort to work around display-function and now he has to deal with window-save-excursion restoring the the window configuration *from our own calling sites*. If you think the new behavior should be limited to interactive use, I could perhaps understand that, but I still would suggest a defvar to control this so those of us who use bookmarks programmatically can still do so.