[bug#55769] [PATCH] gnu: Add xwhite.

[bug#55769] [PATCH] gnu: Add xwhite.

[derekchuank]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

diff --git a/gnu/packages/xdisorg.scm b/gnu/packages/xdisorg.scm
index e5a98edb35..250860274d 100644
--- a/gnu/packages/xdisorg.scm
+++ b/gnu/packages/xdisorg.scm
@@ -54,6 +54,7 @@
 ;;; Copyright © 2021 jgart <jgart@dismail.de>
 ;;; Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
 ;;; Copyright © 2022 Jai Vetrivelan <jaivetrivelan@gmail.com>
+;;; Copyright © 2022 Derek Chuank <derekchuank@outlook.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1499,6 +1500,40 @@ (define-public redshift-wayland
 protocol.")
       (license license:gpl3+))))

+(define-public xwhite
+  (package
+    (name "xwhite")
+    (version "0.0.1")
+    (source
+     (origin
+       (method url-fetch)
+       (uri
+        (string-append "https://github.com/derekchuank/xwhite/"
+                       "releases/download/v" version
+                       "/xwhite-" version ".tar.gz"))
+       (sha256
+        (base32 "1gadgvl408zdl9drz6rb0dhq285a6w57bn94ql2sgcpp9mq7ym5q"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:tests? #f
+       #:phases
+       (modify-phases %standard-phases
+         (delete 'configure)
+         (replace 'install
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let ((out (assoc-ref outputs "out")))
+               (install-file "xwhite" (string-append out "/bin"))
+               #t))))))
+    (inputs
+     (list libxrandr))
+    (home-page "https://github.com/derekchuank/xwhite")
+    (synopsis "Adjust color balance")
+    (description "xwhite is a command line tool for adjusting color
+balance of screen.  It is based on xrandr's gamma correction and
+brightness adjustment.  Typically used for tuning color balance
+while setting color temperature.")
+    (license license:gpl2)))
+
 (define-public gammastep
   (package
     (name "gammastep")


[-- Attachment #2: Type: text/html, Size: 3650 bytes --]

[bug#55769] [PATCH] gnu: Add xwhite.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

derekchuank@outlook.com schreef op do 02-06-2022 om 15:39 [+0000]:
> +    (arguments
> +     `(#:tests? #f

Always add a comment on why tests are disabled.
In this case, probably because there are no tests?

> +       #:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +         (replace 'install
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out")))
> +               (install-file "xwhite" (string-append out "/bin"))
> +               #t))))))

Trailing #t haven't been necessary anymore since a long time

Also, a comment on the makefile.  In the makefile you do CC=gcc, but
that won't work when cross-compiling, because then TARGET-gcc (e.g. 
aarch64-linux-gnu-gcc) is required.  There are multiple ways to resolve
this, but given that you are upstream, I recommend switching to a build
system such as, say, meson[0].  meson (and autotools) take care of such
details for you (cross-compilation, also choosing the right
installation directory).

If you use meson or autotools, then you won't have to add any Guix
phases or do cross-compilation detection in the makefile, you then only
have to replace gnu-build-system by
meson-build-system.

A comment on the license: strictly speaking, you have licensed it as
‘any version of the GPL whatsoever’.  From the GPL itself:

> If the Program does not specify a version number of this License, you
> may choose any version ever published by the Free Software
> Foundation.

I doubt that's what you mean, so maybe document it in the README to be
GPL-2-or-later or GPL-2-only or GPL-2-OR-3-only?

A comment on the code itself: I recommend returning a non-zero value on
failure.  Also, all this is already implemented by another tool:

$ redshift -P -g 2:2:0.1 -O 10000

so to me there appears to be no need to write, maintain and package a
new tool.

[0] https://mesonbuild.com/

Greetings,
Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55769] [PATCH] gnu: Add xwhite.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

[Please keep 55769@debbugs.gnu.org in CC]

derekchuank@outlook.com schreef op vr 03-06-2022 om 01:33 [+0000]:
> Thank you for your patient.

Nitpick not relevant to the discussion: patient (adjective) -> patience
(noun).

> "Also, all this is already implemented by another tool:
> $ redshift -P -g 2:2:0.1 -O 10000
> so to me there appears to be no need to write, maintain and package a new tool."


derekchuank@outlook.com schreef op vr 03-06-2022 om 01:33 [+0000]:
> Setting gamma(redshift -g) correction in redshift is [...].

Ok, it fullfills a somewhat different niche.

> If you think it's worth a new package, I would fix the issues you
mentioned. Either way, I'm happy with the conversation with
> you.


> Setting gamma(redshift -g) correction in redshift is the same as `xrandr --output eDP-1 --gamma 2:2:0.1` in principle,
> which changes the gamma curve shape while keeps the curve two end point position still, resulting a strange
> color scheme with white color unchanged. And that's the problem setting color temperature(redshift -O) is trying to
> resolve, compressing the whole gamma curve, however, the RGB of white color of a fixed color temperature is fixed too,
> the above gamma correction can't change it. That's why I wrote xwhite, a simple but more flexible way to manual color
> balance, setting to a warm color tempature while enhancing green color to avoiding a red screen due to the backlight
> aging. I'm satisfied with it finally.
>
> If you think it's worth a new package, I would fix the issues you
> mentioned. Either way, I'm happy with the conversation with you. 

Given that it fullfills a different niche, a new package 'xwhite'
sounds reasonable to me.  Though maybe you can give a small comparison
of xwhite with redshift in the package description?


> +    (description "xwhite is a command line tool for adjusting color
> +balance of screen.  It is based on xrandr's gamma correction and
> +brightness adjustment.  Typically used for tuning color balance
> +while setting color temperature.")

xwhite -> @command{white} (TeXinfo markup).

adjusting color balance -> adjusting the color balance.

color can be replaced by colour or kept as-is.

It is based on xrandr's gamma correction and brightness adjustment.
-> maybe add the caveat ‘As such, it can only be used for X displays
and not Wayland displays?’

Typically used for tuning color balance while setting color
temperature.  --> It is typically used for tuning the color balance and
color temperature.

Maybe add: ‘It has a similar function as @command{redshift -P -g R:G:B
-O temperature}, but @command{xwhite} is more flexible in that it does
not keep the white color fixed.’

Greetings,
Maxime.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55769] [PATCH] gnu: Add xwhite.

[derekchuank]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

I've fixed all the issues you mentioned: adopt meson build system, add GPTv2-only license in README, fix return values in code and a new description. Again, Thank you very much for your detailed guidance and patience.


 gnu/packages/xdisorg.scm | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/gnu/packages/xdisorg.scm b/gnu/packages/xdisorg.scm
index e5a98edb35..1c09128c17 100644
--- a/gnu/packages/xdisorg.scm
+++ b/gnu/packages/xdisorg.scm
@@ -54,6 +54,7 @@
 ;;; Copyright © 2021 jgart <jgart@dismail.de>
 ;;; Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
 ;;; Copyright © 2022 Jai Vetrivelan <jaivetrivelan@gmail.com>
+;;; Copyright © 2022 Derek Chuank <derekchuank@outlook.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1499,6 +1500,37 @@ (define-public redshift-wayland
 protocol.")
       (license license:gpl3+))))

+(define-public xwhite
+  (package
+   (name "xwhite")
+   (version "0.0.2")
+   (source
+    (origin
+     (method url-fetch)
+     (uri
+      (string-append "https://github.com/derekchuank/xwhite/"
+                     "releases/download/v" version
+                     "/xwhite-" version ".tar.gz"))
+     (sha256
+      (base32 "0jbnlj5a91ib4anprmylqqnbv9wa73cr7fsc1s54df0a0w5yq8sz"))))
+   (build-system meson-build-system)
+   (arguments
+    `(#:tests? #f)) ; No test suite.
+   (native-inputs
+    (list pkg-config))
+   (inputs
+    (list libxrandr))
+   (home-page "https://github.com/derekchuank/xwhite")
+   (synopsis "Adjust the color balance")
+   (description "@command{xwhite} is a command line tool for adjusting the colour
+balance of screen.  It is based on xrandr's gamma correction and brightness adjustment.
+As such, it can only be used for X displays and not Wayland displays.  It is typically
+used for tuning the color balance and color temperature.  It has a similar function as
+@command{redshift -P -g R:G:B -O temperature}, but @command{xwhite} is more flexible
+in that it does not keep the white color fixed, suitable for setting the white color
+to an arbitrary balanced color.")
+   (license license:gpl2)))
+
 (define-public gammastep
   (package
     (name "gammastep")


[-- Attachment #2: Type: text/html, Size: 4256 bytes --]

[bug#55769] [PATCH] gnu: Add xwhite.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

user guix
usertags 55769 + reviewed looks-good reviewed-looks-good
thanks
(list available at <https://debbugs.gnu.org/cgi/pkgreport.cgi?tag=reviewed-looks-good&users=guix>)

derekchuank@outlook.com schreef op vr 03-06-2022 om 19:07 [+0000]:
> +     (uri
> +      (string-append "https://github.com/derekchuank/xwhite/"
> +                     "releases/download/v" version
> +                     "/xwhite-" version ".tar.gz"))
> +     (sha256
> +      (base32
> "0jbnlj5a91ib4anprmylqqnbv9wa73cr7fsc1s54df0a0w5yq8sz"))))


New package definition LGTM (looks good to me).
The hash matches, without any malware in the tarball.
Assuming it actually builds (I haven't checked) it should be fine to
apply.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

bug#55769: [PATCH] gnu: Add xwhite.

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> user guix
> usertags 55769 + reviewed looks-good reviewed-looks-good
> thanks
> (list available at <https://debbugs.gnu.org/cgi/pkgreport.cgi?tag=reviewed-looks-good&users=guix>)
>
> derekchuank@outlook.com schreef op vr 03-06-2022 om 19:07 [+0000]:
>> +     (uri
>> +      (string-append "https://github.com/derekchuank/xwhite/"
>> +                     "releases/download/v" version
>> +                     "/xwhite-" version ".tar.gz"))
>> +     (sha256
>> +      (base32
>> "0jbnlj5a91ib4anprmylqqnbv9wa73cr7fsc1s54df0a0w5yq8sz"))))
>
>
> New package definition LGTM (looks good to me).
> The hash matches, without any malware in the tarball.
> Assuming it actually builds (I haven't checked) it should be fine to
> apply.

Applied.  Thanks Derek and Maxime!

Ludo’.