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

From: Thierry Volpiatto <[email-masked]>
Date: Tue, 18 Mar 2025 15:22:35 +0000

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.

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

>   We'd still need a method to inhibit display-func in the interactive
> bookmark menu case.

No.

-- 
Thierry