Subject: Re: bug#78794: [PATCH] Pretiffy package-menu

From: Eli Zaretskii <[email-masked]>
Date: Mon, 16 Jun 2025 09:29:27 +0300

> From: Elijah Gabe Pérez <[email-masked]>
> Cc: [email-masked]
> Date: Sun, 15 Jun 2025 13:02:06 -0600
> 
> >> +*** New user option 'package-menu-mode-line'.
> >> +If non-nil, package-menu will display additional information about Total
> >> +packages installed, Total packages from all the package archives, Total
> >> +packages to upgrade and Total new packages available.
> >
> > Any reason not to have this by default, and drop the option?
> 
> This is enabled by default (I forgot to include it in the NEWS entry).
> The reason why i decided make it an option is because some user
> may find it annoying.

In what ways could it be annoying?

Philip, do you see any reason to make this customizable with a boolean
variable, and not via the usual mode-line customizations?  Will many
users want to remove this new information?

> >> +(defface package-mark-install-line
> >> +  '((((class color) (min-colors 88) (background light))
> >> +     :background "darkolivegreen1" :extend t)
> >> +    (((class color) (min-colors 88) (background dark))
> >> +     :background "seagreen" :extend t)
> >> +    (((class color) (min-colors 8))
> >> +     :background "green" :foreground "white" :extend t)
> >
> > If tty-color translation produces "green" from the two green shades
> > you define for 88+ color displays, then the last part is not needed.
> 
> Fine.
> 
> >> +    (t :inverse-video t :extend t))
> >
> > Isn't the mode line shown in inverse video by default in monochrome
> > case?  If so, this is not needed, either.
> 
> These faces are not for the mode-line, are for the package-menu buffer,
> specifically for highlight the line where a package was marked for
> install/delete.

Then I think inverse-video is too radical.  I'd suggest to use bold
(if supported) or underline (if supported), with inverse-video being
the last resort.

> +(defface package-mark-install-line
> +  '((((class color) (min-colors 88) (background light))
> +     :background "darkolivegreen1" :extend t)
> +    (((class color) (min-colors 88) (background dark))
> +     :background "seagreen" :extend t)
> +    (t :inverse-video t :extend t))

This will cause any display with fewer than 88 colors use the
inverse-video alternative.  I suggest to remove the min-colors
requirement, since it isn't needed.

> +(defface package-mark-delete-line
> +  '((((class color) (min-colors 88) (background light))
> +     :background "rosybrown1" :extend t)
> +    (((class color) (min-colors 88) (background dark))
> +     :background "indianred4" :extend t)
> +    (t :inverse-video t :extend t))

Likewise.

> +(defcustom package-menu-marks-indicators
> +  '((install . "I")
> +    (delete . "D"))
> +  "Alist indicators to indicate a package is marked to install or delete.
> +The value of each list must be in the formt: '(KIND . MARK) where KIND
> +is the kind mark performed (`install' or `delete') in package-menu and
> +MARK a string to use for mark the packages.  This currently support
> +marks for install and delete."

This doc string has several grammatical issues, and also lacks some
important information.  I suggest to rephrase as follows:

   "Indicators for packages to be installed or deleted.
 The value is an alist whose elements have the form (KIND . MARK),
 where KIND is the operation to perform, either `install' or `delete',
 and MARK is a string to indicate that the operation is pending for
 the package.  The MARK string should satisfy the requirements of the
 TAG argument of `tabulated-list-put-tag', which see.  It is displayed
 in the padding area of the package's line.

 Currently, only indicators for installing or deleting a package are
 supported."

> +  :type '(list (cons symbol string)
> +               (cons symbol string))

Can we come up with a meaningful :tag for the value?

> +(defun package-menu--overlay-line (face)
> +  "Highlight whole line with face FACE."
> +  (let ((ov (make-overlay (line-beginning-position)
> +                          (1+ (line-end-position)))))
> +    (overlay-put ov 'pkg-menu-ov t)
> +    (overlay-put ov 'evaporate t)
> +    (overlay-put ov 'face face)))

Does package-menu use other faces in overlays, and if so, should we
consider giving this overlay a non-default priority?

> +                  (total-help "Total packages of all package archives")
> +                  (installed-help "Total packages installed")
> +                  (upgrade-help "Total packages to upgrade")
> +                  (new-help "Total new packages available"))

Please use "Total number of SOMETHING" in these help strings.
Otherwise, it is not immediately clear what "Total" refers to.