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:48:04 -0400

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.