bug#44068: 28.0.50; Faulty uses of tabulated-list-format

[Stephen Berman]

On Sun, 18 Oct 2020 15:01:31 -0700 Stefan Kangas <stefankangas@gmail.com> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> On trying out the new tabulated-list implementation of the bookmarks
>> list I noticed that, when clicking on the File header, the sort
>> indicator is not displayed, unlike with the Bookmark header.  Then I
>> noticed that the same thing happens in the tabulated buffer list (C-x
>> C-b).  Then I grepped for all uses of tabulated-list-format in the Emacs
>> sources and found the same problem in most of them.  The reason is that
>> in these modes the width of at least one of the columns is too narrow,
>> so that tabulated-list-init-header omits the indicator.  In most cases
>> the problematic column is the final one, but in a couple of cases there
>> are also non-final too narrow columns.  And I think these bugs are due
>> to a misleading description in tabulated-list-format's doc string.
>
> Your analysis sounds correct to me.
>
>> The attached patch corrects the doc string and the problematic uses of
>> tabulated-list-format.  The patch also fixes a typo and tries to
>> improve column alignment in timer-list-mode: this is one of the few
>> modes derived from tabulated-list-mode whose column widths didn't need
>> to be corrected, but the alignment seemed suboptimal; however, when
>> the header line uses a variable-pitch face, the alignment is still
>> suboptimal even with the patch, and I don't know how to fix that.
>
> Thanks for the patch.  I've tested it and it indeed fixes several bugs
> in this area.
>
> But it got me thinking: for the final column at least, maybe we should
> just make tabulated-list-mode work as advertised, and itself figure out
> that it should use this length?  That way, we would solve any bugs also
> for external packages that have been misled by the doc string.  Or would
> that have any downsides?

That was my first thought when I noticed the problem, and came up with
this patch to fix it:

diff --git a/lisp/emacs-lisp/tabulated-list.el 
b/lisp/emacs-lisp/tabulated-list.el
index b13f609f88..d6bec72ade 100644
--- a/lisp/emacs-lisp/tabulated-list.el
+++ b/lisp/emacs-lisp/tabulated-list.el
@@ -271,11 +271,12 @@ tabulated-list-init-header
        (button-props `(help-echo "Click to sort by column"
                        mouse-face header-line-highlight
                        keymap ,tabulated-list-sort-button-map))
+        (len (length tabulated-list-format))
        (cols nil))
     (if display-line-numbers
         (setq x (+ x (tabulated-list-line-number-width))))
     (push (propertize " " 'display `(space :align-to ,x)) cols)
-    (dotimes (n (length tabulated-list-format))
+    (dotimes (n len)
       (let* ((col (aref tabulated-list-format n))
             (label (nth 0 col))
             (width (nth 1 col))
@@ -293,7 +294,11 @@ tabulated-list-init-header
           (apply 'propertize
                  (concat label
                          (cond
-                          ((> (+ 2 (length label)) width) "")
+                          ((and (> (+ 2 (length label)) width)
+                                 (not (= (tabulated-list--column-number
+                                          (car tabulated-list-sort-key))
+                                         (1- len))))
+                                 "")
                           ((cdr tabulated-list-sort-key)
                             (format " %c"
                                     tabulated-list-gui-sort-indicator-desc))

But after I saw that final (rightmost) column in the problematic cases
was simply given too narrow a width, I thought it better to just change
that and clarify the doc.  But maybe you're right that the misleading
doc has affected third-party packages.

Steve Berman