Hi Prot,

I just noticed that with your ef-themes the current line number is not
highlighted for commands like consult-line in Vertico. The face is
consult-line-number. The highlighting of the current candidate works for
consult-line-number-wrapped. The face of the current canidate is
vertico-current. Thanks!

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Fri,  3 Feb 2023 00:32:05 +0100
>
> Hi Prot,

Good day Daniel!

> I just noticed that with your ef-themes the current line number is not
> highlighted for commands like consult-line in Vertico. The face is
> consult-line-number. The highlighting of the current candidate works for
> consult-line-number-wrapped. The face of the current canidate is
> vertico-current. Thanks!

If I understand correctly, you want the vertico line to extend from the
beginning of the line?  Whereas now it starts after the line number.

The reason it has the current behaviour is because 'line-number'
inherits from 'default' (a special case to adjust height when zooming
in/out).  So 'consult-line-number-prefix' gets the background from
there.

Making 'consult-line-number-prefix' the same as 'consult-line-number',
fixed this:

 ef-themes.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ef-themes.el b/ef-themes.el
index 4c0c033..3e60da5 100644
--- a/ef-themes.el
+++ b/ef-themes.el
@@ -1016,6 +1016,7 @@ ;;;; consult
     `(consult-key ((,c :inherit ef-themes-key-binding)))
     `(consult-imenu-prefix ((,c :inherit shadow)))
     `(consult-line-number ((,c :inherit shadow)))
+    `(consult-line-number-prefix ((,c :inherit shadow)))
     `(consult-separator ((,c :foreground ,border)))
 ;;;; corfu
     `(corfu-current ((,c :background ,bg-completion)))

What do you think?

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com

Good morning Prot!

On 2/3/23 06:24, Protesilaos Stavrou wrote:
>> I just noticed that with your ef-themes the current line number is not
>> highlighted for commands like consult-line in Vertico. The face is
>> consult-line-number. The highlighting of the current candidate works for
>> consult-line-number-wrapped. The face of the current canidate is
>> vertico-current. Thanks!
> 
> If I understand correctly, you want the vertico line to extend from the
> beginning of the line?  Whereas now it starts after the line number.

Yes, this is what I would like. With the newest Vertico I can use a
defmethod to add an arrow in front of the current candidate. The arrow
will then appear in front of the line numbers. Without your patch to
ef-themes the line numbers will stand out, there is a gap in background
highlighting. In Modus it looks correct.

~~~
(defvar +vertico-current-arrow t)

(cl-defmethod vertico--format-candidate :around
  (cand prefix suffix index start &context
        ((and +vertico-current-arrow
              (not (bound-and-true-p vertico-flat-mode)))
         (eql t)))
  (setq cand (cl-call-next-method cand prefix suffix index start))
  (if (bound-and-true-p vertico-grid-mode)
      (if (= vertico--index index)
          (concat #("▶" 0 1 (face vertico-current)) cand)
        (concat #("_" 0 1 (display " ")) cand))
    (if (= vertico--index index)
        (concat
         #(" " 0 1 (display (left-fringe right-triangle vertico-current)))
         cand)
      cand)))
~~~

> The reason it has the current behaviour is because 'line-number'
> inherits from 'default' (a special case to adjust height when zooming
> in/out).  So 'consult-line-number-prefix' gets the background from
> there.
> 
> Making 'consult-line-number-prefix' the same as 'consult-line-number',
> fixed this:

Okay, good. Note that Modus differentiates between consult-line-number
(used by consult-line) and consult-line-number-prefix (used by
consult-grep). Do we prefer separate or uniform styling?

All the best,
Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Fri,  3 Feb 2023 07:48:37 +0100
>
> Good morning Prot!

Good morning Daniel!

> On 2/3/23 06:24, Protesilaos Stavrou wrote:
>>> I just noticed that with your ef-themes the current line number is not
>>> highlighted for commands like consult-line in Vertico. The face is
>>> consult-line-number. The highlighting of the current candidate works for
>>> consult-line-number-wrapped. The face of the current canidate is
>>> vertico-current. Thanks!
>> 
>> If I understand correctly, you want the vertico line to extend from the
>> beginning of the line?  Whereas now it starts after the line number.
>
> Yes, this is what I would like. With the newest Vertico I can use a
> defmethod to add an arrow in front of the current candidate. The arrow
> will then appear in front of the line numbers. Without your patch to
> ef-themes the line numbers will stand out, there is a gap in background
> highlighting. In Modus it looks correct.
>
> ~~~
> (defvar +vertico-current-arrow t)
>
> (cl-defmethod vertico--format-candidate :around
>   (cand prefix suffix index start &context
>         ((and +vertico-current-arrow
>               (not (bound-and-true-p vertico-flat-mode)))
>          (eql t)))
>   (setq cand (cl-call-next-method cand prefix suffix index start))
>   (if (bound-and-true-p vertico-grid-mode)
>       (if (= vertico--index index)
>           (concat #("▶" 0 1 (face vertico-current)) cand)
>         (concat #("_" 0 1 (display " ")) cand))
>     (if (= vertico--index index)
>         (concat
>          #(" " 0 1 (display (left-fringe right-triangle vertico-current)))
>          cand)
>       cand)))
> ~~~

I pulled from Vertico git as the package is not updated yet.  Looks
good!

>> The reason it has the current behaviour is because 'line-number'
>> inherits from 'default' (a special case to adjust height when zooming
>> in/out).  So 'consult-line-number-prefix' gets the background from
>> there.
>> 
>> Making 'consult-line-number-prefix' the same as 'consult-line-number',
>> fixed this:
>
> Okay, good. Note that Modus differentiates between consult-line-number
> (used by consult-line) and consult-line-number-prefix (used by
> consult-grep). Do we prefer separate or uniform styling?

I made the change in Modus as well.

    commit 2404de30c29ff0afca68a3b9d8badc483863b08f
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Fri Feb 3 09:05:15 2023 +0200

        Make all Consult line numbers look the same

        We use the 'shadow' face in all interfaces where line numbers are
        contextual information (Occur, Grep,...).  With Consult, this was not
        the case for commands like 'consult-line'.

        This commit also addresses the issue discussed in commit 7e29eba in
        the 'ef-themes' repository (thanks again to Daniel Mendler for
        informing me about this):

            commit 7e29eba3eeb4f152a5512aeba4a98a192cd8b444
            Author: Protesilaos Stavrou <info@protesilaos.com>
            Date:   Fri Feb 3 08:58:17 2023 +0200

                Remove background from consult lines

                The default value of 'consult-line-number-prefix' inherits from the
                'line-number' face.  The Ef themes make the latter inherit from
                'default' in order to have the lines increase/decrease in font size
                when the user calls the 'text-scale-adjust' command.  This arrangement
                meant that Consult was implicitly getting the main background which
                caused commands like 'consult-line' to not be highlighted from their
                absolute beginning but only after the line number.

                Thanks to Daniel Mendler for bringing this matter to my attention:
                <https://lists.sr.ht/~protesilaos/ef-themes/%3Cb03413a6-cb77-615d-145d-db4eb710bfca%40daniel-mendler.de%3E>.

             ef-themes.el | 1 +
             1 file changed, 1 insertion(+)

     modus-themes.el | 1 +
     1 file changed, 1 insertion(+)

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/3/23 08:13, Protesilaos Stavrou wrote:
> I pulled from Vertico git as the package is not updated yet.  Looks
> good!

Thanks. Note that due to cl-defgeneric we don't need advices but can
extend Vertico in a cleaner way.

>>> The reason it has the current behaviour is because 'line-number'
>>> inherits from 'default' (a special case to adjust height when zooming
>>> in/out).  So 'consult-line-number-prefix' gets the background from
>>> there.
>>>
>>> Making 'consult-line-number-prefix' the same as 'consult-line-number',
>>> fixed this:
>>
>> Okay, good. Note that Modus differentiates between consult-line-number
>> (used by consult-line) and consult-line-number-prefix (used by
>> consult-grep). Do we prefer separate or uniform styling?
> 
> I made the change in Modus as well.

Okay, sounds good. But please also check vertico-group-format=nil. Maybe
it is undesired to use the same coloring in this case.

Thanks!

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Fri,  3 Feb 2023 08:33:26 +0100
>
> On 2/3/23 08:13, Protesilaos Stavrou wrote:
>> I pulled from Vertico git as the package is not updated yet.  Looks
>> good!
>
> Thanks. Note that due to cl-defgeneric we don't need advices but can
> extend Vertico in a cleaner way.

Yes, this looks nice.

> [... 12 lines elided]

>> I made the change in Modus as well.
>
> Okay, sounds good. But please also check vertico-group-format=nil. Maybe
> it is undesired to use the same coloring in this case.

With Modus, what I see there is the colour of the file in magenta and
then line numbers in the 'shadow' face.  With Ef it depends on the theme
as the 'consult-file' inherits from 'font-lock-function-name-face'.

The combination of a colour for the file name and then 'shadow' for the
line numbers is the same as in the grep-mode buffers.  Though grep file
names use the 'compilation-info' face which cannot be red or similar due
to the semantics of "info" as a general purpose face (there should be a
'compilation-file' face or something).

We could change 'consult-file' to look like 'compilation-info' instead
of 'font-lock-function-name-face':

 modus-themes.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/modus-themes.el b/modus-themes.el
index 55f7da3..16294e8 100644
--- a/modus-themes.el
+++ b/modus-themes.el
@@ -1923,6 +1923,7 @@ ;;;;; completions
     `(completions-first-difference ((,c :inherit modus-themes-completion-match-1)))
 ;;;;; consult
     `(consult-async-split ((,c :inherit error)))
+    `(consult-file ((,c :inherit modus-themes-bold :foreground ,info)))
     `(consult-key ((,c :inherit modus-themes-key-binding)))
     `(consult-imenu-prefix ((,c :inherit shadow)))
     `(consult-line-number ((,c :inherit shadow)))

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/3/23 16:11, Protesilaos Stavrou wrote:
>> Okay, sounds good. But please also check vertico-group-format=nil. Maybe
>> it is undesired to use the same coloring in this case.
> 
> With Modus, what I see there is the colour of the file in magenta and
> then line numbers in the 'shadow' face.  With Ef it depends on the theme
> as the 'consult-file' inherits from 'font-lock-function-name-face'.
> 
> The combination of a colour for the file name and then 'shadow' for the
> line numbers is the same as in the grep-mode buffers.  Though grep file
> names use the 'compilation-info' face which cannot be red or similar due
> to the semantics of "info" as a general purpose face (there should be a
> 'compilation-file' face or something).
> 
> We could change 'consult-file' to look like 'compilation-info' instead
> of 'font-lock-function-name-face':

I leave that to you. :)

I still haven't updated to Modus 4 since the package does not update on
ELPA and since I didn't set aside time yet to adjust my config accordingly.

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Fri,  3 Feb 2023 18:18:12 +0100

> [... 15 lines elided]

>> We could change 'consult-file' to look like 'compilation-info' instead
>> of 'font-lock-function-name-face':
>
> I leave that to you. :)

That way they know who to go after!  Haha!  Here it is:

    commit f0ddbfd9aacd8de1ea3a008d8591918392bcb6b0
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Sat Feb 4 09:16:26 2023 +0200

        Make 'consult-file' like the same as in the Grep buffers

        This face is used when the user option 'vertico-group-format' is set
        to nil.  With this change, we keep things consistent in the common
        workflow of using 'consult-grep' and exporting to a grep buffer via
        'embark'.

        Thanks to Daniel Mendler for bringing this matter to my attention:
        <https://lists.sr.ht/~protesilaos/ef-themes/%3Cb03413a6-cb77-615d-145d-db4eb710bfca%40daniel-mendler.de%3E#%3C37f01118-1102-d0a9-ce8d-5101f3d44679@daniel-mendler.de%3E>.

     modus-themes.el | 1 +
     1 file changed, 1 insertion(+)

> I still haven't updated to Modus 4 since the package does not update on
> ELPA and since I didn't set aside time yet to adjust my config accordingly.

That's okay.  If you need pointers for the transition, please let me know.

I think ELPA will remain in this state for a while longer.  I will try
to publish 4.1.0 some time this month, though I do not know if that will
fix things.  The error seems spurious to me but I have had no time and
motivation to troubleshoot it.

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/4/23 16:22, Protesilaos Stavrou wrote:
>> I leave that to you. :)
> 
> That way they know who to go after!  Haha!  Here it is:

Haha, I hope it won't be too bad.

>> I still haven't updated to Modus 4 since the package does not update on
>> ELPA and since I didn't set aside time yet to adjust my config accordingly.
> 
> That's okay.  If you need pointers for the transition, please let me know.

Many thanks! Your faint color presets will already help a lot. But when
I tried it a while ago (via `package-install-from-buffer`), it still
looked a bit too different from Modus 3 for my taste and given that
automatic Modus updates are stuck right now, I am also stuck on Modus 3.
That being said I am fully with you about the update to Modus 4 such
that it stays maintainable in the long term, even if it causes some
inconvenience for the users.

> I think ELPA will remain in this state for a while longer.  I will try
> to publish 4.1.0 some time this month, though I do not know if that will
> fix things.  The error seems spurious to me but I have had no time and
> motivation to troubleshoot it.

Okay, no worries about that. I will probably update as soon as a new
version gets distributed. Note that elpa-devel seems to be stuck too.
That may give you some indication earlier. (I pull all my packages from
melpa-devel, gnu-devel or nongnu-devel, since I like to report back
early and most installed packages are highly stable anyway.)

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Sat,  4 Feb 2023 17:10:25 +0100

> [... 21 lines elided]

>> I think ELPA will remain in this state for a while longer.  I will try
>> to publish 4.1.0 some time this month, though I do not know if that will
>> fix things.  The error seems spurious to me but I have had no time and
>> motivation to troubleshoot it.
>
> Okay, no worries about that. I will probably update as soon as a new
> version gets distributed. Note that elpa-devel seems to be stuck too.
> That may give you some indication earlier. (I pull all my packages from
> melpa-devel, gnu-devel or nongnu-devel, since I like to report back
> early and most installed packages are highly stable anyway.)

I do the same for my packages.  Modus is an exception because the GNU
ELPA version is defined as a :core package.  It does not come from my
git repo but from emacs.git (this was done before elpa-devel was
introduced).  I sync my changes with emacs.git only when I publish a new
version, so elpa and elpa-devel have the same package.  For this
specific case, there is modus-themes on MELPA.

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/6/23 08:33, Protesilaos Stavrou wrote:
> I do the same for my packages.  Modus is an exception because the GNU
> ELPA version is defined as a :core package.  It does not come from my
> git repo but from emacs.git (this was done before elpa-devel was
> introduced).  I sync my changes with emacs.git only when I publish a new
> version, so elpa and elpa-devel have the same package.  For this
> specific case, there is modus-themes on MELPA.

I see. So I should probably prioritize MELPA for Modus. Generally I use
GNU > NonGNU > MELPA, which is preferable for me since ELPA uses
versions and builds info files from Org. Packages which are only
available from MELPA are pulled from there.

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Mon,  6 Feb 2023 14:34:10 +0100
>
> On 2/6/23 08:33, Protesilaos Stavrou wrote:
>> I do the same for my packages.  Modus is an exception because the GNU
>> ELPA version is defined as a :core package.  It does not come from my
>> git repo but from emacs.git (this was done before elpa-devel was
>> introduced).  I sync my changes with emacs.git only when I publish a new
>> version, so elpa and elpa-devel have the same package.  For this
>> specific case, there is modus-themes on MELPA.
>
> I see. So I should probably prioritize MELPA for Modus. Generally I use
> GNU > NonGNU > MELPA, which is preferable for me since ELPA uses
> versions and builds info files from Org. Packages which are only
> available from MELPA are pulled from there.

I do the same with 'package-archive-priorities'.  The Info manuals and
the stable versions are helpful.  I set up 'package-pinned-packages'
only for my packages and do it just to be sure that everything works.
They all are elpa-devel except Modus which is pulled from melpa.

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/6/23 15:19, Protesilaos Stavrou wrote:
>> From: Daniel Mendler <mail@daniel-mendler.de>
>> Date: Mon,  6 Feb 2023 14:34:10 +0100
>>
>> On 2/6/23 08:33, Protesilaos Stavrou wrote:
>>> I do the same for my packages.  Modus is an exception because the GNU
>>> ELPA version is defined as a :core package.  It does not come from my
>>> git repo but from emacs.git (this was done before elpa-devel was
>>> introduced).  I sync my changes with emacs.git only when I publish a new
>>> version, so elpa and elpa-devel have the same package.  For this
>>> specific case, there is modus-themes on MELPA.
>>
>> I see. So I should probably prioritize MELPA for Modus. Generally I use
>> GNU > NonGNU > MELPA, which is preferable for me since ELPA uses
>> versions and builds info files from Org. Packages which are only
>> available from MELPA are pulled from there.
> 
> I do the same with 'package-archive-priorities'.  The Info manuals and
> the stable versions are helpful.  I set up 'package-pinned-packages'
> only for my packages and do it just to be sure that everything works.
> They all are elpa-devel except Modus which is pulled from melpa.

I wonder why you release Modus ELPA from :core? Why not use the ELPA
repository with :auto-sync? I think :core makes sense mostly if your
package is completely developed within Emacs itself. Also sometimes the
package within Emacs differs slightly from packages on ELPA, e.g.,
transient, which prohibits using :core.

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Mon,  6 Feb 2023 15:31:38 +0100

> [... 18 lines elided]

>> I do the same with 'package-archive-priorities'.  The Info manuals and
>> the stable versions are helpful.  I set up 'package-pinned-packages'
>> only for my packages and do it just to be sure that everything works.
>> They all are elpa-devel except Modus which is pulled from melpa.
>
> I wonder why you release Modus ELPA from :core? Why not use the ELPA
> repository with :auto-sync? I think :core makes sense mostly if your
> package is completely developed within Emacs itself. Also sometimes the
> package within Emacs differs slightly from packages on ELPA, e.g.,
> transient, which prohibits using :core.

I should just do it as you say.  Maybe that will automagically resolve
the issue with the broken package.  The :core does not add any value.  I
simply did it that way back in 2020 because (i) :auto-sync did not exist
and (ii) elpa-devel was not available yet.  It was simply easier for me
to update just emacs.git than to do that and then also take care of
elpa.git.

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/9/23 12:30, Protesilaos Stavrou wrote:
>> I wonder why you release Modus ELPA from :core? Why not use the ELPA
>> repository with :auto-sync? I think :core makes sense mostly if your
>> package is completely developed within Emacs itself. Also sometimes the
>> package within Emacs differs slightly from packages on ELPA, e.g.,
>> transient, which prohibits using :core.
> 
> I should just do it as you say.  Maybe that will automagically resolve
> the issue with the broken package.  The :core does not add any value.  I
> simply did it that way back in 2020 because (i) :auto-sync did not exist
> and (ii) elpa-devel was not available yet.  It was simply easier for me
> to update just emacs.git than to do that and then also take care of
> elpa.git.

Yes, I think so. With :auto-sync everything works smoothly, without any
intervention from the developer. In contrast pushing your updates to
core will always be manual and is better decoupled from the ELPA/MELPA
releases. Stefan Monnier also told me that he is no fan of using the
:core mechanism of ELPA. It works only in rare cases if strict
discipline is followed and if the package is developed in the core
anyway as I already mentioned. For example due to my work on Compat, I
just saw that the seq package on ELPA is quite different from the one
provided by Emacs itself for compatibility reasons. This means in
principle one can distribute separate versions if the ELPA release
requires additional changes for compatibility. I believe Jonas
Bernoulli's transient package is also like that - there the ELPA version
is not exactly the same as the Emacs core version. When he imports
transient into Emacs, he makes certain modification which are only
relevant to the core version. You could do the same with Modus if the
need comes up. Of course it is better if you don't have to do that, but
it may happen in the future depending on how far you want to be backward
compatible.

Daniel

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Fri, 10 Feb 2023 12:07:54 +0100

> [... 7 lines elided]

>> I should just do it as you say.  Maybe that will automagically resolve
>> the issue with the broken package.  The :core does not add any value.  I
>> simply did it that way back in 2020 because (i) :auto-sync did not exist
>> and (ii) elpa-devel was not available yet.  It was simply easier for me
>> to update just emacs.git than to do that and then also take care of
>> elpa.git.
>
> Yes, I think so. With :auto-sync everything works smoothly, without any
> intervention from the developer. In contrast pushing your updates to
> core will always be manual and is better decoupled from the ELPA/MELPA
> releases. Stefan Monnier also told me that he is no fan of using the
> :core mechanism of ELPA. It works only in rare cases if strict
> discipline is followed and if the package is developed in the core
> anyway as I already mentioned. For example due to my work on Compat, I
> just saw that the seq package on ELPA is quite different from the one
> provided by Emacs itself for compatibility reasons. This means in
> principle one can distribute separate versions if the ELPA release
> requires additional changes for compatibility. I believe Jonas
> Bernoulli's transient package is also like that - there the ELPA version
> is not exactly the same as the Emacs core version. When he imports
> transient into Emacs, he makes certain modification which are only
> relevant to the core version. You could do the same with Modus if the
> need comes up. Of course it is better if you don't have to do that, but
> it may happen in the future depending on how far you want to be backward
> compatible.

You are right.  I changed the recipe on elpa.git.  The :core was an old
arrangement that was not adding value anymore.

-- 
Protesilaos Stavrou
https://protesilaos.com

On 2/12/23 18:23, Protesilaos Stavrou wrote:
> You are right.  I changed the recipe on elpa.git.  The :core was an old
> arrangement that was not adding value anymore.

That's good! But ELPA still doesn't seem to build the new version of
Modus? Probably Stefan has to do some manual intervention? (cc'ed)

Daniel