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 16:45:46 -0400

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

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

Perhaps we can also consider introducing bookmark-record keys that
influence bookmark-jump behavior so specialized bookmark-record makers can
indicate what they need.