GNU bug report logs - #74200
[PATCH] Add song viewer to 'mpc'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74200 in the body.
You can then email your comments to 74200 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add song viewer to 'mpc'
Date: Sun, 03 Nov 2024 21:26:52 -0600

Tags: patch

This adds a mpc-describe-song command which brings up a buffer
full of information about the selected song.

Message #8 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: 74200 <at> debbugs.gnu.org
Cc: monnier <at> iro.umontreal.ca
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Sun, 03 Nov 2024 21:29:53 -0600

[0001-Add-song-viewer-to-mpc-Bug-74200.patch (text/x-patch, attachment)]

[Message part 2 (text/plain, inline)]

john muhl <jm <at> pub.pink> writes:

> Tags: patch
>
> This adds a mpc-describe-song command which brings up a buffer
> full of information about the selected song.

Message #11 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: john muhl <jm <at> pub.pink>
Cc: monnier <at> iro.umontreal.ca, 74200 <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Mon, 04 Nov 2024 14:46:02 +0200

> Cc: monnier <at> iro.umontreal.ca
> From: john muhl <jm <at> pub.pink>
> Date: Sun, 03 Nov 2024 21:29:53 -0600
> 
> >From 861491d591a4e03bf4d4ceff001fd5036c0afef7 Mon Sep 17 00:00:00 2001
> From: john muhl <jm <at> pub.pink>
> Date: Sat, 19 Oct 2024 18:25:41 -0500
> Subject: [PATCH] Add song viewer to 'mpc' (Bug#74200)
> 
> * lisp/mpc.el (mpc-describe-song): New command.
> (mpc-mode-map): Bind "i" to 'mpc-describe-song'.
> (mpc-mode-menu): Add menu item.
> (mpc-secs-to-time): Ensure secs argument is an integer.
> (mpc-song-viewer-empty, mpc-song-viewer-tag):
> (mpc-song-viewer-value): New face.
> (mpc-song-viewer-tags): New option.
> (mpc-song-viewer-tagtypes): New constant.
> ---
>  etc/NEWS    |   7 ++++
>  lisp/mpc.el | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 111 insertions(+), 2 deletions(-)

Thanks, a few minor comments.

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -619,6 +619,13 @@ When non-nil, MPC will crossfade between songs for the specified number
>  of seconds.  Crossfading can be toggled using the command
>  'mpc-toggle-crossfade' or from the MPC menu.
>  
> +*** New command 'mpc-describe-song'.
> +This command displays information about the currently playing song or
> +song at point in the MPC-Songs buffer.  The list of tags to display can
> +be customized using the new user option 'mpc-song-viewer-tags' and the
> +appearance of the list with the new faces 'mpc-song-viewer-tag',
> +'mpc-song-viewer-value', and 'mpc-song-viewer-empty'.

This entry should be marked with "---", as we don't intend to document
this in any manual.

> +(defcustom mpc-song-viewer-tags
> +  '("Title" "Artist" "Album" "Performer" "Composer"
> +    "Date" "Duration" "Disc" "Track" "Genre" "File")
> +  "The list of tags to display with `mpc-describe-song'.

Each new defcustom should have a :version tag.

Message #14 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, 74200 <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Mon, 04 Nov 2024 07:25:18 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -619,6 +619,13 @@ When non-nil, MPC will crossfade between songs
>> for the specified number
>>  of seconds.  Crossfading can be toggled using the command
>>  'mpc-toggle-crossfade' or from the MPC menu.
>>  
>> +*** New command 'mpc-describe-song'.
>> +This command displays information about the currently playing song or
>> +song at point in the MPC-Songs buffer.  The list of tags to display can
>> +be customized using the new user option 'mpc-song-viewer-tags' and the
>> +appearance of the list with the new faces 'mpc-song-viewer-tag',
>> +'mpc-song-viewer-value', and 'mpc-song-viewer-empty'.
>
> This entry should be marked with "---", as we don't intend to document
> this in any manual.

Fixed. Added one to previous entry too which slipped through
unnoticed.

>> +(defcustom mpc-song-viewer-tags
>> +  '("Title" "Artist" "Album" "Performer" "Composer"
>> +    "Date" "Duration" "Disc" "Track" "Genre" "File")
>> +  "The list of tags to display with `mpc-describe-song'.
>
> Each new defcustom should have a :version tag.

Hmm. Not sure how that got lost in transmission but hopefully it
makes it through this time.

Thanks for looking.

[0001-Add-song-viewer-to-mpc-Bug-74200.patch (text/x-patch, attachment)]

Message #17 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: john muhl <jm <at> pub.pink>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74200 <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
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

Message #20 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74200 <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Tue, 12 Nov 2024 19:01:17 -0600

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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.

[0001-Add-song-viewer-to-mpc-Bug-74200.patch (text/x-patch, attachment)]

Message #23 received at 74200 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: john muhl <jm <at> pub.pink>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74200 <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Thu, 14 Nov 2024 10:45:57 -0500

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

Really?  AFAIK the one function in that family that's shunned is
`switch-to-buffer` (because its intention is unclear: sometimes it
means "change the content of this window (to that buffer)" and sometimes
it means "display this buffer (in the selected window)"),

Anyway, thanks, installed into `master`.


        Stefan

Message #28 received at 74200-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 74200-done <at> debbugs.gnu.org
Subject: Re: bug#74200: [PATCH] Add song viewer to 'mpc'
Date: Thu, 14 Nov 2024 12:08:47 -0500

> Anyway, thanks, installed into `master`.

Closing,


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.