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

From: Philip Kaludercic <[email-masked]>
Date: Mon, 16 Jun 2025 12:47:10 +0000

Eli Zaretskii <[email-masked]> writes:

>> 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?

I agree that it doesn't seem like something that should be too annoying.
>From the description, it reminds me of Flymake, that also displays the
number of warnings and errors by default.  I haven't heard frequent
complaints about that.

>> >> +(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.

I would use the `highlight' face.  We could also add a semantic
"selection".

>> +(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."

What is the reason we would want to customise this in the first place?

>
> 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?

(I haven't check it, but...) is this the type of
`package-menu-marks-indicators'?  If so, we shouldn't document the user
option as an Alist.

>> +(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?

>From a quick search, we don't appear to use any overlays in package.el.

>> +                  (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.

1+