GNU bug report logs - #29099
[PATCH] gnu: Add kodi-cli.

Previous Next

Full log

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

From: Marius Bakke <mbakke <at> fastmail.com>
To: Oleg Pykhalov <go.wigust <at> gmail.com>
Cc: 29099 <at> debbugs.gnu.org
Subject: Re: [bug#29099] [PATCH] gnu: Add kodi-cli.
Date: Thu, 02 Nov 2017 21:00:14 +0100

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

Oleg Pykhalov <go.wigust <at> gmail.com> writes:

> Hello Marius,
>
> Thank you for review!
>
> Marius Bakke <mbakke <at> fastmail.com> writes:
>
>>> +(define-public kodi-cli
>>> +  (let ((commit "104dc23b2a993c8e6db8c46f4f8bec24b146549b") ; Add support for
>>> +        (revision "1"))                                     ; `$HOME/.kodirc'.
>>> +    (package
>>> +      (name "kodi-cli")
>>> +      (version (string-append "1.1-" revision "." (string-take commit 7)))
>>> +      (source (origin
>>> +                (method git-fetch)
>>> +                (uri (git-reference (url "https://github.com/nawar/kodi-cli")
>>> +                                    (commit commit)))
>>> +                (sha256
>>> +                 (base32
>>> +                  "1xjhasc5gngfxpr1dlzy6q24w0wpdfjx12p43fanjppxw4i49n5p"))
>>> +                (file-name (string-append name "-" version "-checkout"))))
>>> +      (build-system trivial-build-system)
>>> +      (inputs `(("bash" ,bash)))
>>> +      (propagated-inputs `(("curl" ,curl)))
>>
>> There is only one reference to `curl` in the script, can you try to
>> substitute it with the absolute path and make it a regular input?  It's
>> good to avoid propagation when we can.
>
> Done.  I also added input mps-youtube and did the same.

Excellent.

>>> +@item Interactive and noninteractive volume control.
>>> +@item Interactive navigation.
>>> +@item Send text.
>>
>> What does this mean?  Arbitrary commands, or text notifications?
>
> From my experience this allows you do the following:
>
> 1. Open Videos.
> 2. Add videos…
> 3. Select <none>.
> 4. Enter path by sending a command via 'kodi-cli -t PATH'.
>
> Maybe call this item of the feature list as
> “Send text to the Kodi keyboard”?

"Send text to the Kodi keyboard" is much better, thanks!

> New patch is attached.

[...]

> +      (arguments
> +       `(#:modules ((guix build utils))
> +         #:builder
> +         (begin
> +           (use-modules (guix build utils))
> +           (copy-recursively (assoc-ref %build-inputs "source") ".")
> +           (substitute* "kodi-cli"
> +             (("/bin/bash") (string-append (assoc-ref %build-inputs "bash")
> +                                           "/bin/bash"))
> +             (("output=\\$\\((curl)" all curl)
> +              (string-append "output=$("
> +                             (assoc-ref %build-inputs "curl")
> +                             "/bin/" curl))
> +             (("play_youtube `(mpsyt)" all mpsyt)
> +              (string-append "play_youtube `"
> +                             (assoc-ref %build-inputs "mps-youtube")
> +                             "/bin/" mpsyt)))

FYI, you could also use (which "curl") etc from (guix build utils) here,
but I usually prefer this form.

[...]

> +@itemize
> +@item Play, pause, stop the current playing item.
> +@item Skip forward or backward in the current playing item.

These should be "currently playing".

> +@item Play or queue to the current list YouTube videos.

"... current list of YouTube videos."

(note the "of")

LGTM otherwise.  Don't forget to add a copyright line for yourself.

Thanks!

[signature.asc (application/pgp-signature, inline)]

Previous Next

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