On Thu, Mar 13, 2025 at 3:56 PM Ship Mints wrote: > On Thu, Mar 13, 2025 at 3:48 PM Ship Mints wrote: > >> On Thu, Mar 13, 2025 at 3:34 PM Ship Mints wrote: >> >>> On Thu, Mar 13, 2025 at 3:21 PM Thierry Volpiatto >>> wrote: >>> >>>> Ship Mints writes: >>>> >>>> >> From: Thierry Volpiatto >>>> >> Cc: Thierry Volpiatto , 75354@debbugs.gnu.org >>>> >> Date: Wed, 08 Jan 2025 13:52:32 +0000 >>>> >> >>>> >> > This is contrary to what you originally wrote (with which I agree): >>>> >> >>>> >> Yes, after deeper search I found that `bookmark--jump-via` is >>>> behaving >>>> >> like this AFAIU: >>>> >> - It calls the handler which creates a new buffer related to >>>> bookmark. >>>> >> - It then displays the current-buffer (the one before jumping to >>>> bmk) in >>>> >> a window according to DISPLAY-FUNCTION and make the bookmark buffer >>>> current. >>>> >> >>>> >> This approach is OK as long as the handler fn doesn't try do do one >>>> part >>>> >> of the job (window handling) itself, which is not the case at least >>>> with >>>> >> eww and w3m. It is as well counter intuitive, DISPLAY-FUNCTION >>>> should >>>> >> be called on the buffer generated by bookmark and not the contrary, >>>> so >>>> >> this change makes the code simpler and easier to understand. >>>> >> >>>> >> > By contrast, the change you propose now will affect all the uses of >>>> >> > bookmarks, everywhere, >>>> >> >>>> >> Yes, this is intended, in addition of fixing eww, it fixes w3m and >>>> also >>>> >> the quit function of eww (actually broken). >>>> >> >>>> >> > which is riskier, given how many different variants of bookmark >>>> usage >>>> >> > are out there. >>>> >> >>>> >> Tested here on many different kinds of bookmarks and work as expected >>>> >> unlike the current code. >>>> > >>>> > It took me a while to find time to focus on some bugs with Emacs 31 in >>>> > my workflow (reverting to Emacs 30 until I had time). >>>> > >>>> > I discovered that this change breaks all bookmark handlers that rely >>>> > on managing window-state or window-configuration. >>>> >>>> Which bookmark handlers? >>> >>> >>> At least these packages rely on bookmarks that store window state and >>> their handlers restore window state. There are workarounds I have >>> considered but they are distasteful, to say the least. >>> >>> bufferlo >>> activities >>> burly >>> >>> I help maintain bufferlo and this change broke bookmarks for all our >>> users who have tried master, including me. >>> >>> > I have no workaround yet this until we fix it, I have to do screwy >>>> > things to make 'bookmark--jump-via' ignore 'save-window-excursion'. >>>> > >>>> > What's the rationale for wrapping 'bookmark-handle-bookmark' with >>>> > 'save-window-excursion'? >>>> >>>> 1) The bookmark handler function setup a buffer. >>>> 2) When this buffer is ready we jump to this buffer with >>>> DISPLAY-FUNCTION. >>>> >>>> If you don't save the window excursion in 1) and buffer is not ready, >>>> the DISPLAY-FUNCTION may run from the buffer that was current before >>>> calling the handler function. >>>> >>> >>> Emacs is single threaded. What does it mean for a buffer to "not be >>> ready?" >>> >>> Previously we were calling DISPLAY-FUNCTION on current-buffer, assuming >>>> the handler fn had switched to a new buffer which should be current, >>>> when the new buffer was not current we were swicthing to the >>>> current-buffer the one that was current just before as explained above. >>>> >>>> So this change has fixed not only eww but some other bookmarks (like >>>> w3m). >>>> >>> >>> Yes, you said in the original bug report. >>> >>> > If this behavior is truly necessary (I'd like understand the use >>>> > case), I suggest binding a new defvar to bind and trigger this >>>> > behavior rather than affect all bookmark handlers. Unless eww can be >>>> > fixed. It could handle window configuration management in its own >>>> > bookmark handler. >>>> >>>> That's the point, you should not handle window management in your >>>> bookmark handler, this defeat bookmark--jump-via DISPLAY-FUNCTION. >>>> >>> >>> Yes, we should handle window configurations. Bookmarks are not limited >>> to just files or URLs. Any new limitations imposed on bookmarks that >>> assume so makes bookmarks worse, not better. >>> >>> This change also fixes old bugs where people were trying various things >>>> in their bookmark handlers to get things right including polling or >>>> running code under a timer etc... >>>> >>> >>> Right. Those are some of the distasteful workarounds that Adam Porter >>> had to resort to for activities in an effort to work around >>> display-function and now he has to deal with window-save-excursion >>> restoring the the window configuration *from our own calling sites*. If >>> you think the new behavior should be limited to interactive use, I could >>> perhaps understand that, but I still would suggest a defvar to control this >>> so those of us who use bookmarks programmatically can still do so. >>> >> >> In fact, it would be even better to optionally inhibit both >> save-window-excursion and display-function. As it is, programmatically, we >> invoke bookmark-jump with #'ignore as the display-function. >> > > One other point on this is that bufferlo and activities users, can use > bookmark-bmenu-list to invoke these bookmarks and the new > save-window-excursion wrapper will interfere with those uses also. We > could recommend that those users stick to the native package invocation > functions but that would be a limitation that bookmarks did not themselves > have. > Perhaps we can also consider introducing bookmark-record keys that influence bookmark-jump behavior so specialized bookmark-record makers can indicate what they need.