[PATCH V4] ui: add new config flag for interface selection
Augie Fackler
raf at durin42.com
Sat Feb 13 18:37:53 EST 2016
> On Feb 12, 2016, at 10:40 AM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>
>
>
> On 02/11/2016 05:14 PM, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon at fb.com>
>> # Date 1455210657 28800
>> # Thu Feb 11 09:10:57 2016 -0800
>> # Node ID 53494a3e515b7744759dfa064685b1b8299fffb7
>> # Parent 01a5143cd25f285f8c745a92986cd7186bb32c90
>> ui: add new config flag for interface selection
>>
>> This patch introduces a new config flag ui.interface to select the interface
>> for interactive commands. It currently only applies to chunks selection.
>> The config can be overridden on a per feature basis with the flag
>> ui.interface.<feature>.
>>
>> features for the moment can only be 'chunkselector', moving forward we expect
>> to have 'histedit' and other commands there.
>>
>> If an incorrect value is given to ui.interface we print a warning and use the
>> default interface: text. If HGPLAIN is specified we also use the default
>> interface: text.
>
> I think we should have a "auto" (name may varies) value, pick the currently best interface per sub feature.
I vehemently disagree with this idea, as I’ve stated before. I strongly feel that curses or other such horrors should be opt-in (see also the pain we’re experiencing with progress, color, and prompts being broken on Windows, if you want something other than opinion.)
(other than this point, I don’t disagree with pyd's review)
> For example we already have a "curses" ui in storage for histedit, but is definitly not polished enough yet. We'll want to use curses for chunk selection as a default long before we might want curses ui for histedit.
>
> That would be the default. Don't update your patch for that, we can follow up on that if people agree.
>
>> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
>> --- a/mercurial/help/config.txt
>> +++ b/mercurial/help/config.txt
>> @@ -1587,6 +1587,17 @@
>> ``interactive``
>> Allow to prompt the user. (default: True)
>>
>> +``interface``
>> + Select the default interface for interactive features (default: text).
>> + Possible values are 'text' and 'curses'.
>> +
>> +``interface.chunkselector``
>> + Select the interface for chunkselection.
>
> "override interactive interface choice for chunk selection"
>
> I think we should also explain what is "chunk selection" here. Most people will not be able to link this with "hg commit -i".
>
> Maybe "chunk selection" should be "patch edition" ? (very low confidence guess, juste hitting at the idea of having a different name).
>
>> + Possible values are 'text' and 'curses'.
>> + This config overrides the interface specified by ui.interface.
>> + If ui.interface and ui.interface.chunkselector aren't set, we use the text
>> + interface.
>
> I think the last sentence is implies in the default of ui.interface and can be dropped.
>
>> ``logtemplate``
>> Template string for commands that print changesets.
>>
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -699,6 +699,73 @@
>> return False
>> return util.isatty(fh)
>>
>> + def interface(self, feature):
>> + '''what interface to use for interactive console features?
>
> small nits: we usually use """ for docstring (not ''')
>
>> +
>> + The interface is controlled by the value of `ui.interface` but also by
>> + the value of feature-specific configuration. For example:
>> +
>> + ui.interface.histedit = text
>> + ui.interface.chunkselector = curses
>> +
>> + Here the features are "histedit" and "chunkselector".
>> +
>> + The configuration above means that the default interfaces for commands
>> + is curses, the interface for histedit is text and the interface for
>> + selecting chunk is crecord (the best curses interface available).
>> +
>> + Consider the following exemple:
>> + ui.interface = curses
>> + ui.interface.histedit = text
>> +
>> + Then histedit will use the text interface and chunkselector will use
>> + the default curses interface (crecord at the moment).
>> + '''
>> + if self.plain():
>> + return "text"
>> +
>> + alloweddefaultinterfaces = ("text", "curses")
>> + defaultfeaturesinterfaces = {
>> + "chunkselector": {
>> + "text": "text",
>> + "curses": "crecord"
>
> Why do we need to introduce a mention of crecord here? Could we do all this with "curses" and introduce the notion of value's hierarchy later?
>
> small nits: verylowvariablenamestarttohurtreadability.
>
>> + }
>> + }
>> +
>> + # Default interface for all the features
>> + defaultinterface = "text"
>> + i = self.config("ui", "interface", None)
>> + if i is not None:
>> + if i not in alloweddefaultinterfaces:
>> + self.warn(_("invalid value for ui.interface: %s\n") % i)
>> + self.warn(_("(using default interface instead %s)\n")
>> + % defaultinterface)
>
> We rarely do double line message. Could we fit this on a single line?
>
>> + else:
>> + defaultinterface = i
>
> This is going to be quite confusing from a code flow perspective. You just move the value of a non-default interface into a variable called "defaultinterface"
>
> I would use
>
> interface = self.config(…)
> if invalide(interface):
> warn()
> interface = defaultinterface
>
> instead.
>
> You can also get rid of the 'None' testing by using 'defaultinterface' as the fallback value of self.config(…)
>
>
>> + # Feature-specific interface
>> + if feature not in defaultfeaturesinterfaces.keys():
>> + # Programming error, not user error
>> + raise ValueError("Unknown feature requested %s" % feature)
>
> If we skip remove the crecord thingy, in my opinion we can drop this entirely.
>
> interface('clownyfeature') would return the value in `ui.interface`. And possible value of ui.interface MUST be understood by the underlying feature.
>
>> + availablefeatureinterfaces = defaultfeaturesinterfaces[feature].keys()
>> + defaultinterface = defaultfeaturesinterfaces[feature][defaultinterface]
>> +
>> + f = self.config("ui", "interface.%s" % feature, None)
>> + if f is None:
>> + return defaultinterface
>
> Again, we could just use 'defaultinterface' as the fallback value.
>
>> + else:
>> + if f not in availablefeatureinterfaces:
>> + self.warn(_("invalid value for ui.interface.%s: %s\n")
>> + % (feature, f))
>> + self.warn(_("(using default interface instead: %s)\n")
>> + % defaultinterface)
>
> ditto about double line warning.
>
>> + return defaultinterface
>> + else:
>> + return defaultfeaturesinterfaces[feature][f]
>> +
>> def interactive(self):
>> '''is interactive input allowed?
>
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160213/4398baf2/attachment.sig>
More information about the Mercurial-devel
mailing list