Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'

From: Stefan Monnier <[email-masked]>
Date: Fri, 08 Nov 2024 14:20:20 -0500

> +  "i"                           #'mpc-describe-song)

What about using "d" (i.e. the "describe" mnemonic instead of the "info"
mnemonic)?

> +(defconst mpc-song-viewer-tagtypes
> +  (sort (append '("Bitrate" "Duration" "File" "Format") (mpc-cmd-tagtypes)))
> +  "Tag types available for use in `mpc-song-viewer-tags'.")

Hmm... running `mpc-cmd-tagtypes` when we load the file seems risky: we
may not know the `mpc-host` yet, or any other thing may go wrong.

> +(defun mpc-describe-song (&optional file)

Any reason we can't make the argument mandatory?

> +    (when tags
> +      (with-current-buffer (get-buffer-create buffer)
> +        (special-mode)
> +        (visual-line-mode)
> +        (let ((buffer-read-only nil))

This should bind `inhibit-read-only` instead.

> +      (select-window (display-buffer buffer '((display-buffer-reuse-window
> +                                               display-buffer-same-window)
> +                                              (reusable-frames . t)))))))

Why use `select-window + display-buffer` instead of `pop-to-buffer`?

        Stefan