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: Thu, 27 Mar 2025 08:11:39 -0400
[Message part 1 (text/plain, inline)]
On Mon, Mar 24, 2025 at 3:16 PM Ship Mints <shipmints <at> gmail.com> wrote: > 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. > 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
[Message part 2 (text/html, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.