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

From: Ship Mints <[email-masked]>
Date: Thu, 27 Mar 2025 08:11:39 -0400

On Mon, Mar 24, 2025 at 3:16 PM Ship Mints <[email-masked]> wrote:

> On Tue, Mar 18, 2025 at 2:12 PM Ship Mints <[email-masked]> wrote:
>
>> On Tue, Mar 18, 2025 at 12:28 Ship Mints <[email-masked]> wrote:
>>
>>>
>>> On Tue, Mar 18, 2025 at 11:55 AM Thierry Volpiatto <[email-masked]>
>>> wrote:
>>>
>>>> Ship Mints <[email-masked]> writes:
>>>>
>>>> > 1.  ( ) text/plain          (*) text/html
>>>> >
>>>> > On Tue, Mar 18, 2025 at 11:15 AM Thierry Volpiatto <
>>>> [email-masked]> wrote:
>>>> >
>>>> >     Ship Mints <[email-masked]> writes:
>>>> >
>>>> >     > 1.  ( ) text/plain          (*) text/html
>>>> >     >
>>>> >     > On Tue, Mar 18, 2025 at 2:55 AM Thierry Volpiatto <
>>>> [email-masked]> wrote:
>>>> >     >
>>>> >     >     Sorry for late reply, was busy.
>>>> >     >
>>>> >     >     Ship Mints <[email-masked]> writes:
>>>> >     >
>>>> >     >     > On Sun, Mar 16, 2025 at 5:10 PM Ship Mints <
>>>> [email-masked]> wrote:
>>>> >     >     >
>>>> >     >     >     On Sat, Mar 15, 2025 at 10:18 AM Ship Mints <
>>>> [email-masked]> wrote:
>>>> >     >     >
>>>> >     >     >         On Sat, Mar 15, 2025 at 1:37 AM Thierry Volpiatto
>>>> <[email-masked]> wrote:
>>>> >     >     >
>>>> >     >     >             Ship Mints <[email-masked]> writes:
>>>> >     >     >
>>>> >     >     >             > I have workarounds that work only for the
>>>> most simplistic cases.  Many
>>>> >     >     >             > of our bookmarks themselves contain
>>>> embedded bookmarks and bookmark
>>>> >     >     >             > references (which are individually
>>>> addressable so can be used
>>>> >     >     >             > separately) with window-states we need to
>>>> restore in tab-bar tabs that
>>>> >     >     >             > they represent.
>>>> >     >     >
>>>> >     >     >             I don't really understand what your packages
>>>> are doing or are intended
>>>> >     >     >             doing, but FWICS in bufferlo: You are using
>>>> in some places
>>>> >     >     >             (bookmark-jump name #'ignore); why don't you
>>>> do all this work (restore
>>>> >     >     >             window-states in tab) in DISPLAY-FUNCTION
>>>> instead of using `ignore`?
>>>> >     >     >             Your handler would be much simpler by moving
>>>> the window-state-put and
>>>> >     >     >             alike calls in DISPLAY-FUNCTION:
>>>> >     >     >
>>>> >     >     >             (bookmark-jump name
>>>> #'your_function_restoring_window_or_frame_state)
>>>> >     >     >
>>>> >     >     >             Using (bookmark-jump name #'ignore) with all
>>>> the code that jump to
>>>> >     >     >             frame/tab etc... in the handler is just a
>>>> workaround to fix the previous
>>>> >     >     >             buggy behavior of bookmark--jump-via. IMO.
>>>> >     >     >
>>>> >     >     >             It would be good to start with a good example
>>>> or recipe to see if we can
>>>> >     >     >             find a good solution.
>>>> >     >     >
>>>> >     >     >         We need the bookmarks to work from the bookmark
>>>> menu where no display-function overrides are supported.
>>>> >     >     >
>>>> >     >     >         I suggest we add bookmark-record keys that
>>>> indicate to bookmark-jump to inhibit/or allow messing with
>>>> window-configurations.  The bufferlo bookmarks (and Adam's if he
>>>> >     wants)
>>>> >     >     would
>>>> >     >     >         contain these hint keys.
>>>> >     >     >
>>>> >     >     >         '(bookmark-jump-inhibit-window-actions . t) ; or
>>>> whatever we come up with
>>>> >     >     >
>>>> >     >     >         I can contrive an example, if necessary, but I
>>>> believe y'all get the point.  Nested bookmarks whose handlers expect their
>>>> window-configurations not to be messed with up
>>>> >     the
>>>> >     >     >         chain, will always be broken without additional
>>>> controls.
>>>> >     >     >
>>>> >     >     >     The attached patch implements such a scheme that
>>>> works for us, and is transparent to other bookmark uses.
>>>> >     >     >
>>>> >     >     > Perhaps we should restore bookmark--jump-via to its
>>>> previous behavior
>>>> >     >     > and better document the "rules of the road" for bookmark
>>>> handlers.
>>>> >     >     > For simple file- and point-based bookmarks, handlers need
>>>> to ensure
>>>> >     >     > that when they return, the selected window and current
>>>> buffer are
>>>> >     >     > what's intended.  For bookmark handlers that perform
>>>> other actions,
>>>> >     >     > those rules need not apply to leverage the bookmark
>>>> infrastructure.
>>>> >     >
>>>> >     >     What we could do is propose a more flexible solution so
>>>> that you could
>>>> >     >     use whatever you want for bookmark--jump-via; With what you
>>>> have proposed so
>>>> >     >     far, you still have the problem of DISPLAY-FUNCTION which
>>>> will always
>>>> >     >     run (I see there is comments about this problem in your
>>>> mentionned
>>>> >     >     packages), with the patch below you could define a
>>>> display-function
>>>> >     >     entry in your bookmark-record e.g. (display-function .
>>>> ignore) and then
>>>> >     >     add a special method for bookmark--jump-via:
>>>> >     >
>>>> >     >     (cl-defmethod bookmark--jump-via (bookmark-name-or-record
>>>> (_ (eql 'ignore)))
>>>> >     >       (do_watever_you_want_here)) ; e.g. run only the handler
>>>> fn.
>>>> >     >
>>>> >     >     NOTE: I used 'ignore as example but you could use whatever
>>>> you want.
>>>> >     >
>>>> >     >     Here the patch:
>>>> >     >
>>>> >     >     diff --git a/lisp/bookmark.el b/lisp/bookmark.el
>>>> >     >     index 99bb26e83cc..e594387f364 100644
>>>> >     >     --- a/lisp/bookmark.el
>>>> >     >     +++ b/lisp/bookmark.el
>>>> >     >     @@ -1259,7 +1259,7 @@ it to the name of the bookmark
>>>> currently being set, advancing
>>>> >     >        "Hook run after `bookmark-jump' jumps to a bookmark.
>>>> >     >      Useful for example to unhide text in `outline-mode'.")
>>>> >     >
>>>> >     >     -(defun bookmark--jump-via (bookmark-name-or-record
>>>> display-function)
>>>> >     >     +(cl-defgeneric bookmark--jump-via (bookmark-name-or-record
>>>> display-function)
>>>> >     >        "Handle BOOKMARK-NAME-OR-RECORD, then call
>>>> DISPLAY-FUNCTION.
>>>> >     >      DISPLAY-FUNCTION is called with the new buffer as argument.
>>>> >     >
>>>> >     >     @@ -1319,8 +1319,12 @@ DISPLAY-FUNC would be
>>>> `switch-to-buffer-other-window'."
>>>> >     >        ;; Don't use `switch-to-buffer' because it would let the
>>>> >     >        ;; window-point override the bookmark's point when
>>>> >     >        ;; `switch-to-buffer-preserve-window-point' is non-nil.
>>>> >     >     -  (bookmark--jump-via bookmark (or display-func
>>>> 'pop-to-buffer-same-window)))
>>>> >     >     +  (bookmark-jump-1 bookmark display-func))
>>>> >     >
>>>> >     >     +(defun bookmark-jump-1 (bookmark display-func)
>>>> >     >     +  (let ((dfn (or (bookmark-prop-get bookmark
>>>> 'display-function)
>>>> >     >     +                 display-func 'pop-to-buffer-same-window)))
>>>> >     >     +    (bookmark--jump-via bookmark dfn)))
>>>> >     >
>>>> >     >      ;;;###autoload
>>>> >     >      (defun bookmark-jump-other-window (bookmark)
>>>> >     >     @@ -2303,7 +2307,7 @@ the related behaviors of
>>>> `bookmark-save' and `bookmark-bmenu-save'."
>>>> >     >              (pop-up-windows t))
>>>> >     >          (delete-other-windows)
>>>> >     >          (switch-to-buffer (other-buffer) nil t)
>>>> >     >     -    (bookmark--jump-via bmrk 'pop-to-buffer)
>>>> >     >     +    (bookmark-jump-1 bmrk 'pop-to-buffer)
>>>> >     >          (bury-buffer menu)))
>>>> >     >
>>>> >     >     @@ -2317,7 +2321,7 @@ the related behaviors of
>>>> `bookmark-save' and `bookmark-bmenu-save'."
>>>> >     >        "Select this line's bookmark in other window, leaving
>>>> bookmark menu visible."
>>>> >     >        (interactive nil bookmark-bmenu-mode)
>>>> >     >        (let ((bookmark (bookmark-bmenu-bookmark)))
>>>> >     >     -    (bookmark--jump-via bookmark
>>>> 'switch-to-buffer-other-window)))
>>>> >     >     +    (bookmark-jump-1 bookmark
>>>> 'switch-to-buffer-other-window)))
>>>> >     >
>>>> >     >      (defun bookmark-bmenu-other-frame ()
>>>> >     >     @@ -2333,7 +2337,7 @@ The current window remains selected."
>>>> >     >        (interactive nil bookmark-bmenu-mode)
>>>> >     >        (let ((bookmark (bookmark-bmenu-bookmark))
>>>> >     >             (fun (lambda (b) (display-buffer b t))))
>>>> >     >     -    (bookmark--jump-via bookmark fun)))
>>>> >     >     +    (bookmark-jump-1 bookmark fun)))
>>>> >     >
>>>> >     >      (defun bookmark-bmenu-other-window-with-mouse (event)
>>>> >     >        "Jump to bookmark at mouse EVENT position in other
>>>> window.
>>>> >     >
>>>> >     >     Also I guess trying to call bookmark-jump-other-window and
>>>> friends is
>>>> >     >     failing with your special bookmarks, with this it would run
>>>> just as
>>>> >     >     bookmark-jump without (possible) errors.
>>>> >     >
>>>> >     >     WDYT?
>>>> >     >
>>>> >     > Thanks for the continuing discussion.
>>>> >     >
>>>> >     > The concept will work but it feels a bit over-engineered.
>>>> >
>>>> >     It is not, it is quite simple.
>>>> >
>>>> >     > The approach of ignoring save-window-excursion and display-func
>>>> via
>>>> >     > bookmark-record entries or using properties on the handler seem
>>>> less
>>>> >     > intrusive or a mix, if we feel that's appropriate.
>>>> >
>>>> >     I proposed this solution to help you cleaning up your code which
>>>> is full
>>>> >     of workarounds for the current behavior (prior 31).  Of course if
>>>> you
>>>> >     don't want to make an effort to update your code, what you
>>>> propose is
>>>> >     simpler (i.e. you have nothing to change on your side), but
>>>> generally we
>>>> >     (external emacs extensions developers) try to adapt ourselves to
>>>> Emacs
>>>> >     source and not the contrary.
>>>> >
>>>> > Thanks for the input.
>>>> >
>>>> > The idea that I "don't want to make an effort" is insulting.
>>>>
>>>> Sorry if you take it like this, it was not the intention.
>>>>
>>>> >   Perhaps a little less coffee.
>>>>
>>>> I don't drink coffee.
>>>>
>>>> >     > Why not just fix the eww bookmark handler to do its own
>>>> >     > save-window-excursion, again, rather than make a default
>>>> bookmark jump
>>>> >     > behavior policy change?
>>>> >
>>>> >     Because the problem is not just about eww, but more generally on
>>>> how
>>>> >     bookmark handlers work.
>>>> >
>>>> > Curious to know which other ones are broken?  I read eww and w3m.
>>>>
>>>> It is not only about eww AND w3m.  The point is not if things are broken
>>>> or not, it is to provide a good API for all bookmarks (and future kind
>>>> of bookmarks).
>>>>
>>>>
>>>> > What do the Emacs maintainers think about this as a matter of taste,
>>>> > easy adoption for other bookmark users, and idiomatic usage?
>>>>
>>>> Now Eli and other maintainers will decide what is the best for emacs.
>>>>
>>>
>>> You may not have seen it but there is already precedent for
>>> bookmark-handler properties in bookmark.el in bookmark-insert for the
>>> 'bookmark-inhibit property on a handler.  It could contain a list of
>>> inhibitions.
>>>
>>
>> I'll submit a patch to make that property into a list.  It was my code to
>> begin with and used only in shell-bookmark and I should have planned ahead.
>> Even if we don't use it for the above purposes.
>>
>
> I've attached a patch to bookmark-jump/bookmark--jump-via that supports
> inhibiting window actions and display function using the 'bookmark-inhibit
> handler function property list that Michael Albinus pushed to master last
> week.
>
> Usage for bookmark handler authors is simply:
>
> (put #'xxx-bookmark-handler 'bookmark-inhibit '(insert
>                                                 jump-window-actions
>                                                 jump-display-func))
>
> I tested this with a version of bufferlo waiting in the wings and it works
> nicely.
>
> If we want to add generic bookmark handler functions as a separate
> enhancement, I'm all for that in the future.
>

I'd like to see the latest approach I proposed installed.  We can add
fancier method overrides later.

We have a major version of bufferlo just about ready for release and I'd
like to ensure that it works well in Emacs 31/master.

We've already adopted many of the improvements in Emacs 31; including
expose-hidden-buffer, tab-bar fixes, window-state-get improvements,
define-ibuffer-op improvements.  For Emacs < 31, bufferlo advises
bookmark--jump-via to avoid display-func when called interactively but it
cannot easily avoid save-window-excursion in Emacs 31 without this patch.
Many users rely on bookmark-bmenu-list and bookmark-jump commands and we'd
love to be able to continue supporting those use cases.

Shall we, please?

-Stephane