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

[Stephen Berman <stephen.berman@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On Thu, 29 Oct 2020 18:06:16 -0700 Stefan Kangas <stefankangas@gmail.com> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> Try the attached patch; does it do what you're suggesting well enough?
>
> Yes, it looks better.
>
>> I meant that in some uses of tabulated-list-mode there are columns whose
>> default width is so narrow that, when selected for sorting, they are
>> immediately truncated (using the patch in my previous post), e.g. the
>> "Status" column in list-processes.  And the issue is exacerbated with
>> the current patch implementing your new suggestion: with its default
>> width "Status" is now truncated whether selected or not.
>
> But the "Status" field in process-menu-mode is 7, so why is it
> truncated when there is room?  "Status" is only 6 characters.
>
> I would expect that "Status" was not truncated in this case, IOW that:
>
> - Without a sorting indicator
>    - label width <= column width  => display it all
>    - label width >  column width  => truncate
>
> - With a sorting indicator
>    - label width <= column width  => display it all
>    - label width >  column width  => truncate
>
> For the last column, we should probably just show it all regardless of
> its width, and never truncate.
>
> Does the above make sense?

It seems to.  I did experiment a bit with the width when I made the
patch just for the selected column and thought the length I used there
gave the best results, but maybe that was too cautious.  Does this patch
give better results (it differs from the previous one only in the
lengths checked and used for truncating):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tabulated-list-init-header patch --]
[-- Type: text/x-patch, Size: 2699 bytes --]

diff --git a/lisp/emacs-lisp/tabulated-list.el b/lisp/emacs-lisp/tabulated-list.el
index b13f609f88..30577679f2 100644
--- a/lisp/emacs-lisp/tabulated-list.el
+++ b/lisp/emacs-lisp/tabulated-list.el
@@ -269,42 +269,48 @@ tabulated-list-init-header
   ;; FIXME: Should share code with tabulated-list-print-col!
   (let ((x (max tabulated-list-padding 0))
 	(button-props `(help-echo "Click to sort by column"
-			mouse-face header-line-highlight
-			keymap ,tabulated-list-sort-button-map))
+			          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))
+             (not-last-col (< n (1- len)))
 	     (label (nth 0 col))
+             (lablen (length label))
+             (pname label)
 	     (width (nth 1 col))
 	     (props (nthcdr 3 col))
 	     (pad-right (or (plist-get props :pad-right) 1))
              (right-align (plist-get props :right-align))
              (next-x (+ x pad-right width)))
+        (when (and (>= lablen 3) (> lablen width) not-last-col)
+          (setq label (truncate-string-to-width label (- lablen 1) nil nil t)))
 	(push
 	 (cond
 	  ;; An unsortable column
 	  ((not (nth 2 col))
-	   (propertize label 'tabulated-list-column-name label))
+	   (propertize label 'tabulated-list-column-name pname))
 	  ;; The selected sort column
 	  ((equal (car col) (car tabulated-list-sort-key))
 	   (apply 'propertize
-		  (concat label
-			  (cond
-			   ((> (+ 2 (length label)) width) "")
-			   ((cdr tabulated-list-sort-key)
+                  (concat label
+                          (cond
+                           ((and (< lablen 3) not-last-col) "")
+                           ((cdr tabulated-list-sort-key)
                             (format " %c"
                                     tabulated-list-gui-sort-indicator-desc))
-			   (t (format " %c"
+                           (t (format " %c"
                                       tabulated-list-gui-sort-indicator-asc))))
-		  'face 'bold
-		  'tabulated-list-column-name label
-		  button-props))
+                  'face 'bold
+                  'tabulated-list-column-name pname
+                  button-props))
 	  ;; Unselected sortable column.
 	  (t (apply 'propertize label
-		    'tabulated-list-column-name label
+		    'tabulated-list-column-name pname
 		    button-props)))
 	 cols)
         (when right-align

[-- Attachment #3: Type: text/plain, Size: 986 bytes --]


It might be better to take more width parameters into account, as
tabulated-list-print-col does, but that seems rather more complicated
and I currently don't have time to pursue it.

> BTW, perhaps we should add unit tests for all this, since it seems like
> we have a fair amount of use-cases to think about.  Maybe that could be
> a good excuse to make tabulated-list-init-header a bit more functional...

For sure (but again, I currently don't have time for that).

>> In such cases it may be better to make the default width of such
>> columns wider, which is what my first patch did.  Then with the
>> current patch, it will still be truncated when the column is
>> sufficiently narrowed with `{'.
>
> Yes, the default width of such columns should also be made wider, I
> think.  But first we should probably make sure that the underlying logic
> here is sound.

I agree

> Thanks again for working on this.

I wish I had more time for it but at the moment I don't.

Steve Berman