Package: emacs;
Reported by: david <davidimagid <at> gmail.com>
Date: Wed, 12 Mar 2025 13:25:02 UTC
Severity: normal
Found in version 31.0.50
Message #23 received at 76978 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: david <davidimagid <at> gmail.com> Cc: 76978 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>, Stefan Kangas <stefankangas <at> gmail.com> Subject: Re: [PATCH] package.el: Add built-in version check and active package marking Date: Tue, 18 Mar 2025 18:42:19 +0000
david <davidimagid <at> gmail.com> writes:
> Hello Philip and Emacs maintainers,
>
> I’m submitting this patch to enhance `describe-package` by adding
> information about the currently activated package and improving the
> built-in version check. Since "Other versions" already displays the
> current version of the package, I’ve added an "activated" marker to
> indicate which version is active in the current Emacs session.
>
> This patch for `package.el` introduces two improvements:
>
> 1. Built-in version check: The package menu now compares the built-in
> version of a package with the available version. If the available
> version is newer, the status is set to "available"; otherwise, it is
> marked as "built-in". This helps users identify when updates are
> available for built-in packages.
>
> 2. Activated packages in "Other versions": Packages listed in the
> "Other versions" section are now marked with ", activated" if they are
> currently active in the Emacs session. This makes it clear which
> versions are in use.
>
> I’d appreciate your feedback and suggestions for installing this patch
> into the master branch.
>
> Best regards,
> David D.
>
> From 8b552c182a0ba103fcad89f80abd4343b69bef9a Mon Sep 17 00:00:00 2001
> From: dimagid <dimagidve <at> gmail.com>
> Date: Tue, 18 Mar 2025 13:02:40 -0400
> Subject: [PATCH] package.el: Add built-in version check and active package
> marking
>
> This commit introduces two improvements:
>
> 1. Built-in version check: The package menu now compares the built-in
> version of a package with the available version. If the available
> version is newer, the status is set to "available"; otherwise, it is
> marked as "built-in". This helps users identify when an update is
> available for built-in packages.
>
> 2. Activated packages in "Other versions": Packages listed in the "Other
> versions" section are now marked with ", activated" if they are
> currently active in the Emacs session. This provides clarity on which
> versions are in use.
Could you rewrite this to fit the commit message style described in
the CONTRIBUTE file?
> ---
> lisp/emacs-lisp/package.el | 72 ++++++++++++++++++++++++++++----------
> 1 file changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index b9a8dacab15..7a615be2461 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -104,7 +104,7 @@
> ;; Vinicius Jose Latorre <viniciusjl.gnu <at> gmail.com>
> ;; Phil Hagelberg <phil <at> hagelb.org>
>
> -;;; ToDo:
> +;;; TODO:
If possible, avoid unrelated changes in the same patch.
>
> ;; - putting info dirs at the start of the info path means
> ;; users see a weird ordering of categories. OTOH we want to
> @@ -117,7 +117,6 @@
> ;; - give users a way to view a package's documentation when it
> ;; only appears in the .el
> ;; - use/extend checkdoc so people can tell if their package will work
> -;; - "installed" instead of a blank in the status column
> ;; - tramp needs its files to be compiled in a certain order.
> ;; how to handle this? fix tramp?
> ;; - maybe we need separate .elc directories for various emacs
> @@ -1230,7 +1229,7 @@ package-tar-file-info
> ((filename (tar-header-name (car tar-parse-info))))
> (let ((dirname (file-name-directory filename)))
> ;; The first file can be in a subdir: look for the top.
> - (if dirname (loop (directory-file-name dirname))
> + (if dirname (cl-loop (directory-file-name dirname))
Watch out, this is not an occurrence of cl-lib's loop, but the function
bound by named-let!
And also, if it were a instance of the old loop, this should be changed
in a separate patch.
> (file-name-as-directory filename)))))
> (desc-file (package--description-file dir-name))
> (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
> @@ -2977,25 +2976,34 @@ describe-package-1
> (package--print-email-button author)))
> (let* ((all-pkgs (append (cdr (assq name package-alist))
> (cdr (assq name package-archive-contents))
> - (let ((bi (assq name package--builtins)))
> - (if bi (list (package--from-builtin bi))))))
> - (other-pkgs (delete desc all-pkgs)))
> + (when-let* ((bi (assq name package--builtins)))
I would prefer `and-let*' here. Also the change confuses me a bit,
could you clarify what is going on with a comment?
> + (list (package--from-builtin bi)))))
> + (other-pkgs (remove desc all-pkgs)))
> (when other-pkgs
> (package--print-help-section "Other versions"
> (mapconcat (lambda (opkg)
> (let* ((ov (package-desc-version opkg))
> - (dir (package-desc-dir opkg))
> (from (or (package-desc-archive opkg)
> - (if (stringp dir) "installed" dir))))
> - (if (not ov) (format "%s" from)
> - (format "%s (%s)"
> - (make-text-button (package-version-join ov) nil
> - 'font-lock-face 'link
> - 'follow-link t
> - 'action
> - (lambda (_button)
> - (describe-package opkg)))
> - from))))
> + (if (package-desc-dir opkg)
> + "installed" "built-in")))
> + ;; Check if the package is currently activated and
> + ;; matches the described version.
> + (activated (and (member (package-desc-name opkg)
> + package-activated-list)
> + (equal ov (package-desc-version desc)))))
> + (format "%s (%s%s)"
> + (if ov
> + (make-text-button (package-version-join ov) nil
> + 'font-lock-face 'link
> + 'follow-link t
> + 'action
> + (lambda (_button)
> + (describe-package opkg)))
> + from)
> + from
> + ;; Append ", activated" if the package is currently
> + ;; activated.
> + (if activated ", activated" ""))))
Will this formatting be clear, and not cause any confusion if there is a
package called "activated"?
> other-pkgs ", ")
> ".")))
>
> @@ -3640,11 +3648,37 @@ package-status-avail-obso
>
> ;;; Package menu printing
>
> +(defun package-menu--check-built-in-version (pkg)
> + "Check if PKG is a built-in package and compare versions.
> +Return the appropriate status for PKG."
> + (let* ((status (package-desc-status pkg))
> + (name (package-desc-name pkg))
> + (version (package-desc-version pkg))
> + ;; Check if the package is built-in by looking it up in
> + ;; `package--builtins'.
> + (built-in-pkg (assq name package--builtins))
> + ;; If the package is built-in, get its version.
> + (built-in-version (if built-in-pkg
> + (package-desc-version
> + (package--from-builtin built-in-pkg)))))
> +
> + ;; If the package is built-in and marked as "available", compare
> + ;; versions.
> + (when (and built-in-pkg
> + (equal status "available"))
> + ;; If the built-in version is older than the available version,
> + ;; keep status as "available". Otherwise, set status to
> + ;; "built-in".
> + (if (version-list-< built-in-version version)
> + (setq status "available")
> + (setq status "built-in")))
> + status))
I think you can simplify the code here by just returning the values
directly:
(cond
((not (and built-in-pkg (equal status "available")))
status)
((version-list-< built-in-version version)
"available")
("built-in"))
> +
> (defun package-menu--print-info-simple (pkg)
> "Return a package entry suitable for `tabulated-list-entries'.
> PKG is a `package-desc' object.
> Return (PKG-DESC [NAME VERSION STATUS DOC])."
> - (let* ((status (package-desc-status pkg))
> + (let* ((status (package-menu--check-built-in-version pkg))
> (face (pcase status
> ("built-in" 'package-status-built-in)
> ("external" 'package-status-external)
> @@ -3676,7 +3710,7 @@ package-menu--print-info-simple
> 'font-lock-face face)
> ,(propertize status 'font-lock-face face)
> ,(propertize (or (package-desc-archive pkg) "")
> - 'font-lock-face face)
> + 'font-lock-face face)
Again, this looks like a unrelated change?
> ,(propertize (package-desc-summary pkg)
> 'font-lock-face 'package-description)])))
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.