Make color numbers on Windows consistent

[gwsw]

Currently on Windows, if a color is specified by -D followed by an uppercase letter and a number, the number is interpreted as an 8-bit SGR 38;5 color value, but if a color is specified by -D followed by a lowercase letter and a number, the number is interpreted as a 4-bit CHAR_INFO.Attribute color value. On other platforms, all color numbers are interpreted as SGR 38;5 values.

This should be unified so that on Windows, all color numbers are SGR values.

This breaks backwards compatibility on Windows. However the CHAR_INFO.Attribute colors would still accessible using letters instead of numbers for color values (for example, to make underlined text be red on blue, use -Durb instead of -Du4.1).

[avih]

For which release do you aim this?

I think we should make one release with the current scheme, and updated, finally-correct docs, and then, for the next release, try to unify the codepaths between *nix and windows as far as color handling and config goes, and I suspect it would be a risky-ish change.

What are your thoughts on this?

[gwsw]

I'm not sure; I'm inclined to get all the changes done before the next production release. I agree that it will be risky and there are already a lot of changes since v643. On the other hand, the current behavior is ugly and hard to understand, so I'd like to get it all cleaned up and unified as soon as practical. I certainly have no objection to at least getting the initial cleanup phase done and make a numbered release for it before embarking on the bigger change. The decision about whether to make that build a production release can be made separately.

[avih]

Well, I really want to have one release with correct docs of the behavior over the past few years, before changing the behavior, and before refactoring and replacing the code which is responsible for this behavior.

Maybe you'll fix -Da and -Dd/s/k/u, I'll open a PR only for the docs (or just take the diff I posted at the other issue, polish it, and commit it yourself) then make a release, then we'll start with the code changes?

The code we'll be touching is decades old, and it would be easy to introduce issues which might not be noticed till after the release...

[gwsw]

Hm, it appears that colordesc() has a feature whereby if the first char of the color description is 'u', it adds LVB_COMMON_UNDERSCORE to the color. This confusingly conflicts with the 'u' character that selects underscored text as the text whose color is changed. As far as I can see, it's not documented in the man page. It will be a fair amount of work to implement now that options are parsed before get_term, so I'm inclined to remove it. @avih @adoxa Does anyone have thoughts?

[avih]

Well, IIRC LVB_COMMON_UNDERSCORE is related to CJK codepages? I have zero familiarity with it, but this is also something I'm inclined to not-modify till after the release.

[gwsw]

In b4ea7b4 I think I've fixed the -D bugs introduced by the init reordering in 09377f7. I haven't updated the man page yet but will do that next. I did remove support for the undocumented 'u' character. With some work that can be re-added if necessary. However I think long term it would be better to implement the new attribute modifiers added in 608c8e5 (which are currently ignored on Windows), rather than to have Windows-specific syntax for this.

[adoxa]

@gwsw Check your emails from 27/8 July 2017 - I sent you a patch where 'u' was indeed documented (present in 520 release).

@avih When the console gained terminal emulation everyone got a real underline, so there became a choice between sticking with the color or having an actual underline (or both).

[gwsw]

Ah, I see that now. Unfortunately the introduction of color letters has made the use of this 'u' modifier ambiguous in some cases. For example, -Dukr could mean "make underlined text black fg on red bg" or it could mean "make blinking text red fg on default bg with underline". I'm not sure what to do about this, but ultimately I think we should use "_" rather than "u" to add the UNDERSCORE bit. Whether that should be part of this initial phase or later, I'm not sure yet.

[avih]

For future reference, the functionality and docs of u color modifier were added in commit 0734f75 in 2017 (at v515), and the docs were removed (apparently while keeping the u functionality, or some of it) in commit c6eb7aa in 2021 (before v576).

Also for reference, u was still working as expected, at least as far as I tried, before you just removed it at the last commit (today). e.g. less -Dd2u indeed changed bold into green with underline.

I think we should use "_" rather than "u" to add the UNDERSCORE bit

Agreed, but he docs curently say that these modifiers don't work on windows:

              A 4-bit or 8-bit color string may be followed by one or more  of
              the  following  characters to set text attributes in addition to
              the color.  (These attribute characters are not supported on MS-
              DOS/Windows versions.)

              *      Bold

              ~      Standout

              _      Underline

              &      Blink

I don't know if "not supported" refers to difficulties of entering them as arguments at the cmd.exe shell, or that they're simply disabled in less.

If the former, then they can be entered with other means, like the LESS env var, and other shells, like posix ones (busybox-w32 ash, variants of bash, like in git-for-windows, and others), possibly powershell, and maybe others too.

I don't see that dos/windows are excluded when parsing these in parse_color.

Also, as we now know, the handling of d/s/k/u is different than the "upper case letters", like N. However, with the (very) little I tried, I don't think I could make it work with neither lower case letter nor upper case, but that shouldn't serve as evidence that it completely doesn't work.

Whether that should be part of this initial phase or later, I'm not sure yet.

Please, later.

I wouldn't have touched the u thing either, and maybe we can restore it while only removing the ambiguity (taking your word that it is ambiguous), e.g. by allowing it only at the end of the color number string (2u, but not u2), and restore the docs.

At this phase IMO let's just add correct docs, fix bugs which were introduce after the last release - as much as we find such, and make a release.

Then we can break things.

[gwsw]

In 48c3369 I have restored the 'u' modifier, but only at the end of the color string. This is now integrated with the new attribute letters (~_*&) and I have added alphabetic alternatives to these chars, so you can use either 'u' or '_' to get UNDERSCORE, and you can use '~' or 's' to get REVERSE_VIDEO. The other two are not implemented on Windows since I don't see a way to create bold or blinking text via the CHAR_INFO API. These attributes only work on with 4-bit color; that is with the lowercase color selectors, and without -Da. More could be done here to make them work with 8-bit color on a VT.

BTW I have made a small update to the man page in this change but the man page changes for Windows are not yet complete.

[gwsw]

In a1391d8 the u and s modifiers now work with uppercase color selectors as well as lowercase.

[avih]

I wouldn't call it restore. It's a different thing.

It was working only with d/s/k/u on any version of windows, and now it it doesn't work for d/s/k/u on any version of windows, but does work on the upper case letters on win10 and later.

Nevertheless, I think it's not worth spending any more time on it, so let it be.

[gwsw]

now it it doesn't work for d/s/k/u on any version of windows,

Can you explain this, or give an example? As far as I can see, it works with the lowercase letters. For example, -Dsgu displays search matches (standout) in underlined green and -Dubu displays underlined text in underlined blue. It also works with numeric colors: -Ds4u displays error messages in underlined red. This is all with --use-color disabled of course.

[avih]

Apologies.

I only looked briefly at the code, and was a bit fraustrated that instead of restoring 2-3 lines of code for the u thing, you added a new feature, which I didn't want to make you spend time on, and I wouldn't have wanted anyway when we're trying to minimize risk for a release.

It was working only with d/s/k/u on any version of windows

That's not true. Not sure why I thought that's the case, as I only tested it on win10 (before the recent changes). Now I tested the same version again, and it only works on 10 for me (not XP, not 7).

Anyway, looking at it again, I have misinterpreted the scope of it, and i now do see it has code for d/s/k/u too. Apologies again.

So, I just tried it too, and the first thing I tried is LESS= less -Dd2u --help (posix sh), and while bold is indeed transformed to green with underline, the normal text doesn't show at all, and neither is standout.

It's also broken (normal/inverse text invisible) without the u - -Dd2.

Removing the -Dd2 option makes normal and standout text visible.

For example, -Dsgu displays ...
.. -Dubu ...

These have a similar issue for me. It does affect the mapped attribute as expected, but normal text is invisible...

[gwsw]

I'm not sure what you mean "added a new feature"; are you referring to the *~_& modifiers? That was unrelated to this work; it was added in response to #471 for all platforms.

I do see the bug where normal text disappears with -Dd2. It only happens when you put the -D option on the command line; it works correctly if you invoke -D as a command within less. I'll look into that.

[avih]

I'm not sure what you mean "added a new feature"

I was referring to 48c3369 and a1391d8, which looked like a new feature to me ("add alphabetic alternative..." - on all platforms) and touched a considerable number of lines of code.

Hard for me to tell whether all of it was a response to this "let's restore 'u'" thing, or only parts of it, and I may have misunderstood that as well.

[gwsw]

I see. 48c3369 was just restoring 'u' functionality but it involved more code than adoxa's original implementation because the -D option (like all options) is handled before get_term, so we don't yet know what the default window color is. So we need to save some state in opt_D and actually set the colors in get_term. a1391d8 was making 'u' work for uppercase letters, which is indeed a new feature but I think worth it because it's not much code and avoids yet another way that uppercase letters behave differently from lowercase letters.

The invisible normal text bug should be fixed in top of tree now.

[avih]

The invisible normal text bug should be fixed in top of tree now.

Confirmed. Thanks.

Also, for what it's worth, help doesn't crash if built with gcc/clang mingw (filesize does return -1), but it did seem to cash when built with msvc.

The crash was a regression of commit 1fafd96 .