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.