Subject: Re: bug#75354: (29.4; eww buffer is not displayed correctly when used from bookmark-jump )

From: Ship Mints <[email-masked]>
Date: Thu, 13 Mar 2025 15:56:15 -0400

On Thu, Mar 13, 2025 at 3:48 PM Ship Mints <[email-masked]> wrote:

> On Thu, Mar 13, 2025 at 3:34 PM Ship Mints <[email-masked]> wrote:
>
>> On Thu, Mar 13, 2025 at 3:21 PM Thierry Volpiatto <[email-masked]>
>> wrote:
>>
>>> Ship Mints <[email-masked]> writes:
>>>
>>> >> From: Thierry Volpiatto <[email-masked]>
>>> >> Cc: Thierry Volpiatto <[email-masked]>,  [email-masked]
>>> >> 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.
>>
>
> In fact, it would be even better to optionally inhibit both
> save-window-excursion and display-function.  As it is, programmatically, we
> invoke bookmark-jump with #'ignore as the display-function.
>

One other point on this is that bufferlo and activities users, can use
bookmark-bmenu-list to invoke these bookmarks and the new
save-window-excursion wrapper will interfere with those uses also.  We
could recommend that those users stick to the native package invocation
functions but that would be a limitation that bookmarks did not themselves
have.