From: Eli Zaretskii <eliz@gnu.org>
To: Joost Kremers <joostkremers@fastmail.fm>
Cc: adam@alphapapa.net, 73775@debbugs.gnu.org, arstoffel@gmail.com
Subject: bug#73775: 30.0.90; vtable: can't handle 0 data rows
Date: Thu, 31 Oct 2024 12:30:09 +0200 [thread overview]
Message-ID: <86msik36q6.fsf@gnu.org> (raw)
In-Reply-To: <86h69cf2aq.fsf@fastmail.fm> (message from Joost Kremers on Wed, 16 Oct 2024 18:19:41 +0200)
> From: Joost Kremers <joostkremers@fastmail.fm>
> Cc: Eli Zaretskii <eliz@gnu.org>, arstoffel@gmail.com, 73775@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. 😆 )
Thanks. I wanted to install this, but at least the first patch no
longer applies; could you please rebase and resubmit? When you do,
please mention the bug number in all the commit log messages.
> * lisp/emacs-lisp/vtable.el (vtable--compute-columns): if a column was
> not created with an explicit 'align' property, allow changing this
> property when the column data changes from numeric to non-numeric (or
> vice versa). This makes it possible to add data to an empty table,
> because in a table without data all columns are assumed to be numeric
> and right-aligned.
This should be reformatted according to our conventions: start with a
capital letter, leave two spaces between sentences, and not to indent
lines.
> +** 'make-vtable' can create an empty vtable
This should end in a period. Also, please mark the entry with "+++",
since the manuals were updated.
prev parent reply other threads:[~2024-10-31 10:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-12 17:22 bug#73775: 30.0.90; vtable: can't handle 0 data rows Augusto Stoffel
2024-10-12 18:24 ` Eli Zaretskii
2024-10-13 0:12 ` Adam Porter
2024-10-13 5:27 ` Eli Zaretskii
2024-10-13 8:00 ` Joost Kremers
2024-10-14 11:25 ` Joost Kremers
2024-10-16 4:12 ` Adam Porter
2024-10-16 16:19 ` Joost Kremers
2024-10-27 10:34 ` Eli Zaretskii
2024-10-29 23:39 ` Adam Porter
2024-10-31 10:30 ` Eli Zaretskii [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86msik36q6.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=73775@debbugs.gnu.org \
--cc=adam@alphapapa.net \
--cc=arstoffel@gmail.com \
--cc=joostkremers@fastmail.fm \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.