GNU bug report logs - #78794
[PATCH] Pretiffy package-menu

Previous Next

Full log

View this message in rfc822 format

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78794 <at> debbugs.gnu.org, Elijah Gabe Pérez <eg642616 <at> gmail.com>
Subject: bug#78794: [PATCH] Pretiffy package-menu
Date: Mon, 16 Jun 2025 12:47:10 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
>> Cc: 78794 <at> debbugs.gnu.org
>> 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+

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.