| Submitter | Matt Harbison |
|---|---|
| Date | March 26, 2017, 4:41 a.m. |
| Message ID | <a263702b064a5a3ce1ca.1490503265@Envy> |
| Download | mbox | patch |
| Permalink | /patch/19675/ |
| State | Accepted |
| Headers | show |
Comments
On 03/26/2017 06:41 AM, Matt Harbison wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1490464217 14400 > # Sat Mar 25 13:50:17 2017 -0400 > # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 > # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 > color: stop mutating the default effects map As far as color is concerned, this is a step in the right direction, Code change looks good to me. Thanks
On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1490464217 14400 > # Sat Mar 25 13:50:17 2017 -0400 > # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 > # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 > color: stop mutating the default effects map I re-read the code, and noticed that both _effects and w32effects are mostly constants. Perhaps we can simply switch them by ui._colormode. if ui._colormode == 'win32': activeeffects = w32neffects else: activeeffects = _effects ... embed escape sequences by using activeeffects ...
On Sun, 26 Mar 2017 10:35:34 -0400, Yuya Nishihara <yuya@tcha.org> wrote: > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote: >> # HG changeset patch >> # User Matt Harbison <matt_harbison@yahoo.com> >> # Date 1490464217 14400 >> # Sat Mar 25 13:50:17 2017 -0400 >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 >> # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 >> color: stop mutating the default effects map > > I re-read the code, and noticed that both _effects and w32effects are > mostly > constants. Perhaps we can simply switch them by ui._colormode. > > if ui._colormode == 'win32': > activeeffects = w32neffects > else: > activeeffects = _effects > ... embed escape sequences by using activeeffects ... Will we get into trouble with each ui instance having a _colormode, but the activeeffects map effectively being global? I don't fully understand the purpose of all the separate ui objects.
On Sun, 26 Mar 2017 13:27:57 -0400, Matt Harbison wrote: > On Sun, 26 Mar 2017 10:35:34 -0400, Yuya Nishihara <yuya@tcha.org> wrote: > > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote: > >> # HG changeset patch > >> # User Matt Harbison <matt_harbison@yahoo.com> > >> # Date 1490464217 14400 > >> # Sat Mar 25 13:50:17 2017 -0400 > >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 > >> # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 > >> color: stop mutating the default effects map > > > > I re-read the code, and noticed that both _effects and w32effects are > > mostly > > constants. Perhaps we can simply switch them by ui._colormode. > > > > if ui._colormode == 'win32': > > activeeffects = w32neffects > > else: > > activeeffects = _effects > > ... embed escape sequences by using activeeffects ... > > Will we get into trouble with each ui instance having a _colormode, but > the activeeffects map effectively being global? Yeah, maybe. But we can select either w32effects or _effects only by ui._colormode. Unlike ui._styles, effects table is not user configurable, which probably means we don't have to store a copy of effects in ui. > I don't fully understand > the purpose of all the separate ui objects. That wouldn't be practically important for color, but it isn't nice to mutate globals depending on ui.
On 03/26/2017 04:35 PM, Yuya Nishihara wrote: > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote: >> # HG changeset patch >> # User Matt Harbison <matt_harbison@yahoo.com> >> # Date 1490464217 14400 >> # Sat Mar 25 13:50:17 2017 -0400 >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 >> # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 >> color: stop mutating the default effects map > > I re-read the code, and noticed that both _effects and w32effects are mostly > constants. Perhaps we can simply switch them by ui._colormode. > > if ui._colormode == 'win32': > activeeffects = w32neffects > else: > activeeffects = _effects > ... embed escape sequences by using activeeffects ... Ultimately, yes we want to be there, but I remember other things muding with the effects dict so someone would have to give a close look to the current magic before moving forward.
On Mon, 27 Mar 2017 14:41:46 +0200, Pierre-Yves David wrote: > On 03/26/2017 04:35 PM, Yuya Nishihara wrote: > > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote: > >> # HG changeset patch > >> # User Matt Harbison <matt_harbison@yahoo.com> > >> # Date 1490464217 14400 > >> # Sat Mar 25 13:50:17 2017 -0400 > >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 > >> # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 > >> color: stop mutating the default effects map > > > > I re-read the code, and noticed that both _effects and w32effects are mostly > > constants. Perhaps we can simply switch them by ui._colormode. > > > > if ui._colormode == 'win32': > > activeeffects = w32neffects > > else: > > activeeffects = _effects > > ... embed escape sequences by using activeeffects ... > > Ultimately, yes we want to be there, but I remember other things muding > with the effects dict so someone would have to give a close look to the > current magic before moving forward. Maybe that is debugcolor (and w32effects.) % grep _effects **/*.py mercurial/color.py:_effects = { mercurial/color.py: _effects.update(w32effects) mercurial/color.py: return ((not ui._terminfoparams and effect in _effects) mercurial/color.py: '''Helper function for render_effects().''' mercurial/color.py:def _render_effects(ui, text, effects): mercurial/color.py: start = [str(_effects[e]) for e in ['none'] + effects.split()] mercurial/color.py: stop = '\033[' + str(_effects['none']) + 'm' mercurial/color.py: msg = '\n'.join([_render_effects(ui, line, effects) mercurial/debugcommands.py: for effect in color._effects.keys():
Patch
diff --git a/mercurial/color.py b/mercurial/color.py --- a/mercurial/color.py +++ b/mercurial/color.py @@ -233,12 +233,14 @@ if mode == realmode and ui.formatted(): ui.warn(_('warning: failed to set color mode to %s\n') % mode) + ui._coloreffects = _effects.copy() + if realmode == 'win32': ui._terminfoparams.clear() if not w32effects: modewarn() return None - _effects.update(w32effects) + ui._coloreffects = w32effects.copy() elif realmode == 'ansi': ui._terminfoparams.clear() elif realmode == 'terminfo': @@ -273,7 +275,7 @@ def valideffect(ui, effect): 'Determine if the effect is valid or not.' - return ((not ui._terminfoparams and effect in _effects) + return ((not ui._terminfoparams and effect in ui._coloreffects) or (effect in ui._terminfoparams or effect[:-11] in ui._terminfoparams)) @@ -324,9 +326,9 @@ for effect in ['none'] + effects.split()) stop = _effect_str(ui, 'none') else: - start = [str(_effects[e]) for e in ['none'] + effects.split()] + start = [str(ui._coloreffects[e]) for e in ['none'] + effects.split()] start = '\033[' + ';'.join(start) + 'm' - stop = '\033[' + str(_effects['none']) + 'm' + stop = '\033[' + str(ui._coloreffects['none']) + 'm' return _mergeeffects(text, start, stop) _ansieffectre = re.compile(br'\x1b\[[0-9;]*m') diff --git a/mercurial/ui.py b/mercurial/ui.py --- a/mercurial/ui.py +++ b/mercurial/ui.py @@ -157,6 +157,7 @@ # Blocked time self.logblockedtimes = False # color mode: see mercurial/color.py for possible value + self._coloreffects = {} self._colormode = None self._terminfoparams = {} self._styles = {} @@ -176,6 +177,7 @@ self.environ = src.environ self.callhooks = src.callhooks self.insecureconnections = src.insecureconnections + self._coloreffects = src._coloreffects.copy() self._colormode = src._colormode self._terminfoparams = src._terminfoparams.copy() self._styles = src._styles.copy()
# HG changeset patch # User Matt Harbison <matt_harbison@yahoo.com> # Date 1490464217 14400 # Sat Mar 25 13:50:17 2017 -0400 # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874 # Parent 22533c3af63d5a67d9210596eafd4b99ab9c7904 color: stop mutating the default effects map A future change will make color.setup() callable a second time when the pager is spawned, in order to honor the 'color.pagermode' setting. The problem was that when 'color.mode=auto' was resolved to 'win32' in the first pass, the default ANSI effects were overwritten, making it impossible to honor 'pagermode=ansi'. Also, the two separate maps didn't have the same keys. The symmetric difference is 'dim' and 'italic' (from ANSI), and 'bold_background' (from win32). Thus, the update left entries that didn't belong for the current mode. As an added bonus, this now correctly enables color with MSYS `less` for a command like this, where pager is forced on: $ hg log --config color.pagermode=ansi --pager=yes Previously, the output was corrupted. The raw output, as seen through the ANSI blind `more.com` was: <-[-1;6mchangeset: 34840:50bcb95d40f5<-[-1m ... which MSYS `less -FRX` rendered as: 1;6mchangeset: 34840:50bcb95d40f51m ... (The two '<-' instances were actually an arrow character that TortoiseHg warned couldn't be encoded, and notepad++ translated to a single '?'.)