Re: ampc back on elpa?

[Stefan Monnier <monnier@iro.umontreal.ca>]

> I adapted my code to the elpa ampc version.  Patch follows.  I'm not a skilled
> lisp developer, feel free to comment.

Looks pretty good, except for one spot that doesn't seem right.  See below.


        Stefan


> @@ -569,6 +573,13 @@ modified."
>                   (0.4 playlist ,@pl-prop)
>                   (1.0 playlists)))
>         ,rs_b)
> +      ("Search view"
> +        ,(kbd "F")
> +       horizontal
> +       (0.4 vertical
> +            (6 status)
> +            (1.0 current-playlist ,@pl-prop))
> +       ,search-view)
>        ("Outputs view"
>         ,(kbd "L")
>         outputs :properties (("outputname" :title "Name" :min 10 :max 30)

Indentation looks odd here.  Maybe a mix of spaces and tabs?

>  (defmacro ampc-iterate-source-output (delimiter bindings pad-data &rest body)
> +  "delimiter = what delimit command results in mpd response"

Thank you for helping document the code.  Could you add a first line
describing of the general functionality?  Also put `delimiter` in
all-caps since that's the convention used in Emacs for function/macro
arguments in docstrings.

> @@ -1507,6 +1535,14 @@ modified."
>                           (ampc-send-command 'currentsong))
>                          (playlists
>                           (ampc-send-command 'listplaylists))
> +                        (search
> +                         (if (active-minibuffer-window) ;can't find a better way to check minibuffer
> +                             (when ampc-search-keywords
> +                               (ampc-send-command 'search nil "any" (ampc-quote ampc-search-keywords)))
> +                           (let ((search (read-from-minibuffer "Keywords: ")))
> +                             (unless (string= "" search)
> +                               (setq ampc-search-keywords search)
> +                               (ampc-send-command 'search nil "any" (ampc-quote search))))))
>                          (current-playlist
>                           (ampc-send-command 'playlistinfo))))))
>      (ampc-send-command 'status)

Can you rewrite it to something like

              (search
               (unless (active-minibuffer-window)
                 ;; Can't find a better way to check minibuffer
                 (let ((search (read-from-minibuffer "Keywords: ")))
                   (setq ampc-search-keywords
                         (unless (string= "" search) search))))
               (when ampc-search-keywords
                 (ampc-send-command 'search nil "any"
                                    (ampc-quote ampc-search-keywords))))

But in any case, this looks fishy.  `ampc-update` doesn't seem like
a good place to have user interaction.  Why do you need to
`read-from-minibuffer` *here* (BTW, I recommend you use `read-string`
instead)?  I mean, why can't you set ampc-search-keywords elsewhere?


        Stefan