GNU bug report logs - #74801
[PATCH] gnu: home: services: Add home-mpv-service-type.

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Tomas Volf <~@wolfsden.cz>
Cc: Tanguy Le Carrour <tanguy <at> bioneland.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Gabriel Wicki <gabriel <at> erlikon.ch>, Andrew Tropin <andrew <at> trop.in>, Hilton Chain <hako <at> ultrarare.space>, 74801 <at> debbugs.gnu.org, Janneke Nieuwenhuizen <janneke <at> gnu.org>
Subject: [bug#74801] [PATCH v2] gnu: home: services: Add home-mpv-service-type.
Date: Tue, 06 May 2025 11:44:27 +0200

Hi,

Tomas Volf <~@wolfsden.cz> 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’.

Previous Next

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