bug#59407: [PATCH] Add Colors to proced

[Eli Zaretskii]

> From: Laurence Warne <laurencewarne@gmail.com>
> Date: Fri, 25 Nov 2022 09:34:09 +0000
> Cc: luangruo@yahoo.com, 59407@debbugs.gnu.org, michael.albinus@gmx.de
> 
> I've attached a new patch, the changes this time are that the memory 
> thresholds now take a proportion
> rather than a fixed value, and some of the face defaults have been improved 
> to play nicer with 8 colour
> displays.

Thanks, see some comments below.

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -514,6 +514,22 @@ option) and can be set to nil to disable Just-in-time 
> Lock mode.
>  
>  * Changes in Emacs 29.1
>  
> +---
> +** New user option `proced-enable-color-flag` to enable coloring of proced 
> buffers
> +This option enables prompts some format functions to furnish their
> +respective process attributes with colors and effects in order to
> +make them easier to distinguish and highlight possible issues
> +(e.g. high memory usage), in a manner similar to htop.
> +
> +In particular, the current Emacs process id is highlighted purple in
> +both the process id and parent process id columns, session leaders
> +have their process ids underlined, larger memory sizes for rss are
> +highlighted in darker shades of orange, and the first word in the
> +args property (the executable) is highlighted in blue.
> +
> +Note this option is disabled by default and needs setting to a non-nil
> +value to take effect.

This is too long for a NEWS entry for such a minor feature.  Please make it
shorter.  Just saying that some fields of Proced display will be shown in
distinct colors, and mentioning the new defcustom to turn that on, should be
enough.

> +(defcustom proced-enable-color-flag nil
> +  "Non-nil means some process attributes will be displayed with color."

Our style is to avoid passive tense whenever possible:

  Non-nil means Proced should display some process attributes with color.

> +(defcustom proced-medium-memory-usage-threshold 0.5
> +  "The medium memory usage (in bytes) upper bound.

It is better to avoid such constructs.  Instead, say this:

  The upper bound for medium memory usage, relative to total memory.

Note that I removed "in bytes", since this is not the units in which this is
measured.

> +When `proced-enable-color-flag' is non-nil, rss values denoting a proportion
> +of memory less than this value, but greater than
> +`proced-low-memory-usage-threshold' will be displayed using the
                                      ^
Comma missing there.

> +`proced-memory-medium-usage' face.  rss values denoting a greater proportion

I think "rss" should be in all-caps, as "RSS".  Same for "VSIZE".

> +(defface proced-interruptible-sleep-status-code
> +  '((((class color)) (:foreground "DimGrey"))

Is this color visible well on both dark and light backgrounds?

> +    (t (:italic t)))
> +  "Face used for the interruptible sleep status code character \"S\"."
> +  :version "29.1")

Please mention Proced in all the doc strings of these faces, to make it
clear they are only used by Proced.

> +(defface proced-emacs-pid
> +  '((((class color) (min-colors 88)) (:foreground "purple"))
> +    (((class color)) (:foreground "magenta")))
> +  "Face for the pid of the current Emacs process."
                   ^^^
Please use "process ID", not just its abbreviation.

> +(defface proced-pid
> +  '((((class color) (min-colors 88)) (:foreground "#5085ef"))
> +    (((class color)) (:foreground "blue")))
> +  "Face for process ids."

"Face for process IDs", note the letter-case (here and elsewhere in the
patch).

> +(defface proced-cpu
> +  '((((class color) (min-colors 88)) (:foreground "#6d5cc3" :bold t))
> +    (t (:bold t)))
> +  "Face for process cpu utilization."

"CPU", in caps.

Thanks for working on this.