GNU bug report logs - #73775
30.0.90; vtable: can't handle 0 data rows

Previous Next

Full log

Message #26 received at 73775 <at> debbugs.gnu.org (full text, mbox):

From: Joost Kremers <joostkremers <at> fastmail.fm>
To: Adam Porter <adam <at> alphapapa.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, arstoffel <at> gmail.com, 73775 <at> debbugs.gnu.org
Subject: Re: bug#73775: 30.0.90; vtable: can't handle 0 data rows
Date: Wed, 16 Oct 2024 18:19:41 +0200

[Message part 1 (text/plain, inline)]

Hi Adam,

Thanks for looking at this.

On Tue, Oct 15 2024, Adam Porter wrote:
> Just a few minor suggestions:
>
> +  "Compute the display widths for TABLE.
> +CACHE is TABLE's cache data as returned by `vtable--compute-cache'."
> +  (let ((widths (seq-map-indexed
> +                 (lambda (column index)
> +                   (let ((width
> +                          (or
> +                           ;; Explicit widths.
> +                           (and (vtable-column-width column)
> +                                (vtable--compute-width table
> (vtable-column-width column)))
> +                           ;; If the vtable is empty and no explicit width
> is given,
> +                           ;; set its width to 0 and deal with it below.
> +                           (if (null cache)
>
> I may be mistaken (as I haven't examined all of the relevant code), but if
> CACHE is nil when this function is called, won't it always be null? If so,
> you could check its value once, at first, rather than each time through
> this loop.

Unfortunately, it has to be checked anew in every iteration, because it
determines for each column individually if we need to (temporarily) set its
width to 0. It also needs to keep the following `seq-max` from erroring out
(due to `seq-map` returning `nil`).

> +    ;; If there are any zero-width columns, divide the remaining window
> +    ;; width evenly over them.
> +    (when (member 0 widths)
> +      (let* ((combined-width (apply #'+ widths))
> +             (n-0cols (length (seq-keep #'zerop widths)))
>
> You could use SEQ-COUNT here, which would avoid consing a new list.

There may even be a better way. If I keep track of the number of zero-width
columns in the loop above, I don't even need to count them here. I've
implemented that in the updated patch attached to this message.

> @@ -484,3 +495,8 @@ vtable--compute-columns
>                                               table))
>             (setf (elt numerical index) nil)))
>         (vtable-columns table)))
> +    ;; Check if any columns have an explicit `align' property.
> +    (unless recompute
> +      (dolist (column (vtable-columns table))
> +        (if (vtable-column-align column)
> +            (setf (vtable-column--aligned column) t))))
>
> This could be a WHEN instead of a "one-armed IF".  :)

Yes, sirree! (I don't really agree with the "one-armed if should be
when"-stance, but I'd be hard-pressed to say when I prefer "if" and when
"when", and it's hardly a hill I want to die on, so I made the change. 😆 )

-- 
Joost Kremers
Life has its moments

[0001-Allow-empty-vtable.patch (text/x-patch, attachment)]

[0002-Enable-inserting-new-objects-into-empty-vtable.patch (text/x-patch, attachment)]

[0003-vtable-allow-resetting-column-alignment-when-table-d.patch (text/x-patch, attachment)]

[0004-Update-vtable-documentation.patch (text/x-patch, attachment)]

Previous Next

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