Patchwork [4,of,4,RFC] color: support different styling depending on color support (BC)

login
register
mail settings
Submitter Gregory Szorc
Date July 9, 2017, 11:46 p.m.
Message ID <91d6e437a93d91c1423e.1499643977@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/22196/
State Accepted
Headers show

Comments

Gregory Szorc - July 9, 2017, 11:46 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1499643195 25200
#      Sun Jul 09 16:33:15 2017 -0700
# Node ID 91d6e437a93d91c1423e2ce735dc053a131fd590
# Parent  fa6223b9e2a0d9fbfa81329b83c0512417cee713
[RFC] color: support different styling depending on color support (BC)

What good is support for 16 and 256 colors if we're not going to use
it?

This commit implements support for customizing the default styling
based on how many colors are currently being used.

The _defaultstyles dict now accepts dicts as values. If a dict is
present, keys indicate styling for that amount of available
colors. This allows us to tailor styling based on what is
available, allowing us to provide an optimal experience depending
on the level of color support.

This patch is RFC quality because there are no tests and I basically
threw the implementation together without much thought to see what
the reaction would be to the feature.

I'm not sure if a simple color count in the dict is appropriate
because depending on color support on Windows, our color "namespace
may not align. In addition, we should probably consider how color
blindness comes into play. I'd *really* like a config knob to
declare your color blindness. Presumably then we'd have style
variations for each color blind scenario.
Jun Wu - July 10, 2017, 4:32 a.m.
I think the series is a good improvement and can be queued with some small
in-flight fixes.

Excerpts from Gregory Szorc's message of 2017-07-09 16:46:17 -0700:
> diff --git a/mercurial/color.py b/mercurial/color.py
> --- a/mercurial/color.py
> +++ b/mercurial/color.py
> @@ -135,13 +135,31 @@ except ImportError:
>      'diff.inserted': 'green',
>      'diff.tab': '',
>      'diff.trailingwhitespace': 'bold red_background',
> -    'changeset.public' : '',
> -    'changeset.draft' : '',
> -    'changeset.secret' : '',
> +    'changeset.public': {
> +        8: '',
> +        16: 'brightred',

This is mostly nitpicking, but red seems to be mostly about errors or
warnings. I'd vote "brightgreen" for public changesets. Or make public
brightyellow and draft non-bright yellow.

> [...]

Not only in colortable, it seems config support is also worthwhile, like:

  [color]
  changeset.public:8 = ...
  changeset.public:16 = ...

That could be a future patch.
Kyle Lippincott - July 10, 2017, 11:57 p.m.
On Sun, Jul 9, 2017 at 9:32 PM, Jun Wu <quark@fb.com> wrote:

> I think the series is a good improvement and can be queued with some small
> in-flight fixes.
>
> Excerpts from Gregory Szorc's message of 2017-07-09 16:46:17 -0700:
> > diff --git a/mercurial/color.py b/mercurial/color.py
> > --- a/mercurial/color.py
> > +++ b/mercurial/color.py
> > @@ -135,13 +135,31 @@ except ImportError:
> >      'diff.inserted': 'green',
> >      'diff.tab': '',
> >      'diff.trailingwhitespace': 'bold red_background',
> > -    'changeset.public' : '',
> > -    'changeset.draft' : '',
> > -    'changeset.secret' : '',
> > +    'changeset.public': {
> > +        8: '',
> > +        16: 'brightred',
>
> This is mostly nitpicking, but red seems to be mostly about errors or
> warnings. I'd vote "brightgreen" for public changesets. Or make public
> brightyellow and draft non-bright yellow.
>
> > [...]
>
> Not only in colortable, it seems config support is also worthwhile, like:
>
>   [color]
>   changeset.public:8 = ...
>   changeset.public:16 = ...
>
> That could be a future patch.
>

I would definitely use this in config if it was available :)


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Sean Farley - July 11, 2017, 11:14 p.m.
Kyle Lippincott <spectral@pewpew.net> writes:

> On Sun, Jul 9, 2017 at 9:32 PM, Jun Wu <quark@fb.com> wrote:
>
>> I think the series is a good improvement and can be queued with some small
>> in-flight fixes.
>>
>> Excerpts from Gregory Szorc's message of 2017-07-09 16:46:17 -0700:
>> > diff --git a/mercurial/color.py b/mercurial/color.py
>> > --- a/mercurial/color.py
>> > +++ b/mercurial/color.py
>> > @@ -135,13 +135,31 @@ except ImportError:
>> >      'diff.inserted': 'green',
>> >      'diff.tab': '',
>> >      'diff.trailingwhitespace': 'bold red_background',
>> > -    'changeset.public' : '',
>> > -    'changeset.draft' : '',
>> > -    'changeset.secret' : '',
>> > +    'changeset.public': {
>> > +        8: '',
>> > +        16: 'brightred',
>>
>> This is mostly nitpicking, but red seems to be mostly about errors or
>> warnings. I'd vote "brightgreen" for public changesets. Or make public
>> brightyellow and draft non-bright yellow.

For what it's worth, I've defined the following throughout the years:

public -> blue
draft -> green
secret -> red

I've used yellow for "labeling" (no inherent meaning; just things like
user / date / subject). I might follow-up with some nice templates /
mapfiles if people want those.

Patch

diff --git a/mercurial/color.py b/mercurial/color.py
--- a/mercurial/color.py
+++ b/mercurial/color.py
@@ -135,13 +135,31 @@  except ImportError:
     'diff.inserted': 'green',
     'diff.tab': '',
     'diff.trailingwhitespace': 'bold red_background',
-    'changeset.public' : '',
-    'changeset.draft' : '',
-    'changeset.secret' : '',
+    'changeset.public': {
+        8: '',
+        16: 'brightred',
+    },
+    'changeset.draft': {
+        8: '',
+        16: 'brightyellow',
+    },
+    'changeset.secret': {
+        8: '',
+        16: 'brightmagenta',
+    },
     'diffstat.deleted': 'red',
     'diffstat.inserted': 'green',
     'histedit.remaining': 'red bold',
     'ui.prompt': 'yellow',
+    'log.bookmark': {
+        16: 'brightgreen',
+    },
+    'log.branch': {
+        16: 'brightcyan',
+    },
+    'log.tag': {
+        16: 'brightblue',
+    },
     'log.changeset': 'yellow',
     'patchbomb.finalsummary': '',
     'patchbomb.from': 'magenta',
@@ -168,7 +186,23 @@  except ImportError:
 }
 
 def loadcolortable(ui, extname, colortable):
-    _defaultstyles.update(colortable)
+    for k, v in colortable.items():
+        # Incoming value is a dict. Ensure existing key is a dict then
+        # overlay.
+        if isinstance(v, dict):
+            if (k in _defaultstyles
+                    and not isinstance(_defaultstyles[k], dict)):
+                _defaultstyles[k] = {8: _defaultstyles[k]}
+            else:
+                _defaultstyles[k] = {}
+
+            _defaultstyles[k].update(v)
+        else:
+            # It's a string. Interpret as legacy and assume it is for 8 colors.
+            if k in _defaultstyles and isinstance(_defaultstyles[k], dict):
+                _defaultstyles[k][8] = v
+            else:
+                _defaultstyles[k] = v
 
 def _terminfocolors(ui):
     """Obtain defined terminfo colors as a dict.
@@ -202,6 +236,7 @@  def _terminfocolors(ui):
         colorlimit = 8
 
     usecolors = min(termcolors, colorlimit)
+    ui._colorcount = usecolors
 
     if usecolors >= 16:
         for color, value in TERMINFO_COLOR_16.items():
@@ -364,7 +399,19 @@  def _modesetup(ui):
     return None
 
 def configstyles(ui):
-    ui._styles.update(_defaultstyles)
+    colorcount = ui._colorcount
+
+    for k, v in _defaultstyles.iteritems():
+        if isinstance(v, dict):
+            for colors, colorvalue in sorted(v.items(), reverse=True):
+                # Use the first value that is within our acceptable color count.
+                if colors <= colorcount:
+                    ui._styles[k] = colorvalue
+                    break
+
+        else:
+            ui._styles[k] = v
+
     for status, cfgeffects in ui.configitems('color'):
         if '.' not in status or status.startswith(('color.', 'terminfo.')):
             continue
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -192,6 +192,7 @@  class ui(object):
         self.logblockedtimes = False
         # color mode: see mercurial/color.py for possible value
         self._colormode = None
+        self._colorcount = None
         self._terminfoparams = {}
         self._styles = {}
 
@@ -213,6 +214,7 @@  class ui(object):
             self.callhooks = src.callhooks
             self.insecureconnections = src.insecureconnections
             self._colormode = src._colormode
+            self._colorcount = src._colorcount
             self._terminfoparams = src._terminfoparams.copy()
             self._styles = src._styles.copy()