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

From: Ship Mints <[email-masked]>
Date: Mon, 17 Mar 2025 06:44:02 -0400

On Mon, Mar 17, 2025 at 6:41 AM Ship Mints <[email-masked]> wrote:

> On Mon, Mar 17, 2025 at 6:35 AM Ship Mints <[email-masked]> wrote:
>
>> 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.
>>
>> eww's handler could simply do this itself since it seems eww's url
>> opening behavior warrants this:
>>
>> (defun eww-bookmark-jump (bookmark)
>>   "Default bookmark handler for EWW buffers."
>>   (save-window-excursion
>>     (eww (bookmark-prop-get bookmark 'location))))
>>
>> I'd also suggest that we recommend adding an additional property to a
>> bookmark-handler function that could inhibit bookmark--jump-via's call to
>> display-func entirely.  In our case, when called programmatically, we use
>> #'ignore, but when the bookmark menu is used, we'd prefer to skip the
>> default pop-to-buffer-same-window.
>>
>
> Here's an example of another handler in the wild that handles its own
> save-window-excursion:
>
>
> https://github.com/emacsmirror/info-plus/blob/c6e26367abd2e99791f6e85fced2383de9ec605a/info%2B.el#L7025C1-L7039C98
>
> (defun Info-bookmark-jump (bookmark)
>   "Handler function for record returned by `Info-bookmark-make-record'.
> BOOKMARK is a bookmark name or a bookmark record.
>
> If `Info-bookmark-use-only-node-not-file-flag' is nil, and the
> recorded Info file is readable, then use it.  If not, then go to the
> recorded Info node in the manual for the current Emacs version."
>   (let* ((absfile    (bookmark-prop-get bookmark 'filename))
>          (file       (if (or Info-bookmark-use-only-node-not-file-flag
>  (not (file-readable-p absfile)))
>                          (file-name-nondirectory absfile)
>                        absfile))
>          (info-node  (bookmark-prop-get bookmark 'info-node))
>          (buf        (save-window-excursion ; Vanilla FIXME: doesn't work
> with frames!
>                        (Info-find-node file info-node) (current-buffer))))
>     (bookmark-default-handler `("" (buffer . ,buf) .
> ,(bookmark-get-bookmark-record bookmark)))))
>

And another that handles its own windows which the change to
bookmark--jump-via will break:

https://github.com/sunrise-commander/sunrise-commander/blob/16e6df7e86c7a383fb4400fae94af32baf9cb24e/sunrise-checkpoint.el#L83

(defun sunrise-checkpoint-handler (&optional bookmark)
  "Handler for a checkpoint BOOKMARK."
  (sunrise-ensure-running)
  (sunrise-select-window 'left)
  (let ((dirs (cdr (assq 'sunrise-directories (cdr bookmark))))
        (missing '()))
    (mapc (lambda (x)
            (if (file-directory-p x)
                (sunrise-save-aspect (dired x) (sunrise-bookmark-jump))
              (setq missing (cons sunrise-selected-window missing)))
            (sunrise-change-window))
          dirs)
    (if missing (sunrise-checkpoint-relocate bookmark (reverse missing)))))

https://github.com/sunrise-commander/sunrise-commander/blob/16e6df7e86c7a383fb4400fae94af32baf9cb24e/sunrise.el#L2371

(defun sunrise-change-window()
  "Change to the other Sunrise pane."
  (interactive)
  (sunrise-select-window (sunrise-other-side))
  (setq sunrise-selected-window-width nil))