Subject: Re: bug#65039: [PATCH] Add bookmark handler for shell-mode

From: Michael Albinus <[email-masked]>
Date: Thu, 13 Feb 2025 17:23:55 +0100

Ship Mints <[email-masked]> writes:

Hi,

> I cc'd Michael Albinus on this since it touches remote features and he
> may want to scrutinize what's in this patch. Testing this patch is
> what led me to help fix the bug in ansi-osc-directory-tracker.
>
> Let me know what you all think.

I gave it a cursory view. Nothing problematic; just some minor remarks.

> +(defcustom shell-bookmark-name-function #'shell-bookmark-name-from-default-directory
> +  "Function to generate a shell bookmark name.
> +The default is `shell-bookmark-name', which see."
> +  :group 'shell
> +  :type `(choice (function-item ,#'shell-bookmark-name-from-default-directory)
> +                 (function-item ,#'shell-bookmark-name-from-buffer-name)
> +                 function)

Wouldn't this be sufficient?

--8<---------------cut here---------------start------------->8---
  :type '(choice (function-item shell-bookmark-name-from-default-directory)
                 (function-item shell-bookmark-name-from-buffer-name)
                 function)
--8<---------------cut here---------------end--------------->8---

> +  (replace-regexp-in-string "\\(.*\\)<[[:digit:]]+>\\'"
> +                            "\\1"
> +                            (buffer-name)))

--8<---------------cut here---------------start------------->8---
  (replace-regexp-in-string "<[[:digit:]]+>\\'" "" (buffer-name)))
--8<---------------cut here---------------end--------------->8---

A further remark (don't know where to do it in your code): If you
bookmark a remote file name, I recommend to keep multi-hop file names
(let-bind tramp-show-ad-hoc-proxies to t). Otherwise, a remote file name
like "/ssh:host|sudo:host:/" would be saved in your bookmark-default-file
as "/sudo:host:/", which doesn't work in the next Emacs session.

> -Stephane

Best regards, Michael.