bug#73775: 30.0.90; vtable: can't handle 0 data rows

[Joost Kremers]

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
Description: Text Data

0002-Enable-inserting-new-objects-into-empty-vtable.patch
Description: Text Data

0003-vtable-allow-resetting-column-alignment-when-table-d.patch
Description: Text Data

0004-Update-vtable-documentation.patch
Description: Text Data