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

Previous Next

Full log

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

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

> From: Joost Kremers <joostkremers <at> fastmail.fm>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  arstoffel <at> gmail.com,  73775 <at> debbugs.gnu.org
> Date: Wed, 16 Oct 2024 18:19:41 +0200
> 
> 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. 😆 )

Adam, any further comments?  Or should I install this?

Previous Next

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