[PATCH V4] ui: add new config flag for interface selection
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Fri Feb 12 10:40:00 EST 2016
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 subfeature. 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
More information about the Mercurial-devel
mailing list