Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +  "i"                           #'mpc-describe-song)
>
> What about using "d" (i.e. the "describe" mnemonic instead of the "info"
> mnemonic)?

Done.

>> +(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.

Right you are. The tags are always the same anyway so I put a note
in the docstring and got rid of the risky constant.

>> +(defun mpc-describe-song (&optional file)
>
> Any reason we can't make the argument mandatory?

Nope.

>> +    (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.

Fixed.

>> +      (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`?

Elsewhere I was told not to use pop-to-buffer in new code and I
believe everything anyone tells me.