[-- Attachment #1: Type: text/plain, Size: 561 bytes --] highlight-regexp asks for a face to highlight the matches. If it's a complicated file structure when one wants to apply multiple faces to demote, emphasise, etc. parts for readability, then the face selection could be much easier if beside listing the names they also had their properties (color,etc.) applied to them, so the user could see instantly which faces are suitable to apply to various parts of the file. Like read-color shows the color names with their actual colors, the selection of a face should also show the names with their properties applied. [-- Attachment #2: Type: text/html, Size: 870 bytes --]
[-- Attachment #1: Type: text/plain, Size: 272 bytes --] ndame <laszlomail@protonmail.com> writes: > Like read-color shows the color names with their actual colors, the > selection of a face should also show the names with their properties > applied. The patch below implements this, but it does lead to some strange effects: [-- Attachment #2: Type: image/png, Size: 95517 bytes --] [-- Attachment #3: Type: text/plain, Size: 1444 bytes --] That is, some faces have odd sizes, so the completion display may look a bit odd. But it does accurately tell you what the result is doing to be, so perhaps it's OK? Anybody have any opinions? diff --git a/lisp/hi-lock.el b/lisp/hi-lock.el index b70d4a7569..18e5f73369 100644 --- a/lisp/hi-lock.el +++ b/lisp/hi-lock.el @@ -727,11 +727,16 @@ hi-lock-read-face-name (cdr (member last-used-face hi-lock-face-defaults)) hi-lock-face-defaults)) face) - (if (and hi-lock-auto-select-face (not current-prefix-arg)) + (if (and hi-lock-auto-select-face (not current-prefix-arg)) (setq face (or (pop hi-lock--unused-faces) (car defaults))) - (setq face (completing-read - (format-prompt "Highlight using face" (car defaults)) - obarray 'facep t nil 'face-name-history defaults)) + (let ((completion-extra-properties + (list :annotation-function + (lambda (s) + (add-face-text-property + 0 (length s) (intern s) t s))))) + (setq face (completing-read + (format-prompt "Highlight using face" (car defaults)) + obarray 'facep t nil 'face-name-history defaults))) ;; Update list of un-used faces. (setq hi-lock--unused-faces (remove face hi-lock--unused-faces)) ;; Grow the list of defaults. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
Lars Ingebrigtsen <larsi@gnus.org> writes: > The patch below implements this, but it does lead to some strange > effects: > > That is, some faces have odd sizes, so the completion display may look a > bit odd. In addition to the odd sizes, some face names become completely unreadable (e.g. the ansi-color-* ones). > But it does accurately tell you what the result is doing to be, so > perhaps it's OK? Anybody have any opinions? Maybe using an affixation function (like read-char-by-name) would make things less jarring? E.g. fontifying the same string for every face (say "x" or "example") and using that as suffix (rather than prefix, to keep the face names aligned)? At any rate, I do think that showing the actual sizes (and colors, however unreadable) is a good feature to keep.
Kévin Le Gouguec <kevin.legouguec@gmail.com> writes: > In addition to the odd sizes, some face names become completely > unreadable (e.g. the ansi-color-* ones). Yes. But perhaps that's OK? You don't want to choose those faces here anyway. >> But it does accurately tell you what the result is doing to be, so >> perhaps it's OK? Anybody have any opinions? > > Maybe using an affixation function (like read-char-by-name) would make > things less jarring? E.g. fontifying the same string for every face > (say "x" or "example") and using that as suffix (rather than prefix, to > keep the face names aligned)? Hm, yes, that sounds like a good idea. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
[-- Attachment #1: Type: text/plain, Size: 559 bytes --] Lars Ingebrigtsen <larsi@gnus.org> writes: >> Maybe using an affixation function (like read-char-by-name) would make >> things less jarring? E.g. fontifying the same string for every face >> (say "x" or "example") and using that as suffix (rather than prefix, to >> keep the face names aligned)? > > Hm, yes, that sounds like a good idea. Serving suggestion attached, for discussion purposes; there might be smarter things to do wrt alignment (using :align-to specs? let-binding tab-width?). FWIW it looks pretty good with icomplete-vertical-mode, IMO. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: hi-lock.patch --] [-- Type: text/x-patch, Size: 1005 bytes --] diff --git a/lisp/hi-lock.el b/lisp/hi-lock.el index b70d4a7569..05584b8dc3 100644 --- a/lisp/hi-lock.el +++ b/lisp/hi-lock.el @@ -712,6 +712,13 @@ hi-lock-regexp-okay (error "Regexp cannot match an empty string")) (t regexp))) +(defun hi-lock-decorate-face-names (faces) + (mapcar + (lambda (face) + (list face "" (concat "\t" + (propertize "EXAMPLE" 'face (intern-soft face))))) + faces)) + (defun hi-lock-read-face-name () "Return face for interactive highlighting. When `hi-lock-auto-select-face' is non-nil, just return the next face. @@ -726,6 +733,8 @@ hi-lock-read-face-name (defaults (append hi-lock--unused-faces (cdr (member last-used-face hi-lock-face-defaults)) hi-lock-face-defaults)) + (completion-extra-properties + '(:affixation-function hi-lock-decorate-face-names)) face) (if (and hi-lock-auto-select-face (not current-prefix-arg)) (setq face (or (pop hi-lock--unused-faces) (car defaults)))
>> Like read-color shows the color names with their actual colors, the
>> selection of a face should also show the names with their properties
>> applied.
>
> The patch below implements this, but it does lead to some strange
> effects:
>
> That is, some faces have odd sizes, so the completion display may look a
> bit odd.
>
> But it does accurately tell you what the result is doing to be, so
> perhaps it's OK? Anybody have any opinions?
Whereas highlight-regexp can use all faces, the preferable faces are in
hi-lock-face-defaults, so they need to be displayed separately.
>>> Maybe using an affixation function (like read-char-by-name) would make
>>> things less jarring? E.g. fontifying the same string for every face
>>> (say "x" or "example") and using that as suffix (rather than prefix, to
>>> keep the face names aligned)?
>> Hm, yes, that sounds like a good idea.
>
> Serving suggestion attached, for discussion purposes; there might be
> smarter things to do wrt alignment (using :align-to specs? let-binding
> tab-width?).
>
> FWIW it looks pretty good with icomplete-vertical-mode, IMO.
So this is like `M-x list-faces-display'?
But OTOH, indeed maybe better to use completion-tab-width
like read-char-by-name does?
> > Like read-color shows the color names with their actual colors, the > > selection of a face should also show the names with their properties > > applied. > > The patch below implements this, but it does lead to some strange > effects: Icicles has done this since 2007. See here: https://www.emacswiki.org/emacs/Icicles_-_Candidates_with_Text_Properties (FWIW, I see this Note on that page: "2007-01-24 - RMS has agreed to fix Emacs in this way, so this will become possible in Emacs." AFAIK, that never happened.) ___ Whether face, color, etc. completion candidates are shown as, or accompanied by, such propertized samples is controlled by option `icicle-WYSIWYG-Completions-flag': ---8<------- icicle-WYSIWYG-Completions-flag is a variable defined in `icicles-opt.el'. Its value is "MMMM" Documentation: Non-nil means show candidates in `*Completions*' using WYSIWYG. This has an effect only for completion of things like faces, fonts, and colors. For face and color candidates, the particular non-nil value determines the appearance: * If t, the candidate displays its meaning: WYSIWYG. * If a string, the string is propertized and then appended to the candidate, to serve as a color swatch. Some commands might override a string value with different text. This is the case for `icicle-read-color-WYSIWYG', for instance: the color swatch text is always the color's RGB code. Note that, starting with Emacs 22, if this option is non-nil, then command `describe-face' does not use `completing-read-multiple', since that (non-Icicles) function does not support WYSIWYG candidates. ---8<-------
> > The patch below implements this, but it does lead to some strange
> > effects:
> >
> > That is, some faces have odd sizes, so the completion display may look a
> > bit odd.
>
> In addition to the odd sizes, some face names become completely
> unreadable (e.g. the ansi-color-* ones).
>
> > But it does accurately tell you what the result is doing to be, so
> > perhaps it's OK? Anybody have any opinions?
>
> Maybe using an affixation function (like read-char-by-name) would make
> things less jarring? E.g. fontifying the same string for every face
> (say "x" or "example") and using that as suffix (rather than prefix, to
> keep the face names aligned)?
>
> At any rate, I do think that showing the actual sizes (and colors,
> however unreadable) is a good feature to keep.
Yes.
15 years of practice (with Icicles) says this
enhancement request can be made to work well.
The key is to give users (and their code) the
control. It's perfectly possible to have the
face etc. name itself be shown without any face
property, but to be accompanied by a swatch
that shows the appearance.
I assume that's what you meant by referring to
`read-char-by-name'.
See the screen shots I linked to in my previous
message. Give users 3 ways to show candidates
that determine colors, fonts, and faces:
* No sample - just the plain candidate name
* Candidate name shown as sample
* Candidate name shown next to sample (swatch)
> > In addition to the odd sizes, some face names become completely > > unreadable (e.g. the ansi-color-* ones). > > Yes. But perhaps that's OK? You don't want to choose > those faces here anyway. Bad assumptions. 1. People use completion to do more than just make a choice. 2. The background of the *Completions* window isn't necessarily anything like the place where face you do choose is to be used. > >> But it does accurately tell you what the result is doing to be, so > >> perhaps it's OK? Anybody have any opinions? > > > > Maybe using an affixation function (like read-char-by-name) would make > > things less jarring? E.g. fontifying the same string for every face > > (say "x" or "example") and using that as suffix (rather than prefix, to > > keep the face names aligned)? > > Hm, yes, that sounds like a good idea. It's been helping users since 2007 - as has giving them the possibility to choose whether to use it or not. User control is important to such behavior.
> Serving suggestion attached, for discussion purposes; there might be
> smarter things to do wrt alignment (using :align-to specs? let-binding
> tab-width?).
It shouldn't be limited to hi-lock. It should be
for `read-face-name'.
And users should be able to control whether and in
what way it happens.
(And the same enhancement should be made for reading
colors and fonts.)
>>> Like read-color shows the color names with their actual colors, the
>>> selection of a face should also show the names with their properties
>>> applied.
>>
>> The patch below implements this, but it does lead to some strange
>> effects:
>>
>> That is, some faces have odd sizes, so the completion display may look a
>> bit odd.
>>
>> But it does accurately tell you what the result is doing to be, so
>> perhaps it's OK? Anybody have any opinions?
>
> Whereas highlight-regexp can use all faces, the preferable faces are in
> hi-lock-face-defaults, so they need to be displayed separately.
Also it would be nice to highlight faces displayed in the minibuffer
after fetching them with M-n M-n M-n ...
Kévin Le Gouguec <kevin.legouguec@gmail.com> writes: > Serving suggestion attached, for discussion purposes; there might be > smarter things to do wrt alignment (using :align-to specs? let-binding > tab-width?). Thanks; I've now pushed a variation on this to Emacs 29... Juri Linkov <juri@linkov.net> writes: > So this is like `M-x list-faces-display'? Right; I knew we had something similar to this somewhere. > But OTOH, indeed maybe better to use completion-tab-width > like read-char-by-name does? Yes. Drew Adams <drew.adams@oracle.com> writes: > It shouldn't be limited to hi-lock. It should be > for `read-face-name'. So I put the affixion thing into read-file-name and made hi-lock call read-file-name instead of doing the completion itself. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
[-- Attachment #1: Type: text/plain, Size: 1424 bytes --] Lars Ingebrigtsen <larsi@gnus.org> writes: > So I put the affixion thing into read-file-name and made hi-lock call > read-file-name instead of doing the completion itself. Great! A couple of nits: 1. Is there a reason why you went with (list "SAMPLE\t" "" face) instead of (list face "" \tSAMPLE)? "(elisp) Completion Variables" says that the completion goes first, then the prefix, then the suffix; I don't know how hard a rule that is, however I do think the latter option fares better when faced with variable heights. Cf. patch and screenshots: I like the second option better because IIUC the face names (which is what I'll be focusing my eyes on in order to know what to type; the face decoration I can see well enough in my peripheral vision) stand a better chance of being aligned. (Although I don't know if it's the hour getting late or something, but now that I actually try out C-u M-s h r I just… can't get any highlighting, with or without my patch on top. The completion works fine, but when I exit the minibuffer… nothing gets highlighted. Ugh. Will sleep on it) 2. As you can see on the screenshots, the prompt shows the default face twice. I think fixing this is as simple as removing format-prompt (patch also attached), but then again, I'm not thinking straight this evening it seems. Thanks for your time. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: affixation.patch --] [-- Type: text/x-patch, Size: 632 bytes --] diff --git a/lisp/faces.el b/lisp/faces.el index bb9b1e979f..cd6145cf68 100644 --- a/lisp/faces.el +++ b/lisp/faces.el @@ -1112,10 +1112,10 @@ read-face-name (lambda (faces) (mapcar (lambda (face) - (list (concat (propertize "SAMPLE" 'face face) - "\t") + (list face "" - face)) + (concat "\t" + (propertize "SAMPLE" 'face face)))) faces)))) aliasfaces nonaliasfaces faces) ;; Build up the completion tables. [-- Attachment #3: current.png --] [-- Type: image/png, Size: 99434 bytes --] [-- Attachment #4: suggested.png --] [-- Type: image/png, Size: 99488 bytes --] [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #5: defaults.patch --] [-- Type: text/x-patch, Size: 688 bytes --] diff --git a/lisp/hi-lock.el b/lisp/hi-lock.el index 53e6f779b3..c748bdebfb 100644 --- a/lisp/hi-lock.el +++ b/lisp/hi-lock.el @@ -730,9 +730,7 @@ hi-lock-read-face-name (if (and hi-lock-auto-select-face (not current-prefix-arg)) (setq face (or (pop hi-lock--unused-faces) (car defaults))) (setq face (symbol-name - (read-face-name - (format-prompt "Highlight using face" (car defaults)) - defaults))) + (read-face-name "Highlight using face" defaults))) ;; Update list of un-used faces. (setq hi-lock--unused-faces (remove face hi-lock--unused-faces)) ;; Grow the list of defaults.
Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
> (Although I don't know if it's the hour getting late or something,
> but now that I actually try out C-u M-s h r I just… can't get any
> highlighting, with or without my patch on top. The completion works
> fine, but when I exit the minibuffer… nothing gets highlighted.
>
> Ugh. Will sleep on it)
OK, turns out it really was well past bedtime for me; C-u sets the
regexp *group*, I don't know why, I was certain it was a toggle to
choose the face.
So please disregard this parenthesized tangent 🙃 and let the patches
and screenshots speak for themselves. Sorry for the noise.
Kévin Le Gouguec <kevin.legouguec@gmail.com> writes: > Great! A couple of nits: > > 1. Is there a reason why you went with (list "SAMPLE\t" "" face) instead > of (list face "" \tSAMPLE)? "(elisp) Completion Variables" says that > the completion goes first, then the prefix, then the suffix; I don't > know how hard a rule that is, however I do think the latter option > fares better when faced with variable heights. It looked really bad with the face names first, since they have wildly differing lengths. > 2. As you can see on the screenshots, the prompt shows the default face > twice. I think fixing this is as simple as removing format-prompt > (patch also attached), but then again, I'm not thinking straight this > evening it seems. (hi-lock-read-face-name) doesn't seem to display the default twice in an emacs -Q? -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
Lars Ingebrigtsen <larsi@gnus.org> writes: > Kévin Le Gouguec <kevin.legouguec@gmail.com> writes: > >> Great! A couple of nits: >> >> 1. Is there a reason why you went with (list "SAMPLE\t" "" face) instead >> of (list face "" \tSAMPLE)? "(elisp) Completion Variables" says that >> the completion goes first, then the prefix, then the suffix; I don't >> know how hard a rule that is, however I do think the latter option >> fares better when faced with variable heights. > > It looked really bad with the face names first, since they have wildly > differing lengths. … and so the samples would get misaligned? (Trying to make sure I understand the issue) IMO that's less annoying than the names being misaligned (since that throws me off slightly when I'm looking them over to figure out what I need to type), but YMMV. (I'm still intrigued by the documentation of :affixation-function though: it seems to me that it asks for (list COMPLETION PREFIX SUFFIX), but obviously your code works…) >> 2. As you can see on the screenshots, the prompt shows the default face >> twice. I think fixing this is as simple as removing format-prompt >> (patch also attached), but then again, I'm not thinking straight this >> evening it seems. > > (hi-lock-read-face-name) doesn't seem to display the default twice in an > emacs -Q? It does here 😕 ./src/emacs -Q M-: emacs-repository-version "1228ec3e1d7657c9eb50184719410f37ed0eb750" M-: (hi-lock-read-face-name) Highlight using face (default hi-yellow) (default hi-yellow):
> It looked really bad with the face names first, since they have wildly > differing lengths. I disagree that it looks really bad. https://www.emacswiki.org/emacs/Icicles_-_Candidates_with_Text_Properties#FaceCandidatesWithSwatches And it's definitely better for usability, including scanning names when candidates are sorted alphabetically. But if you think it looks really bad then file a separate bug report (enhancement request) to be able to align parts of completion candidates. Surely this isn't limited to face names and color names.
Kévin Le Gouguec <kevin.legouguec@gmail.com> writes: > … and so the samples would get misaligned? (Trying to make sure I > understand the issue) Yes. > IMO that's less annoying than the names being misaligned (since that > throws me off slightly when I'm looking them over to figure out what I > need to type), but YMMV. It's what we do in `C-x 8 RET TAB', for much the same rasons. >> (hi-lock-read-face-name) doesn't seem to display the default twice in an >> emacs -Q? > > It does here 😕 > > ./src/emacs -Q > M-: emacs-repository-version > "1228ec3e1d7657c9eb50184719410f37ed0eb750" > M-: (hi-lock-read-face-name) > Highlight using face (default hi-yellow) (default hi-yellow): Oh, in the prompt -- I thought you meant in the TAB completions. I must be going blind. Fixed now. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
[-- Attachment #1: Type: text/plain, Size: 797 bytes --] >> It shouldn't be limited to hi-lock. It should be >> for `read-face-name'. > > So I put the affixion thing into read-file-name and made hi-lock call > read-file-name instead of doing the completion itself. This has such regression that before it supported the following workflow: in the minibuffer 'M-n M-n ...' used to pull hi-lock faces from the ordered "future history" list. But read-face-name doesn't support this feature. If highlight-regexp should use read-face-name, then this feature could be added to read-face-name. Here is the simplest patch, but it would be a tough task to explain the need of an additional DEFAULTS arg in the documentation. An alternative would be to rewrite read-face-name and keep cdr of the existing arg DEFAULT for the future history of the minibuffer. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: read-face-name-defaults.patch --] [-- Type: text/x-diff, Size: 1618 bytes --] diff --git a/lisp/faces.el b/lisp/faces.el index bb9b1e979f..7a1fae3669 100644 --- a/lisp/faces.el +++ b/lisp/faces.el @@ -1065,7 +1065,7 @@ invert-face (defvar crm-separator) ; from crm.el -(defun read-face-name (prompt &optional default multiple) +(defun read-face-name (prompt &optional default multiple defaults) "Read one or more face names, prompting with PROMPT. PROMPT should not end in a space or a colon. @@ -1129,7 +1129,7 @@ read-face-name (dolist (face (completing-read-multiple prompt (completion-table-in-turn nonaliasfaces aliasfaces) - nil t nil 'face-name-history default)) + nil t nil 'face-name-history defaults)) ;; Ignore elements that are not faces ;; (for example, because DEFAULT was "all faces") (if (facep face) (push (intern face) faces))) diff --git a/lisp/hi-lock.el b/lisp/hi-lock.el index 8df73b1d37..a6760580f8 100644 --- a/lisp/hi-lock.el +++ b/lisp/hi-lock.el @@ -729,7 +729,8 @@ hi-lock-read-face-name face) (if (and hi-lock-auto-select-face (not current-prefix-arg)) (setq face (or (pop hi-lock--unused-faces) (car defaults))) - (setq face (symbol-name (read-face-name "Highlight using face" defaults))) + (setq face (symbol-name (read-face-name "Highlight using face" + (car defaults) t (cdr defaults)))) ;; Update list of un-used faces. (setq hi-lock--unused-faces (remove face hi-lock--unused-faces)) ;; Grow the list of defaults.
Juri Linkov <juri@linkov.net> writes: > This has such regression that before it supported the following workflow: > in the minibuffer 'M-n M-n ...' used to pull hi-lock faces from the ordered > "future history" list. But read-face-name doesn't support this feature. > > If highlight-regexp should use read-face-name, then this feature > could be added to read-face-name. Here is the simplest patch, > but it would be a tough task to explain the need of an additional > DEFAULTS arg in the documentation. An alternative would be to > rewrite read-face-name and keep cdr of the existing arg DEFAULT > for the future history of the minibuffer. Hm, I assumed that that was what read-face-name did (since it allows DEFAULT to be a list). -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
[-- Attachment #1: Type: text/plain, Size: 786 bytes --] >> This has such regression that before it supported the following workflow: >> in the minibuffer 'M-n M-n ...' used to pull hi-lock faces from the ordered >> "future history" list. But read-face-name doesn't support this feature. >> >> If highlight-regexp should use read-face-name, then this feature >> could be added to read-face-name. Here is the simplest patch, >> but it would be a tough task to explain the need of an additional >> DEFAULTS arg in the documentation. An alternative would be to >> rewrite read-face-name and keep cdr of the existing arg DEFAULT >> for the future history of the minibuffer. > > Hm, I assumed that that was what read-face-name did (since it allows > DEFAULT to be a list). Then read-face-name should be improved to support a list of defaults: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: read-face-name-defaults.patch --] [-- Type: text/x-diff, Size: 1331 bytes --] diff --git a/lisp/faces.el b/lisp/faces.el index bb9b1e979f..c03a0ff8c1 100644 --- a/lisp/faces.el +++ b/lisp/faces.el @@ -1081,6 +1081,7 @@ read-face-name element of DEFAULT is returned. If DEFAULT isn't a list, but MULTIPLE is non-nil, a one-element list containing DEFAULT is returned. Otherwise, DEFAULT is returned verbatim." + (let (defaults) (unless (listp default) (setq default (list default))) (when default @@ -1090,7 +1091,7 @@ read-face-name default ", ") ;; If we only want one, and the default is more than one, ;; discard the unwanted ones. - (setq default (car default)) + (setq defaults default default (car default)) (if (symbolp default) (symbol-name default) default)))) @@ -1137,8 +1138,8 @@ read-face-name (let ((face (completing-read prompt (completion-table-in-turn nonaliasfaces aliasfaces) - nil t nil 'face-name-history default))) - (if (facep face) (intern face)))))) + nil t nil 'face-name-history defaults))) + (if (facep face) (intern face))))))) ;; Not defined without X, but behind window-system test. (defvar x-bitmap-file-path)
Juri Linkov <juri@linkov.net> writes: > Then read-face-name should be improved to support a list of defaults: Makes sense to me; go ahead and push. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no