GNU bug report logs - #46358
28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 46358 in the body.
You can then email your comments to 46358 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; [PATCH] Add vc-dir faces; also apply them to vc-git
Date: Sun, 07 Feb 2021 13:42:09 +0200

[Message part 1 (text/plain, inline)]

Dear maintainers,

The vc-dir.el library hardcodes all of its faces to generic font-lock
ones.  This makes it impossible for users/themes to exert any control
over the presentation of those buffers.

In the attached patch, I do the following:

1. Define new faces.  Each has semantic value in that it applies to
   constructs implied by its name.

2. Cover the vc-git backend's implementation of extra vc-dir headers.
   This necessarily means that not all backends are brought to the same
   state after applying this patch.

3. Address a "FIXME" comment in vc-git.el concerning the use of a
   bespoke face for the stash header's value when that is empty.

4. Use new color combinations which conform with the WCAG AAA standard
   for color contrast against pure white/black (this standard pertains
   to legibility and is the highest of its kind).

With regard to point 2, I only use Git and thus cannot test the other
backends with the requisite degree of confidence.  Do you think I should
try regardless?  Or should we just support the Git backend and hope that
someone else will work on [some of] the other backends?

On point 4, please consider this a proposal: it is a highly opinionated
change.  If you feel we should in no way alienate existing users, I am
prepared to remove all colors and just :inherit from the faces that
applied before.

I attach a couple of screenshots showcasing the expected results.

Please let me know what you think.  I remain at your disposal for any
possible amendments to this patch, assuming you are willing to merge it.

All the best,
Protesilaos

-- 
Protesilaos Stavrou
protesilaos.com

[0001-Add-vc-dir-faces-also-apply-them-to-vc-git.patch (text/x-patch, attachment)]

[vc-dir-refashion-faces-dark.png (image/png, attachment)]

[vc-dir-refashion-faces-light.png (image/png, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces;
 also apply them to vc-git
Date: Sun, 07 Feb 2021 17:15:03 +0200

> From: Protesilaos Stavrou <info <at> protesilaos.com>
> Date: Sun, 07 Feb 2021 13:42:09 +0200
> 
> In the attached patch, I do the following:
> 
> 1. Define new faces.  Each has semantic value in that it applies to
>    constructs implied by its name.

Thanks.  Would it be possible to use color names rather than #RRGGBB
values?  The latter makes it very hard to figure out the color that
will be used by the face.

> 4. Use new color combinations which conform with the WCAG AAA standard
>    for color contrast against pure white/black (this standard pertains
>    to legibility and is the highest of its kind).

Not sure what that means in practical terms: most Emacs users I've
watched working (myself included) use some background color other than
pure black or white.  Doesn't that change the contrast and the optimal
colors?

> With regard to point 2, I only use Git and thus cannot test the other
> backends with the requisite degree of confidence.  Do you think I should
> try regardless?  Or should we just support the Git backend and hope that
> someone else will work on [some of] the other backends?

If you can easily try other backends, it will be appreciated.  But it
is not mandatory, IMO.

> On point 4, please consider this a proposal: it is a highly opinionated
> change.  If you feel we should in no way alienate existing users, I am
> prepared to remove all colors and just :inherit from the faces that
> applied before.

Personally, I think inheriting from the existing faces will be less
drastic, so it's probably better.

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 46358 <at> debbugs.gnu.org
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Sun, 07 Feb 2021 18:15:14 +0200

[Message part 1 (text/plain, inline)]

On 2021-02-07, 17:15 +0200, Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Protesilaos Stavrou <info <at> protesilaos.com>
>> Date: Sun, 07 Feb 2021 13:42:09 +0200
>> 
>> In the attached patch, I do the following:
>> 
>> 1. Define new faces.  Each has semantic value in that it applies to
>>    constructs implied by its name.
>
> Thanks.  Would it be possible to use color names rather than #RRGGBB
> values?  The latter makes it very hard to figure out the color that
> will be used by the face.

I will keep this in mind for the next time.  For this case I removed all
color specifications (please find the revised patch attached to this
message).

>> 4. Use new color combinations which conform with the WCAG AAA standard
>>    for color contrast against pure white/black (this standard pertains
>>    to legibility and is the highest of its kind).
>
> Not sure what that means in practical terms: most Emacs users I've
> watched working (myself included) use some background color other than
> pure black or white.  Doesn't that change the contrast and the optimal
> colors?

You are right: I should have clarified that I meant the default white
background and its inverse.  Other themes would indeed have to adapt
things to their needs.

>> With regard to point 2, I only use Git and thus cannot test the other
>> backends with the requisite degree of confidence.  Do you think I should
>> try regardless?  Or should we just support the Git backend and hope that
>> someone else will work on [some of] the other backends?
>
> If you can easily try other backends, it will be appreciated.  But it
> is not mandatory, IMO.

I will inspect their code and try to identify whatever looks the same as
vc-git.  Then I will prepare a separate patch.

>> On point 4, please consider this a proposal: it is a highly opinionated
>> change.  If you feel we should in no way alienate existing users, I am
>> prepared to remove all colors and just :inherit from the faces that
>> applied before.
>
> Personally, I think inheriting from the existing faces will be less
> drastic, so it's probably better.

Very well!  I am doing just that in the revised patch.  So there should
be no visual difference between this and the prior state, except for one
case: the empty Git stash header, which will ultimately inherit from
'shadow' (before there was a "FIXME" to disambiguate it from other
header values).

-- 
Protesilaos Stavrou
protesilaos.com

[0001-Add-vc-dir-faces-also-apply-them-to-vc-git.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Mon, 08 Feb 2021 07:55:25 +0100

Protesilaos Stavrou <info <at> protesilaos.com> writes:

> Very well!  I am doing just that in the revised patch.  So there should
> be no visual difference between this and the prior state, except for one
> case: the empty Git stash header, which will ultimately inherit from
> 'shadow' (before there was a "FIXME" to disambiguate it from other
> header values).

Looks good to me; pushed to Emacs 28 now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 46358 <at> debbugs.gnu.org
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Mon, 8 Feb 2021 17:54:17 +0200

On 07.02.2021 18:15, Protesilaos Stavrou wrote:
> On 2021-02-07, 17:15 +0200, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>>> From: Protesilaos Stavrou <info <at> protesilaos.com>
>>> Date: Sun, 07 Feb 2021 13:42:09 +0200
>>>
>>> In the attached patch, I do the following:
>>>
>>> 1. Define new faces.  Each has semantic value in that it applies to
>>>     constructs implied by its name.
>>
>> Thanks.  Would it be possible to use color names rather than #RRGGBB
>> values?  The latter makes it very hard to figure out the color that
>> will be used by the face.
> 
> I will keep this in mind for the next time.  For this case I removed all
> color specifications (please find the revised patch attached to this
> message).

Thanks, this is better.

I'm not opposed to changing colors, but this probably should be done 
systematically across many faces in the default theme, rather than in 
one specific UI element. Shouldn't it?

>>> 4. Use new color combinations which conform with the WCAG AAA standard
>>>     for color contrast against pure white/black (this standard pertains
>>>     to legibility and is the highest of its kind).
>>
>> Not sure what that means in practical terms: most Emacs users I've
>> watched working (myself included) use some background color other than
>> pure black or white.  Doesn't that change the contrast and the optimal
>> colors?
> 
> You are right: I should have clarified that I meant the default white
> background and its inverse.  Other themes would indeed have to adapt
> things to their needs.

True.

>>> With regard to point 2, I only use Git and thus cannot test the other
>>> backends with the requisite degree of confidence.  Do you think I should
>>> try regardless?  Or should we just support the Git backend and hope that
>>> someone else will work on [some of] the other backends?
>>
>> If you can easily try other backends, it will be appreciated.  But it
>> is not mandatory, IMO.
> 
> I will inspect their code and try to identify whatever looks the same as
> vc-git.  Then I will prepare a separate patch.

FWIW, Git is the only backend that has a complex dir-printer method.

The rest look pretty much like vc-hg-dir-printer, but 
'font-lock-comment-face' in there should be changed to some new face too.

>> Personally, I think inheriting from the existing faces will be less
>> drastic, so it's probably better.
> 
> Very well!  I am doing just that in the revised patch.  So there should
> be no visual difference between this and the prior state, except for one
> case: the empty Git stash header, which will ultimately inherit from
> 'shadow' (before there was a "FIXME" to disambiguate it from other
> header values).

Some questions:

- vc-dir-ignored face doesn't seem to be used the 'ignored' entries in 
the list. Wasn't that its main point?

- vc-git-dir-printer defaults entries to the 'vc-dir-status-edited' 
face, whereas vc-default-dir-printer defaults to vc-dir-header-value' 
(statuses that are not 'up-to-date', 'missing', 'conflict' or 'edited'). 
Which is the intended behavior? Which one do we want?

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 46358 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Mon, 08 Feb 2021 18:35:31 +0200

On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> Thanks, this is better.
>
> I'm not opposed to changing colors, but this probably should be done
> systematically across many faces in the default theme, rather than in
> one specific UI element. Shouldn't it?

Yes, that would be better.

>>>> With regard to point 2, I only use Git and thus cannot test the other
>>>> backends with the requisite degree of confidence.  Do you think I should
>>>> try regardless?  Or should we just support the Git backend and hope that
>>>> someone else will work on [some of] the other backends?
>>>
>>> If you can easily try other backends, it will be appreciated.  But it
>>> is not mandatory, IMO.
>> I will inspect their code and try to identify whatever looks the same
>> as
>> vc-git.  Then I will prepare a separate patch.
>
> FWIW, Git is the only backend that has a complex dir-printer method.
>
> The rest look pretty much like vc-hg-dir-printer, but
> 'font-lock-comment-face' in there should be changed to some new face
> too.

Thanks!  I still have not had the time to check those, though I plan to
do so.  It would be nice to ensure consistency between all backends.

>>> Personally, I think inheriting from the existing faces will be less
>>> drastic, so it's probably better.
>> Very well!  I am doing just that in the revised patch.  So there
>> should
>> be no visual difference between this and the prior state, except for one
>> case: the empty Git stash header, which will ultimately inherit from
>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>> header values).
>
> Some questions:
>
> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>   the list. Wasn't that its main point?

Can you please specify which are those?

I only applied the 'vc-dir-ignored' face to the empty Git stash and only
did so because there was a "FIXME" for it.  Otherwise, yes, the new face
should be used wherever it makes sense.

> - vc-git-dir-printer defaults entries to the 'vc-dir-status-edited'
>   face, whereas vc-default-dir-printer defaults to vc-dir-header-value'
>   (statuses that are not 'up-to-date', 'missing', 'conflict' or
>   'edited'). Which is the intended behavior? Which one do we want?
>

There is an omission from my part that I will now prepare a patch for
with regard to the "edited" check of 'vc-default-dir-printer'.  It needs
to specify 'vc-dir-status-edited' instead of 'font-lock-constant-face'.

As for its default value, I was not sure what those other states were,
so I just used 'vc-dir-header-value', thinking that this is a neutral
value.

Should the default look like "edited" as well?  Or does it have some
other meaning?  If the latter, then maybe we must have extra face?

-- 
Protesilaos Stavrou
protesilaos.com

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

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46358 <at> debbugs.gnu.org, Protesilaos Stavrou <info <at> protesilaos.com>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Mon, 08 Feb 2021 20:17:33 +0200

>> Very well!  I am doing just that in the revised patch.  So there should
>> be no visual difference between this and the prior state, except for one
>> case: the empty Git stash header, which will ultimately inherit from
>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>> header values).
>
> Looks good to me; pushed to Emacs 28 now.

I don't know if anyone else has such problem, but now highlighting
the empty Git stash header with a different color distinguishes it
from other header lines, and thus attracts more attention.
So now the most noticeable thing in the vc-dir is the Git stash header
(that I almost never use).

Maybe better to display the empty Git stash header using the default
colors, and then highlight it differently only when it's non-empty?

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Juri Linkov <juri <at> linkov.net>, Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 46358 <at> debbugs.gnu.org, Protesilaos Stavrou <info <at> protesilaos.com>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Tue, 9 Feb 2021 01:24:04 +0200

On 08.02.2021 20:17, Juri Linkov wrote:
>>> Very well!  I am doing just that in the revised patch.  So there should
>>> be no visual difference between this and the prior state, except for one
>>> case: the empty Git stash header, which will ultimately inherit from
>>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>>> header values).
>>
>> Looks good to me; pushed to Emacs 28 now.
> 
> I don't know if anyone else has such problem, but now highlighting
> the empty Git stash header with a different color distinguishes it
> from other header lines, and thus attracts more attention.
> So now the most noticeable thing in the vc-dir is the Git stash header
> (that I almost never use).

It looks okay-ish for me, but that must depend on a particular theme.

> Maybe better to display the empty Git stash header using the default
> colors, and then highlight it differently only when it's non-empty?

Not with vc-dir-ignored, though (it is based on the 'shadow' face).

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Tue, 9 Feb 2021 01:33:44 +0200

On 08.02.2021 18:35, Protesilaos Stavrou wrote:
> On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>> Some questions:
>>
>> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>>    the list. Wasn't that its main point?
> 
> Can you please specify which are those?
> 
> I only applied the 'vc-dir-ignored' face to the empty Git stash and only
> did so because there was a "FIXME" for it.  Otherwise, yes, the new face
> should be used wherever it makes sense.

The 'ignored' files in the vc-dir tree.

To see one, edit some file that has a matching entry in .gitignore (such 
as ChangeLog in a Emacs repo checkout). You should see it in VC-Dir 
buffer now, with status 'ignored'.

>> - vc-git-dir-printer defaults entries to the 'vc-dir-status-edited'
>>    face, whereas vc-default-dir-printer defaults to vc-dir-header-value'
>>    (statuses that are not 'up-to-date', 'missing', 'conflict' or
>>    'edited'). Which is the intended behavior? Which one do we want?
>>
> 
> There is an omission from my part that I will now prepare a patch for
> with regard to the "edited" check of 'vc-default-dir-printer'.  It needs
> to specify 'vc-dir-status-edited' instead of 'font-lock-constant-face'.

Thank you.

> As for its default value, I was not sure what those other states were,
> so I just used 'vc-dir-header-value', thinking that this is a neutral
> value.

All possible states are listed in the docstring for 'vc-state'.

About half of them (almost) are pretty rare, though.

> Should the default look like "edited" as well?  Or does it have some
> other meaning?  If the latter, then maybe we must have extra face?

I don't have a strong opinion on this right now. But we should be 
consistent between the 'default' version and the backend-specific 
versions of the method.

Having a face per status might be too much both for the user and the 
theme authors, though (who will have to pick appropriate colors).

So I would keep the number of faces at 4: up-to-date, warning, ignored 
and edited.

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 46358 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Tue, 09 Feb 2021 07:01:00 +0200

[Message part 1 (text/plain, inline)]

On 2021-02-09, 01:33 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> On 08.02.2021 18:35, Protesilaos Stavrou wrote:
>> On 2021-02-08, 17:54 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>>> Some questions:
>>>
>>> - vc-dir-ignored face doesn't seem to be used the 'ignored' entries in
>>>    the list. Wasn't that its main point?
>> Can you please specify which are those?
>> I only applied the 'vc-dir-ignored' face to the empty Git stash and
>> only
>> did so because there was a "FIXME" for it.  Otherwise, yes, the new face
>> should be used wherever it makes sense.
>
> The 'ignored' files in the vc-dir tree.
>
> To see one, edit some file that has a matching entry in .gitignore (such
> as ChangeLog in a Emacs repo checkout). You should see it in VC-Dir
> buffer now, with status 'ignored'.

Please see the attached patch (unless you want me to open a new bug
report).  This should now account for the ignored state.  It also edits
a face that I had missed earlier, as was discussed herein.

>> As for its default value, I was not sure what those other states were,
>> so I just used 'vc-dir-header-value', thinking that this is a neutral
>> value.
>
> All possible states are listed in the docstring for 'vc-state'.
>
> About half of them (almost) are pretty rare, though.
>
>> Should the default look like "edited" as well?  Or does it have some
>> other meaning?  If the latter, then maybe we must have extra face?
>
> I don't have a strong opinion on this right now. But we should be
> consistent between the 'default' version and the backend-specific
> versions of the method.
>
> Having a face per status might be too much both for the user and the
> theme authors, though (who will have to pick appropriate colors).
>
> So I would keep the number of faces at 4: up-to-date, warning, ignored
> and edited.

I also think that 4 faces should suffice.  Having checked the doc string
of 'vc-state' this is how I feel they should be organised.

| status           | face ("?" means suggestion) |
|------------------+-----------------------------|
| up-to-date       | vc-dir-status-up-to-date    |
| edited           | vc-dir-status-edited        |
| USER             | vc-dir-status-warning?      |
| needs-update     | vc-dir-status-warning?      |
| unlocked-changes | vc-dir-status-warning?      |
| added            | vc-dir-status-edited        |
| removed          | vc-dir-status-edited        |
| conflict         | vc-dir-status-warning       |
| missing          | vc-dir-status-warning       |
| ignored          | vc-dir-ignored              |
| unregistered     | vc-dir-status-edited        |

With regard to 'vc-dir-ignored', do you think we should rename it to
'vc-dir-status-ignored' for the sake of consistency?  I wrote it that
way to denote that it is more generic than those that apply to files,
but I am okay either way.

-- 
Protesilaos Stavrou
protesilaos.com

[0001-Refine-use-of-new-vc-dir-faces.patch (text/x-patch, attachment)]

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Tue, 09 Feb 2021 08:42:26 +0200

On 2021-02-09, 01:24 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> On 08.02.2021 20:17, Juri Linkov wrote:
>>>> Very well!  I am doing just that in the revised patch.  So there should
>>>> be no visual difference between this and the prior state, except for one
>>>> case: the empty Git stash header, which will ultimately inherit from
>>>> 'shadow' (before there was a "FIXME" to disambiguate it from other
>>>> header values).
>>>
>>> Looks good to me; pushed to Emacs 28 now.
>> I don't know if anyone else has such problem, but now highlighting
>> the empty Git stash header with a different color distinguishes it
>> from other header lines, and thus attracts more attention.
>> So now the most noticeable thing in the vc-dir is the Git stash header
>> (that I almost never use).
>
> It looks okay-ish for me, but that must depend on a particular theme.

For the default theme, the headers are green while their values are
orange.  The value of an empty stash is gray right now, while that of a
non-empty one is orange, just like the other headers' values.

>> Maybe better to display the empty Git stash header using the default
>> colors, and then highlight it differently only when it's non-empty?
>
> Not with vc-dir-ignored, though (it is based on the 'shadow' face).

Before this change, the empty stash looked the same as all other
headers' values.  This meant that there was no distinction between empty
and non-empty stashes, something that was noted as a "FIXME" in the
source code.

I think there is value in distinguishing between those two states,
though I am fine with some other choice of fallback color/face.
Currently the empty stash uses 'vc-dir-ignored' which inherits from
'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
'font-lock-variable-name-face'.

If you feel this is a problem 'vc-dir-ignored' could also inherit from
'font-lock-variable-name-face'.

-- 
Protesilaos Stavrou
protesilaos.com

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

From: Juri Linkov <juri <at> linkov.net>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Tue, 09 Feb 2021 11:19:01 +0200

>>> Maybe better to display the empty Git stash header using the default
>>> colors, and then highlight it differently only when it's non-empty?
>>
>> Not with vc-dir-ignored, though (it is based on the 'shadow' face).
>
> Before this change, the empty stash looked the same as all other
> headers' values.

It could continue looking the same as all other headers' values.

> This meant that there was no distinction between empty and non-empty
> stashes, something that was noted as a "FIXME" in the source code.

Indeed, it could be better to have a distinction between empty and
non-empty stashes.  The FIXME proposed to use a different face
when nothing is stashed.

But it seems better to use a different face when something is stashed.
It's important to attract user attention to the fact that there are
stashes laying around.

> I think there is value in distinguishing between those two states,
> though I am fine with some other choice of fallback color/face.
> Currently the empty stash uses 'vc-dir-ignored' which inherits from
> 'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
> 'font-lock-variable-name-face'.

Could some of new vc-dir faces that you added recently
be used for non-empty stashes?

> If you feel this is a problem 'vc-dir-ignored' could also inherit from
> 'font-lock-variable-name-face'.

It's good that 'vc-dir-ignored' inherits from the 'shadow' face
when it's used on files with the "ignored" status.  Then it
could be renamed to 'vc-dir-status-ignored' as you already proposed.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Tue, 9 Feb 2021 15:05:39 +0200

On 09.02.2021 07:01, Protesilaos Stavrou wrote:
> I also think that 4 faces should suffice.  Having checked the doc string
> of 'vc-state' this is how I feel they should be organised.
> 
> | status           | face ("?" means suggestion) |
> |------------------+-----------------------------|
> | up-to-date       | vc-dir-status-up-to-date    |
> | edited           | vc-dir-status-edited        |
> | USER             | vc-dir-status-warning?      |
> | needs-update     | vc-dir-status-warning?      |
> | unlocked-changes | vc-dir-status-warning?      |
> | added            | vc-dir-status-edited        |
> | removed          | vc-dir-status-edited        |
> | conflict         | vc-dir-status-warning       |
> | missing          | vc-dir-status-warning       |
> | ignored          | vc-dir-ignored              |
> | unregistered     | vc-dir-status-edited        |

Looks good.

> With regard to 'vc-dir-ignored', do you think we should rename it to
> 'vc-dir-status-ignored' for the sake of consistency?

Yes, probably. Let's see how your discussion with Juri ends up.

But if the only one use of this face is related to stashes, perhaps 
introduce a stash-specific face instead.

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Tue, 09 Feb 2021 18:30:46 +0200

On 2021-02-09, 11:19 +0200, Juri Linkov <juri <at> linkov.net> wrote:

>>>> Maybe better to display the empty Git stash header using the default
>>>> colors, and then highlight it differently only when it's non-empty?
>>>
>>> Not with vc-dir-ignored, though (it is based on the 'shadow' face).
>>
>> Before this change, the empty stash looked the same as all other
>> headers' values.
>
> It could continue looking the same as all other headers' values.

Thinking again about this, I now agree with this view.  An empty stash
is not an "inactive" status but rather a valid header value.

>> This meant that there was no distinction between empty and non-empty
>> stashes, something that was noted as a "FIXME" in the source code.
>
> Indeed, it could be better to have a distinction between empty and
> non-empty stashes.  The FIXME proposed to use a different face
> when nothing is stashed.
>
> But it seems better to use a different face when something is stashed.
> It's important to attract user attention to the fact that there are
> stashes laying around.

I actually feel that this level of distinction already exists.  When the
stash is non-empty, there is a button/link which by default is blue, so
it contrasts well with the orange header values.  This button can be
used to toggle the visibility of the stash list.  Stash entries are the
same color as all other header values, yet their presence is already
quite obvious in context.

>> I think there is value in distinguishing between those two states,
>> though I am fine with some other choice of fallback color/face.
>> Currently the empty stash uses 'vc-dir-ignored' which inherits from
>> 'shadow'.  Whereas others use 'vc-dir-header-value' which inherit from
>> 'font-lock-variable-name-face'.
>
> Could some of new vc-dir faces that you added recently
> be used for non-empty stashes?

In principle yes, though I now believe that we do not need to introduce
such a qualification.

>> If you feel this is a problem 'vc-dir-ignored' could also inherit from
>> 'font-lock-variable-name-face'.
>
> It's good that 'vc-dir-ignored' inherits from the 'shadow' face
> when it's used on files with the "ignored" status.  Then it
> could be renamed to 'vc-dir-status-ignored' as you already proposed.

Yes, it should be renamed.

I will now use this information, as well as what Dmitry shared in the
other comment to prepare a new patch that covers everything.  You can
all test it before it gets applied.  I will share it in this thread
either later in the day or tomorrow.

Thank you!

-- 
Protesilaos Stavrou
protesilaos.com

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Eli Zaretskii <eliz <at> gnu.org>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Tue, 09 Feb 2021 19:46:41 +0200

[Message part 1 (text/plain, inline)]

On 2021-02-09, 18:30 +0200, Protesilaos Stavrou <info <at> protesilaos.com> wrote:

> I will now use this information, as well as what Dmitry shared in the
> other comment to prepare a new patch that covers everything.  You can
> all test it before it gets applied.  I will share it in this thread
> either later in the day or tomorrow.
>
> Thank you!

Hello again!

I have prepared a new patch.

1. Incorporate the feedback received thus far:

   + Do not apply special treatment to the Git stash header.  It now
     looks the same as all other headers.

   + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.

   + Apply 'vc-dir-status-ignored' to gitignored files.

2. Implement the new faces in all backends, while ensuring that every
   value documented for 'vc-state' is taken into account.

   + Please double check that the 'rev-and-lock' state is correct: it is
     the only one I could infer from the context of what the USER of
     'vc-state' is.

If you think there is something else that remains to be done, please let
me know.

Thank you for your time!

--
Protesilaos Stavrou
protesilaos.com

[0001-Refine-use-of-vc-dir-faces-apply-to-all-backends.patch (text/x-patch, attachment)]

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>, Juri Linkov <juri <at> linkov.net>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Wed, 10 Feb 2021 03:48:28 +0200

Hello!

On 09.02.2021 19:46, Protesilaos Stavrou wrote:

> I have prepared a new patch.
> 
> 1. Incorporate the feedback received thus far:
> 
>     + Do not apply special treatment to the Git stash header.  It now
>       looks the same as all other headers.
> 
>     + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.
> 
>     + Apply 'vc-dir-status-ignored' to gitignored files.
> 
> 2. Implement the new faces in all backends, while ensuring that every
>     value documented for 'vc-state' is taken into account.

Thanks!

Looking good, I've applied and pushed this patch. Except for one thing:

>     + Please double check that the 'rev-and-lock' state is correct: it is
>       the only one I could infer from the context of what the USER of
>       'vc-state' is.

USER denotes string values (which will denote being locked by that user).

I don't think the symbol `rev-and-lock' can ever be the value returned 
by 'vc-state', though I'll have to admit not really understanding what 
vc-rcs-consult-headers does.

So I removed that bit. Someone should feel free to correct me.

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

From: Protesilaos Stavrou <info <at> protesilaos.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them
 to vc-git
Date: Wed, 10 Feb 2021 06:06:28 +0200

[Message part 1 (text/plain, inline)]

On 2021-02-10, 03:48 +0200, Dmitry Gutov <dgutov <at> yandex.ru> wrote:

> Hello!
>
> On 09.02.2021 19:46, Protesilaos Stavrou wrote:
>
>> I have prepared a new patch.
>> 1. Incorporate the feedback received thus far:
>>     + Do not apply special treatment to the Git stash header.  It now
>>       looks the same as all other headers.
>>     + Rename 'vc-dir-ignored' to 'vc-dir-status-ignored'.
>>     + Apply 'vc-dir-status-ignored' to gitignored files.
>> 2. Implement the new faces in all backends, while ensuring that every
>>     value documented for 'vc-state' is taken into account.
>
> Thanks!
>
> Looking good, I've applied and pushed this patch. Except for one thing:

I just realised that I forgot to update the NEWS entry.  Patch
attached.  Sorry about that!

-- 
Protesilaos Stavrou
protesilaos.com

[0001-Update-NEWS-entry-for-vc-dir-faces.patch (text/x-patch, attachment)]

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Protesilaos Stavrou <info <at> protesilaos.com>
Cc: 46358 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#46358: 28.0.50; [PATCH] Add vc-dir faces; also apply them to
 vc-git
Date: Wed, 10 Feb 2021 15:32:33 +0200

On 10.02.2021 06:06, Protesilaos Stavrou wrote:
> I just realised that I forgot to update the NEWS entry.  Patch
> attached.  Sorry about that!

Applied, thank you.

Previous Next

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