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

From: Ship Mints <[email-masked]>
Date: Tue, 18 Mar 2025 11:02:35 -0400

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.  Perhaps that's a
matter of taste, and certainly not a dig at the idea.

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.

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?  We'd still need a method to inhibit display-func
in the interactive bookmark menu case.

-Stephane