Package: emacs;
Reported by: Thierry Volpiatto <thievol <at> posteo.net>
Date: Sat, 4 Jan 2025 16:15:02 UTC
Severity: normal
Found in version 29.4
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Ship Mints <shipmints <at> gmail.com> To: Thierry Volpiatto <thievol <at> posteo.net> Cc: eliz <at> gnu.org, 75354 <at> debbugs.gnu.org Subject: bug#75354: (29.4; eww buffer is not displayed correctly when used from bookmark-jump ) Date: Mon, 24 Mar 2025 15:16:20 -0400
[Message part 1 (text/plain, inline)]
On Tue, Mar 18, 2025 at 2:12 PM Ship Mints <shipmints <at> gmail.com> wrote:
> On Tue, Mar 18, 2025 at 12:28 Ship Mints <shipmints <at> gmail.com> wrote:
>
>>
>> On Tue, Mar 18, 2025 at 11:55 AM Thierry Volpiatto <thievol <at> posteo.net>
>> wrote:
>>
>>> Ship Mints <shipmints <at> gmail.com> writes:
>>>
>>> > 1. ( ) text/plain (*) text/html
>>> >
>>> > On Tue, Mar 18, 2025 at 11:15 AM Thierry Volpiatto <thievol <at> posteo.net>
>>> wrote:
>>> >
>>> > Ship Mints <shipmints <at> gmail.com> writes:
>>> >
>>> > > 1. ( ) text/plain (*) text/html
>>> > >
>>> > > On Tue, Mar 18, 2025 at 2:55 AM Thierry Volpiatto <
>>> thievol <at> posteo.net> wrote:
>>> > >
>>> > > Sorry for late reply, was busy.
>>> > >
>>> > > Ship Mints <shipmints <at> gmail.com> writes:
>>> > >
>>> > > > On Sun, Mar 16, 2025 at 5:10 PM Ship Mints <
>>> shipmints <at> gmail.com> wrote:
>>> > > >
>>> > > > On Sat, Mar 15, 2025 at 10:18 AM Ship Mints <
>>> shipmints <at> gmail.com> wrote:
>>> > > >
>>> > > > On Sat, Mar 15, 2025 at 1:37 AM Thierry Volpiatto <
>>> thievol <at> posteo.net> wrote:
>>> > > >
>>> > > > Ship Mints <shipmints <at> gmail.com> 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.
-Stephane
[Message part 2 (text/html, inline)]
[0001-bookmark-jump-inhibit-window-actions-and-display-fun.patch (application/octet-stream, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.