bug#74362: 31.0.50; Add completions for font names

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 31.0.50


In the UI of `customize-face`, the font family name is the main one that
provides no help to fill it.
The simple patch below provides completion (I had to put the completion
function in `cus-edit.el` because of a circularity between
`cus-face.el` and `minibuffer.el` so we can't call
`completion-table-dynamic` while loading `cus-face.el`).

Comments/objections?


        Stefan


diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 8eba4270bcb..763dfc31fb8 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3425,6 +3425,12 @@ custom-visibility
 
 ;;; The `custom-face-edit' Widget.
 
+(defalias 'custom-face--font-completion 
+  (completion-table-dynamic
+   (lambda (_string)
+     (nconc (mapcar #'car face-font-family-alternatives)
+            (font-family-list)))))
+
 (define-widget 'custom-face-edit 'checklist
   "Widget for editing face attributes.
 The following properties have special meanings for this widget:
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index d0a1a66e29f..9a0ab4b47c7 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -48,6 +48,7 @@ custom-declare-face
 (defconst custom-face-attributes
   `((:family
      (string :tag "Font Family"
+	     :completions custom-face--font-completion
 	     :help-echo "Font family or fontset alias name."))
 
     (:foundry

bug#74362: 31.0.50; Add completions for font names

[Robert Pluim]

>>>>> On Thu, 14 Nov 2024 17:48:54 -0500, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    Stefan> Package: Emacs
    Stefan> Version: 31.0.50


    Stefan> In the UI of `customize-face`, the font family name is the main one that
    Stefan> provides no help to fill it.
    Stefan> The simple patch below provides completion (I had to put the completion
    Stefan> function in `cus-edit.el` because of a circularity between
    Stefan> `cus-face.el` and `minibuffer.el` so we can't call
    Stefan> `completion-table-dynamic` while loading `cus-face.el`).

    Stefan> Comments/objections?

You have trailing whitespace. Also it would be better if this was
wrapped in `completion-table-case-fold', I think (everyone disagrees
on how to write "mono").

Robert
-- 

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> You have trailing whitespace.

Thanks.  🙂

> Also it would be better if this was wrapped in
> `completion-table-case-fold', I think (everyone disagrees on how to
> write "mono").

So you're saying font family names are case-insensitive?
I was wondering about that and couldn't find it documented anywhere.


        Stefan

bug#74362: 31.0.50; Add completions for font names

[Robert Pluim]

>>>>> On Fri, 15 Nov 2024 11:50:14 -0500, Stefan Monnier <monnier@iro.umontreal.ca> said:
    Stefan> So you're saying font family names are case-insensitive?
    Stefan> I was wondering about that and couldn't find it documented anywhere.

No, Iʼm saying that the interface to select them should be
case-insensitive.

Robert
-- 

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>>>> On Fri, 15 Nov 2024 11:50:14 -0500, Stefan Monnier <monnier@iro.umontreal.ca> said:
>     Stefan> So you're saying font family names are case-insensitive?
>     Stefan> I was wondering about that and couldn't find it documented anywhere.
> No, Iʼm saying that the interface to select them should be
> case-insensitive.

I see, my last patch does use `completion-table-case-fold` so I think it
does provide the behavior you're asking for.


        Stefan

bug#74362: 31.0.50; Add completions for font names

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca
> Date: Thu, 14 Nov 2024 17:48:54 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> In the UI of `customize-face`, the font family name is the main one that
> provides no help to fill it.
> The simple patch below provides completion (I had to put the completion
> function in `cus-edit.el` because of a circularity between
> `cus-face.el` and `minibuffer.el` so we can't call
> `completion-table-dynamic` while loading `cus-face.el`).
> 
> Comments/objections?

I suggest to try this on a system that has a lot of fonts installed,
to make sure this doesn't slow down customize-face too much.  In
bug#74337 a user reported that font-family-list returned a list of no
fewer than 10775(!) families.  I wonder what will that do to this
command.

Also, the doc string of customize-face should be amended to say that
it offers completion on fonts.

Btw, how does one invoke completion on the face attributes which are
supposed to provide one, such as weight or colors?  M-C-i doesn't seem
to do it for me, or what am I missing?

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

>> Comments/objections?
>
> I suggest to try this on a system that has a lot of fonts installed,

I don't have such a thing.  My `font-family-list` has only 1k entries.
But I tried it with `corfu-mode` on a slow build (debug +
enable-checking) and I did not see any lag even though it presumably
rebuilds the list after each character I type.

This said, clearly, there is room for improvement via caching, so the
new patch below does that.

> to make sure this doesn't slow down customize-face too much.

It should make no difference in general: the list of completions is
computed only when a completion is actually requested on that field.

> Also, the doc string of customize-face should be amended to say that
> it offers completion on fonts.

Hmm... the docstring currently doesn't describe the UI at all (and
I can't think of a reason why it should).  Similarly the manual doesn't
seem to describe any part of the UI of `customize-face`.
Apparently, we presume the UI is self-explanatory.  If we want to fix
that, I think it's beyond the scope of this patch submission.

> Btw, how does one invoke completion on the face attributes which are
> supposed to provide one, such as weight or colors?  M-C-i doesn't seem
> to do it for me, or what am I missing?

M-TAB worked for me on the "Foregound" color name field.
For weights, I see a menu rather than a text field so there doesn't seem
to be a "completion" option.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: font-completion.patch --]
[-- Type: text/x-diff, Size: 1459 bytes --]

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 8eba4270bcb..0ef52948b39 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3425,6 +3425,25 @@ custom-visibility
 
 ;;; The `custom-face-edit' Widget.
 
+(defalias 'custom-face--font-completion
+  (let ((lastlist nil)
+        (lasttime nil)
+        (lastframe nil))
+    (completion-table-case-fold
+     (completion-table-dynamic
+      (lambda (_string)
+        ;; Cache the list for a max of 60s.
+        (let ((time (float-time)))
+         (if (and lastlist (eq (selected-frame) lastframe)
+                  (> 60 (- time lasttime)))
+             lastlist
+           (message "last list time: %s" (if lasttime (- time lasttime)))
+           (setq lasttime time)
+           (setq lastframe (selected-frame))
+           (setq lastlist
+                 (nconc (mapcar #'car face-font-family-alternatives)
+                        (font-family-list))))))))))
+
 (define-widget 'custom-face-edit 'checklist
   "Widget for editing face attributes.
 The following properties have special meanings for this widget:
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index d0a1a66e29f..9a0ab4b47c7 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -48,6 +48,7 @@ custom-declare-face
 (defconst custom-face-attributes
   `((:family
      (string :tag "Font Family"
+	     :completions custom-face--font-completion
 	     :help-echo "Font family or fontset alias name."))
 
     (:foundry

bug#74362: 31.0.50; Add completions for font names

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 74362@debbugs.gnu.org
> Date: Fri, 15 Nov 2024 12:33:14 -0500
> 
> > Also, the doc string of customize-face should be amended to say that
> > it offers completion on fonts.
> 
> Hmm... the docstring currently doesn't describe the UI at all (and
> I can't think of a reason why it should).  Similarly the manual doesn't
> seem to describe any part of the UI of `customize-face`.
> Apparently, we presume the UI is self-explanatory.  If we want to fix
> that, I think it's beyond the scope of this patch submission.

How about mentioning the completion in the help-echo of this field?

Btw, could set-face-font also benefit from this improvement?

> > Btw, how does one invoke completion on the face attributes which are
> > supposed to provide one, such as weight or colors?  M-C-i doesn't seem
> > to do it for me, or what am I missing?
> 
> M-TAB worked for me on the "Foregound" color name field.
> For weights, I see a menu rather than a text field so there doesn't seem
> to be a "completion" option.

Right.

Btw^2: When we select a font, do we have to change the Foundry as
well?  Or does Emacs disregard any non-matching foundries (if, for
example, the selected Family is not available with the specified
Foundry)?

> +        ;; Cache the list for a max of 60s.
> +        (let ((time (float-time)))
> +         (if (and lastlist (eq (selected-frame) lastframe)
> +                  (> 60 (- time lasttime)))

Maybe the 60 thing should be a defvar?

> --- a/lisp/cus-face.el
> +++ b/lisp/cus-face.el
> @@ -48,6 +48,7 @@ custom-declare-face
>  (defconst custom-face-attributes
>    `((:family
>       (string :tag "Font Family"
> +	     :completions custom-face--font-completion
>  	     :help-echo "Font family or fontset alias name."))

I'm curious: how come :foreground and :background don't need a
:completions specifier, and yet they provide completion OOTB?

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > Also, the doc string of customize-face should be amended to say that
>> > it offers completion on fonts.
>> Hmm... the docstring currently doesn't describe the UI at all (and
>> I can't think of a reason why it should).  Similarly the manual doesn't
>> seem to describe any part of the UI of `customize-face`.
>> Apparently, we presume the UI is self-explanatory.  If we want to fix
>> that, I think it's beyond the scope of this patch submission.
> How about mentioning the completion in the help-echo of this field?

I don't understand why you'd want that, so I don't know how it should be
phrased.  Can you point to some other help-echo which states such
a thing, for inspiration?

> Btw, could set-face-font also benefit from this improvement?

AFAICT `set-face-font` already has its own completion.

> Btw^2: When we select a font, do we have to change the Foundry as
> well?  Or does Emacs disregard any non-matching foundries (if, for
> example, the selected Family is not available with the specified
> Foundry)?

I can't remember the last time I used the foundry attribute, so you're
asking the wrong guy, sorry.  In my corner of the world, font foundries
are simply never used (and the info I can find about foundries of
installed fonts on my system is weird/poor(?), with non-descript names
suggesting nobody else cares about them).

>> +        ;; Cache the list for a max of 60s.
>> +        (let ((time (float-time)))
>> +         (if (and lastlist (eq (selected-frame) lastframe)
>> +                  (> 60 (- time lasttime)))
> Maybe the 60 thing should be a defvar?

I doubt it's worth the trouble, but if you insist I'll do that.

>> --- a/lisp/cus-face.el
>> +++ b/lisp/cus-face.el
>> @@ -48,6 +48,7 @@ custom-declare-face
>>  (defconst custom-face-attributes
>>    `((:family
>>       (string :tag "Font Family"
>> +	     :completions custom-face--font-completion
>>  	     :help-echo "Font family or fontset alias name."))
>
> I'm curious: how come :foreground and :background don't need a
> :completions specifier, and yet they provide completion OOTB?

Because those use the `color` widget defined in `wid-edit.el`:

    (define-widget 'color 'editable-field
      "Choose a color name (with sample)."
      :format "%{%t%}: %v (%{sample%})\n"
      :value-create 'widget-color-value-create
      :size (1+ (apply #'max 13 ; Longest RGB hex string.
                       (mapcar #'length (defined-colors))))
      :tag "Color"
      :value "black"
      :completions (defined-colors)
      :sample-face-get 'widget-color-sample-face-get
      :notify 'widget-color-notify
      :match #'widget-color-match
      :validate #'widget-color-validate
      :action 'widget-color-action)


- Stefan

bug#74362: 31.0.50; Add completions for font names

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 74362@debbugs.gnu.org
> Date: Sat, 16 Nov 2024 10:12:21 -0500
> 
> >> > Also, the doc string of customize-face should be amended to say that
> >> > it offers completion on fonts.
> >> Hmm... the docstring currently doesn't describe the UI at all (and
> >> I can't think of a reason why it should).  Similarly the manual doesn't
> >> seem to describe any part of the UI of `customize-face`.
> >> Apparently, we presume the UI is self-explanatory.  If we want to fix
> >> that, I think it's beyond the scope of this patch submission.
> > How about mentioning the completion in the help-echo of this field?
> 
> I don't understand why you'd want that, so I don't know how it should be
> phrased.  Can you point to some other help-echo which states such
> a thing, for inspiration?

It is unusual, yes.  But it is also unusual to expect completion in
any such field, since it is not the minibuffer.

Anyway, it isn't worth the argument, just forget it.  I can make such
a change myself if I think it's a good thing.

> > Btw, could set-face-font also benefit from this improvement?
> 
> AFAICT `set-face-font` already has its own completion.

Right, it just expects an unusual character as the first character of
a font's name.

> >> +        ;; Cache the list for a max of 60s.
> >> +        (let ((time (float-time)))
> >> +         (if (and lastlist (eq (selected-frame) lastframe)
> >> +                  (> 60 (- time lasttime)))
> > Maybe the 60 thing should be a defvar?
> 
> I doubt it's worth the trouble, but if you insist I'll do that.

Someone will ask for that even if I don't ;-)

> >> +	     :completions custom-face--font-completion
> >>  	     :help-echo "Font family or fontset alias name."))
> >
> > I'm curious: how come :foreground and :background don't need a
> > :completions specifier, and yet they provide completion OOTB?
> 
> Because those use the `color` widget defined in `wid-edit.el`:

Got it, thanks.

(Maybe we should have a 'font' widget, then, in some distant future.)

bug#74362: 31.0.50; Add completions for font names

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Someone will ask for that even if I don't ;-)

OK, pushed with that and an attempt at mentioning in the help-echo that
completion is available.
Thanks, closing.

>> Because those use the `color` widget defined in `wid-edit.el`:
> Got it, thanks.
> (Maybe we should have a 'font' widget, then, in some distant future.)

It's used a lot less often than color, and there's a lot more variety
(read: complexity) in the notion of "font" so I suspect "distant" is quite right.


        Stefan