Re: [PATCH 4/8] gnu: Add zathura.

[Mark H Weaver <mhw@netris.org>]

Paul van der Walt <paul@denknerd.org> writes:

> * gnu/packages/pdf.scm (zathura): New variable.
> * gnu-system.am: Add zathura-plugindir-environment-variable.patch
> * gnu/packages/patches/zathura-plugindir-environment-variable.patch: Add patch
>   for zathura environment variable.

It's not important, but the way we usually write this is:

* gnu/packages/patches/zathura-plugindir-environment-variable.patch: New file.
* gnu-system.am: Add it.

We don't generally describe what new files are for in the commit log.
Such explanations belong in the files themselves.

> ---
>  gnu-system.am                                      |  4 +-
>  .../zathura-plugindir-environment-variable.patch   | 44 ++++++++++++++++++++
>  gnu/packages/pdf.scm                               | 47 ++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 2 deletions(-)
>  create mode 100644 gnu/packages/patches/zathura-plugindir-environment-variable.patch
>
> diff --git a/gnu-system.am b/gnu-system.am
> index e42e89a..5dda73a 100644
> --- a/gnu-system.am
> +++ b/gnu-system.am
> @@ -550,8 +550,8 @@ dist_patch_DATA =						\
>    gnu/packages/patches/xf86-video-trident-remove-mibstore.patch	\
>    gnu/packages/patches/xf86-video-vmware-glibc-2.20.patch	\
>    gnu/packages/patches/xfce4-panel-plugins.patch		\
> -  gnu/packages/patches/xmodmap-asprintf.patch
> -
> +  gnu/packages/patches/xmodmap-asprintf.patch                   \
> +  gnu/packages/patches/zathura-plugindir-environment-variable.patch
>  MISC_DISTRO_FILES = gnu/packages/javac.in

For consistency, please use tabs before the "\", and keep the blank line
above the MISC_DISTRO_FILES definition.

> +diff --git a/zathura/zathura.c b/zathura/zathura.c
> +index 589dd28..d3c9887 100644
> +--- a/zathura/zathura.c
> ++++ b/zathura/zathura.c
> +@@ -413,12 +413,24 @@ zathura_set_plugin_dir(zathura_t* zathura, const char* dir)
> +   g_return_if_fail(zathura != NULL);
> +   g_return_if_fail(zathura->plugins.manager != NULL);
> + 
> ++  // Get the new environment variable, if any.
> ++  const gchar* envvar_plugin_dir = g_getenv("ZATHURA_PLUGIN_DIR");
> ++
> +   if (dir != NULL) {
> ++    // This bit sets the plugin dir from command-line.
> +     girara_list_t* paths = girara_split_path_array(dir);
> +     GIRARA_LIST_FOREACH(paths, char*, iter, path)
> +     zathura_plugin_manager_add_dir(zathura->plugins.manager, path);
> +     GIRARA_LIST_FOREACH_END(paths, char*, iter, path);
> +     girara_list_free(paths);
> ++  } else if (envvar_plugin_dir != NULL) {
> ++    // Set plugins dir to the contents of environment variable
> ++    // ZATHURA_PLUGIN_DIR.
> ++    girara_list_t* paths = girara_split_path_array(envvar_plugin_dir);
> ++    GIRARA_LIST_FOREACH(paths, char*, iter, path)
> ++    zathura_plugin_manager_add_dir(zathura->plugins.manager, path);
> ++    GIRARA_LIST_FOREACH_END(paths, char*, iter, path);
> ++    girara_list_free(paths);
> +   } else {

Since the environment variable can hold a path with more than one
directory, it is misnamed.  How about ZATHURA_PLUGIN_PATH instead?

Also the code above could be simplified.
How about something like this: (untested)

--8<---------------cut here---------------start------------->8---
  g_return_if_fail(zathura != NULL);
  g_return_if_fail(zathura->plugins.manager != NULL);

  if (dir == NULL)
    dir = g_getenv("ZATHURA_PLUGIN_PATH");

  if (dir != NULL) {
    girara_list_t* paths = girara_split_path_array(dir);
    GIRARA_LIST_FOREACH(paths, char*, iter, path)
    zathura_plugin_manager_add_dir(zathura->plugins.manager, path);
    GIRARA_LIST_FOREACH_END(paths, char*, iter, path);
    girara_list_free(paths);
  } else {
--8<---------------cut here---------------end--------------->8---

This would be just three new lines, but it depends on 'dir' not being
used for anything else further down in the function, and it not being
freed.  If it is freed later, you could just wrap the 'g_getenv' with
'g_strdup'.

What do you think?

> diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
> index 5e24a43..1c5c193 100644
> --- a/gnu/packages/pdf.scm
> +++ b/gnu/packages/pdf.scm
> @@ -29,6 +29,9 @@
>    #:use-module (gnu packages compression)
>    #:use-module (gnu packages fontutils)
>    #:use-module (gnu packages ghostscript)
> +  #:use-module (gnu packages databases)
> +  #:use-module (gnu packages gettext)
> +  #:use-module (gnu packages backup)
>    #:use-module (gnu packages lesstif)
>    #:use-module (gnu packages image)
>    #:use-module (gnu packages pkg-config)
> @@ -146,6 +149,50 @@
>  
>  
>  
> +
> +(define-public zathura

Too many blank lines.

> +  (package
> +    (name "zathura")
> +    (version "0.3.2")
> +    (source (origin
> +              (method url-fetch)
> +              (uri
> +               (string-append "https://pwmt.org/projects/zathura/download/zathura-"
> +                              version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1qk5s7cyqp4l673yhma5igk9g24p5jyqyy81fdk7q7xjqlym19px"))
> +              (patches
> +               (list
> +                (search-patch "zathura-plugindir-environment-variable.patch")))))
> +    (native-inputs
> +     `(("pkg-config" ,pkg-config)
> +       ("gettext" ,gnu-gettext)))
> +    (native-search-paths
> +     (list (search-path-specification
> +            (variable "ZATHURA_PLUGIN_DIR")
> +            (files '("lib/zathura")))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:make-flags
> +       `(,(string-append "PREFIX=" (assoc-ref %outputs "out"))
> +         "CC=gcc" "COLOR=0")
> +       #:tests? #f ; Tests fail: "Gtk cannot open display".
> +       #:test-target "test"
> +       #:phases
> +       (alist-delete
> +        'configure
> +        %standard-phases)))

This fits easily on one line:

 #:phases (alist-delete 'configure %standard-phases)

> +    (home-page "https://pwmt.org/projects/zathura/")
> +    (synopsis "Lightweight keyboard-driven PDF viewer")
> +    (description "zathura is a highly customizable and functional document
> +viewer.  It provides a minimalistic and space saving interface as well as an
> +easy usage that mainly focuses on keyboard interaction.")

Probably the description should start with a capital letter, and I would
remove "and functional" and "as well as an easy usage" which sound like
marketing-speak.

> +    (inputs `(("girara" ,girara)
> +              ("sqlite" ,sqlite)
> +              ("gtk+" ,gtk+)))

Please move this next to 'native-inputs'.

Can you send an updated patch?

     Thanks!
       Mark