Subject: Re: [bug#74801] [PATCH v2] gnu: home: services: Add home-mpv-service-type.

From: Ludovic Courtès <[email-masked]>
Date: Tue, 06 May 2025 11:44:27 +0200

Hi,

Tomas Volf <[email-masked]> writes:

> This commit adds a new service type to generate configuration file for the mpv
> media player.
>
> Originally I attempted to use Guix Records (via define-configuration) for
> this, but ran into the bug #74748, so I had to switch to procedures instead.
> The usage is (hopefully) sufficiently described in the documentation.  When
> the bug is resolved, I will update it to use define-configuration instead.

Is #74748 still a significant issue after
5b158ddca9425d79ea4ceb374003fe0f7e6bd336?

(This v2 already takes quite a few seconds to build, so it might be a
good starting point for profiling…)

> The full list of supported options is documented, however I decided to *not*
> document types and purpose for each individual fields.  While I had mostly
> working prototype to extract the documentation from mpv, once I realized it
> would be few 10k of lines added, I decided it is not worth it.  It would bloat
> the .texi file (by more than 50%), be hard to maintain and, in my opinion,
> would not provide enough value to justify that.  The current version seems
> like sane middle ground.

Sounds reasonable.

> Option to configure inputs (for mpv) will come later in a separate patch.

OK.

(I think this text is not meant for the commit log itself; you can
instead write it after the --- line.)

> +@deffn {Procedure} make-home-mpv-configuration
> +Return a new instance of @code{home-mpv-configuration}.  Available
> +keyword arguments are:
> +
> +@table @asis
> +@item @code{inherit} (default: @code{#t})
> +Inherit fields from an another instance.
> +
> +@item @code{global} (default: @code{(make-mpv-profile-configuration)})
> +The global configuration preceding all profiles.
> +
> +@item @code{profiles} (default: @code{()})
> +An alist containing configuration for any additional profiles to include
> +in the configuration file.
> +
> +@lisp
> +(make-home-mpv-configuration
> + #:profiles `((fullscreen . ,(make-mpv-profile-configuration
> +                              #:fullscreen #t))))
> +@end lisp
> +
> +@item @code{extra-config} (default: @code{#f})
> +Additional content to include in the configuration file.  It is placed
> +at the end of the file.

Two minor stylistic issues:

  1. We almost always drop the ‘make-’ bit from constructors, so I
     wonder whether we should do the same here (OTOH these procedures
     are different from the usual record constructors, so…).

  2. Please add question marks for boolean flags: #:inherit?,
     #:fullscreen?, etc.

> +;;; Generated code - START.

Could you include the code you used to generate the options?

Apart from that it LGTM.

Thanks,
Ludo’.