[PATCH 2 of 7] color: initialize color for local peer ui

[Pierre-Yves David]

On 03/01/2017 02:03 PM, Yuya Nishihara wrote:
> On Tue, 28 Feb 2017 16:44:36 +0100, Pierre-Yves David wrote:
>> On 02/28/2017 03:29 PM, Yuya Nishihara wrote:
>>> On Tue, 28 Feb 2017 11:35:03 +0100, Pierre-Yves David wrote:
>>>> On 02/28/2017 11:24 AM, Pierre-Yves David wrote:
>>>>> On 02/28/2017 07:57 AM, Martin von Zweigbergk wrote:
>>>>>> On Mon, Feb 27, 2017 at 6:59 AM, Pierre-Yves David
>>>>>> <pierre-yves.david at ens-lyon.org> wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>>>>>>> # Date 1488044041 -3600
>>>>>>> #      Sat Feb 25 18:34:01 2017 +0100
>>>>>>> # Node ID c3224694bdae9cdb7530f952e2c767e419b7f280
>>>>>>> # Parent  92526381242cd381375a465d5a800446916d2d7b
>>>>>>> # EXP-Topic color
>>>>>>> color: initialize color for local peer ui
>>>>>>>
>>>>>>> The local peer
>>>>>>
>>>>>> "local peer" or "localrepository"? The patch seems to be in "class
>>>>>> localrepository".
>>>>>
>>>>> hum, good catch. Things seems clowner than I expected. It looks like we
>>>>> don't use the "lui" (local ui, goes through uisetup) to create the
>>>>> repository but the "baseui" (does not goes through uisetup).
>>>>>
>>>>> Let me grab a shovel and go bad into that code.
>>>>
>>>> Okay so in short "ui initialisation business is not simple". That
>>>> description should be:
>>>>
>>>> color: initialize color for the localrepo ui
>>>>
>>>> The 'ui' object dedicated to a 'localrepo' is independant from the one
>>>> available in dispatch (and 'uisetup'). In addition, it is created from
>>>> the 'baseui' (for good reason apparently). For this reason, we need to
>>>> run the color setup on it after the local repository config is read.
>>>
>>> Maybe we can move color.setup() to ui.fixconfig(), where ui attributes are
>>> updated reflecting to config changes.
>>
>> I considered it but it did not seemed appropriate. Fixconfig is run
>> quite often and the setup function is erasing most internal state
>> related to color. I was not confident make that change. I'm not saying
>> it is entirely impossible but that requires a careful review of the
>> setup process and other fixconfig related thing. So I would rather not
>> go this route for now. In particular, running during 'fixconfig' would
>> mean running it 'too early' before the extensions are loaded (they might
>> change the default style.
>
> Good point and agreed.
>
> It would be nice if we can get rid of the color dependency from localrepo
> in future.

There is two majors tasks in the way of having the color initialization 
more contained to the 'ui' class itself.

First, extensions can define a 'colortable' that augment the 
'_defaultstyles' mapping. This creates an order dependency between the 
initialization of extensions and the setup of color on the 'ui' object. 
That dependency makes things harder since we have to be careful to not 
initialize color too early.
One easy way to remove this dependency is to split the style config in 
two layers. One "default" layer that would be "immutable" from a "
ui" point of view (and updated by the extensions). And one extra layer 
at the ui level for change made through the configuration. When a style 
is requested, the local override would be searched before querying the 
global one. That was, update made by extensions to the global 
'_defaultstyle' could happen -after- the color is initialized on a 'ui' 
object.

Second, the current initialization of the color mode is meant as a 
one-time run and tend to modify, update and clear various mapping. It 
should me possible to update it to be a simple dispatch of value in a 
way that allow to setup the mode multiple time over the life of an 'ui' 
object. That should not be too hard but someone has to look into it.

I'm not planning to do that clean up for now since I've enough on my 
plate already. But I hope this message will hope someone clearing this 
in the future (maybe a me further in the future).

Cheers,

-- 
Pierre-Yves David