[GRASS-dev] r45050: v.info: -r/-m/-t -> shell param; GEM; parser.c

Hi,

re. https://trac.osgeo.org/grass/changeset/45050

grass has certain conventions it uses through many modules. having
all modules work the same way (or near to it) makes it much easier
to pick up new modules and have them work as expected, which
makes grass's reputedly harsh learning curve less so. what's the
justification for introducing non-standard behavior into v.info?

(messing up the modules' CLIs to improve GUI cosmetics makes me
rather grumpy..)

also, re. https://trac.osgeo.org/grass/changeset/45039,
is GEM fundamentally broken on WinGrass or is it buggy or just
a personal decision to disable that lesser used feature?

Mostly I just ask please discuss these things with the list
first. Especially for anything that touches parser.c* and the
core Makefile in the release branch during RC.

[*] even though I'm overjoyed to see "--help" finally parsed in
the dev branches

somewhat related, I object to this change:

r44435 releasebranch_6_4/lib/gis/parser.c:
  call G_usage() when level is verbose (merge r44433 from trunk)

it you got the usage wrong, and you didn't use --quiet, the
obvious thing you'll do next is to look up the correct one...
--verbose to get at it is not obvious, so you waste time finding
the right manual. theoretically the GUIs should never get the
usage wrong as they get it from the --interface-description.

and fwiw this one I think makes things more confusing, as it
implies `dirname` (vs `basename`), when the correct usage is
like [/path/to/]filename.ext.
https://trac.osgeo.org/grass/changeset/44339/grass/branches/releasebranch_6_4/lib/gis/parser.c

hoping for improved discussion,
thanks,
Hamish

Hi,

2011/1/17 Hamish <hamish_b@yahoo.com>:

re. https://trac.osgeo.org/grass/changeset/45050

grass has certain conventions it uses through many modules. having
all modules work the same way (or near to it) makes it much easier
to pick up new modules and have them work as expected, which
makes grass's reputedly harsh learning curve less so. what's the
justification for introducing non-standard behavior into v.info?

are you referring to GRASS6? If so, why don't change some "standard"
or "conventions" in GRASS7? :wink:

I remember discussion about number of flags of g.region. There was
suggestion to reduce number of flags as I have done for v.info. The
CLI follows clear design (group related flags to the one option). In
the case of v.info -r/-m(and new -g) to `shell=basic,region,topo`. GUI
is also much more intuitive in the result (try it out). Same can be
done for g.region, which ended up with extremely large number of
flags. The goal is not to break conventions, just to make interface of
GRASS modules more intuitive for the end-user. Of course it's question
for discussion.

(messing up the modules' CLIs to improve GUI cosmetics makes me
rather grumpy..)

In which way the CLI of v.info has been messed up?

also, re. https://trac.osgeo.org/grass/changeset/45039,
is GEM fundamentally broken on WinGrass or is it buggy or just
a personal decision to disable that lesser used feature?

Mostly I just ask please discuss these things with the list
first. Especially for anything that touches parser.c* and the
core Makefile in the release branch during RC.

gcc -g -Wall -pedantic -Werror-implicit-function-declaration
-fno-common -o tools.o -c tools.c
tools.c: In function `dump_plain':
tools.c:572: error: implicit declaration of function `mkstemp'
make: *** [tools.o] Error 1

Probably gnulib is needed. No time to fix it (and no interest), nobody
has fixed during the last year(s). Who is using GEM?

[*] even though I'm overjoyed to see "--help" finally parsed in
the dev branches

somewhat related, I object to this change:

r44435 releasebranch_6_4/lib/gis/parser.c:
call G_usage() when level is verbose (merge r44433 from trunk)

it you got the usage wrong, and you didn't use --quiet, the
obvious thing you'll do next is to look up the correct one...
--verbose to get at it is not obvious, so you waste time finding
the right manual. theoretically the GUIs should never get the
usage wrong as they get it from the --interface-description.

OK, you are right that calling G_usage() should not be related the
verbosity level. I was always embarrassed with G_usage().

$ r.info input=x --v
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
        (Name of raster map)

Description:
Output basic information about a raster map layer.

Keywords:
raster, metadata

Usage:
r.info [-rstghudmp] map=name [--verbose] [--quiet]

Flags:
  -r Print range only
  -s Print raster map resolution (NS-res, EW-res) only
  -t Print raster map type only
  -g Print map region only
  -h Print raster history instead of info
  -u Print raster map data units only
  -d Print raster map vertical datum only
  -m Print map title only
  -p Print raster map timestamp (day.month.year hour:minute:seconds) only
--v Verbose module output
--q Quiet module output

Parameters:
  map Name of raster map

You need to scroll up to find out what is wrong. Extremely annoying.
Now it ends up with clear message

$ r.info input=x
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
        (Name of raster map)

You get clear info what is wrong. Probably the user could be inform
how to get the usage info. Something like

For module's syntax information call `r.info --help`.

Hm, about the GUI, what about launching commands from Layer Manager
command line? Easy to make a mistake.

and fwiw this one I think makes things more confusing, as it
implies `dirname` (vs `basename`), when the correct usage is
like [/path/to/]filename.ext.
https://trac.osgeo.org/grass/changeset/44339/grass/branches/releasebranch_6_4/lib/gis/parser.c

Sorry I don't understand, this parameter takes path to the file
(relative or absolute)...

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

2011/1/17 Martin Landa <landa.martin@gmail.com>:

[...]

You get clear info what is wrong. Probably the user could be inform
how to get the usage info. Something like

For module's syntax information call `r.info --help`.

Hm, about the GUI, what about launching commands from Layer Manager
command line? Easy to make a mistake.

done in r45064 (backported to grass65). If you have no objections I
would suggest backport to relbr64.

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

Hamish:

> re. https://trac.osgeo.org/grass/changeset/45050

...

> what's the justification for introducing non-standard
> behavior into v.info?

Martin:

are you referring to GRASS6?

both 6 and 7.

If so, why don't change some "standard" or "conventions" in
GRASS7? :wink:

because there's nothing really wrong with the old way?
If there seem to many buttons in the GUI for a module,
maybe the GUI module creation code should try for two columns
or something? personally I am not too overwhelmed by the
v.info and g.region 'Print' tabs, IMO they are fine and easy.

I remember discussion about number of flags of g.region.
There was suggestion to reduce number of flags as I have
done for v.info.

it wasn't just discussion, it was committed and became a horrible
mess for 2-3 months before it got partially reverted/cleaned up,
and maybe now is still not as clean and clear as it once was.
IIRC it was having a single -g flag which changed the behavior
of all the other print flags. partly I'm trying to avoid that
mess from happening again. a good idea perhaps, but it just
didn't work in practice and made the entire GIS harder to use
because it introduced a new paradigm.

The CLI follows clear design (group related flags to the one
option). In the case of v.info -r/-m(and new -g) to
`shell=basic,region,topo`.

no, what I mean is that for something like 25-100 popular modules,
the convention is to use -g to print shell style, -r to print
range etc. mostly only modules which print reports or dump
topology or so will use an option=print for that, but they are
quite the exception. the convention I am talking about is that
flags are used to print something and exit. If you use multiple
flags and you get both e.g. range and bounds. ('r.info -rg')
If you know this, or learn it for 2-3 modules, you magically
know how to use another 22-97 modules which act in the same way.

GUI is also much more intuitive in the result (try it out).

It is a matter of taste and I don't find that way more intuitive,
but for the sake of argument say it was so. That's for individual
modules, but it breaks with how all the other modules act printing
shell style, so makes the entire GIS suite less consistent. Which
harms learning both the CLI /and/ the GUI, even if it's more
subtle in the GUI, the inconsistency is still there.

Also having it as multiple=yes makes a bit of a mess because
then you can't use a pull down menu for that, which leads to
typos and wasted keystrokes/time. and well, I'll be honest:
personally I really like stacking the flags on the command line.
it's quite an efficient and clean way to write scripts.

Same can be done for g.region, which ended up with extremely
large number of flags. The goal is not to break conventions,
just to make interface of GRASS modules more intuitive for the
end-user. Of course it's question for discussion.

> (messing up the modules' CLIs to improve GUI cosmetics
> makes me rather grumpy..)

In which way the CLI of v.info has been messed up?

- the convention of passing letter flags to print a list of
things you want quickly and easily is now broken.
- the module now needs a lot more keystrokes to accomplish
the same thing.

> also, re. https://trac.osgeo.org/grass/changeset/45039,
> is GEM fundamentally broken on WinGrass or is it buggy

...

tools.c: In function `dump_plain':
tools.c:572: error: implicit declaration of function
`mkstemp'

somewhat both buggy & UNIXisms:
https://trac.osgeo.org/grass/ticket/930#comment:6

again, I don't really mind GEM being switched off in WinGrass,
just that the disabling of large features should come with more
notification. anyway moving on,

...

> it you got the usage wrong, and you didn't use --quiet, the
> obvious thing you'll do next is to look up the correct one...
> --verbose to get at it is not obvious, so you waste time
> finding the right manual. theoretically the GUIs should
> never get the usage wrong as they get it from the
--interface-description.

OK, you are right that calling G_usage() should not be
related the verbosity level.

I didn't say that. if it is run by default, --quiet should avoid
it. But the suggestion to use '--help' instead of G_usage()
is good & covers both of our concerns I think.

I was always embarrassed with G_usage().

I've learned to accept my many typos without embarrassment :slight_smile:
if you type fast enough people don't notice. :slight_smile:
public thinkos are the embarrassing ones..

$ r.info input=x --v
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
        (Name of raster map)

[...]

You need to scroll up to find out what is wrong. Extremely
annoying.

much less annoying than having to look up the manual (if you
didn't know about --help). and it is slightly less annoying to
spin the mouse wheel than having to retype 'g.module --help'.

Now it ends up with clear message

$ r.info input=x
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
        (Name of raster map)

You get clear info what is wrong.

*only* if the thing you got wrong was a required option. if it
was clean=toolname spelt wrong you'd have to look it up. grass
is just too big to remember all of these things even after you're
comfortable on the command line. I'm often looking up the usage
for modules I wrote myself when I misremember them.. (eg was it
'mean' or 'average'? 'm' or 'meter'?)

fwiw ISTR Radim wrote up a clear doc of when map= should be used
and when input= should be used, and it made a lot of sense. I
vaguely remember it was outside of the main doc places, I'll
try to find it.

Hm, about the GUI, what about launching commands from Layer
Manager command line? Easy to make a mistake.

same issues. if you made a mistake the next logical thing is
to look up what the correct way is. If that's already done for
you, great, you can solve the problem faster.

[...]

Index: grass/branches/releasebranch_6_4/lib/gis/parser.c

--- a/grass/branches/releasebranch_6_4/lib/gis/parser.c
+++ b/grass/branches/releasebranch_6_4/lib/gis/parser.c
@@ -632,16 +632,16 @@
         Opt->key = "input";
         Opt->type = TYPE_STRING;
- Opt->key_desc = "name";
+ Opt->key_desc = "path";
         Opt->required = YES;
         Opt->gisprompt = "old_file,file,input";
- Opt->description = _("Name of input file");
+ Opt->description = _("Path to input file");
         break;
     case G_OPT_F_OUTPUT:
         Opt->key = "output";
         Opt->type = TYPE_STRING;
- Opt->key_desc = "name";
+ Opt->key_desc = "path";
         Opt->required = YES;
         Opt->gisprompt = "new_file,file,output";
- Opt->description = _("Name for output file");
+ Opt->description = _("Path for output file");
         break;
     case G_OPT_F_SEP:

Hamish:

> and fwiw this one I think makes things more confusing,
> as it implies `dirname` (vs `basename`), when the correct
> usage is like [/path/to/]filename.ext.

Martin:

Sorry I don't understand, this parameter takes path to the
file (relative or absolute)...

it takes both the path and the file name. the path is optional
while the file name is not. The change makes it seem like the
path is required but the filename is optional. in absence of
experience, many new users will take these words literally and
become very confused.

Partly I complain because we have been very loose with "bug fixes
only in the 6.4 branch"; especially parser.c and gisdefs.h need a
very good reason to touch at this point. any change to them is
dangerous.

cheers & thanks,
Hamish

Hi,

2011/1/18 Hamish <hamish_b@yahoo.com>:

If so, why don't change some "standard" or "conventions" in
GRASS7? :wink:

because there's nothing really wrong with the old way?
If there seem to many buttons in the GUI for a module,
maybe the GUI module creation code should try for two columns
or something? personally I am not too overwhelmed by the
v.info and g.region 'Print' tabs, IMO they are fine and easy.

well, the number of flags is the same as the number of introduced
options (shell parameter in v.info case), so number of checkboxes in
GUI dialogs is not changed. They are just logically grouped.

I remember discussion about number of flags of g.region.
There was suggestion to reduce number of flags as I have
done for v.info.

it wasn't just discussion, it was committed and became a horrible
mess for 2-3 months before it got partially reverted/cleaned up,
and maybe now is still not as clean and clear as it once was.
IIRC it was having a single -g flag which changed the behavior
of all the other print flags. partly I'm trying to avoid that
mess from happening again. a good idea perhaps, but it just
didn't work in practice and made the entire GIS harder to use
because it introduced a new paradigm.

Probably time to make it better :wink:

Beside joking I would incline to keep -p/-g flag and content control
by the parameter.

-p Print in human readable format
-g Print in shell script style

Anyway there only few flags in g.region which could be possible
grouped into options of the new parameter (-e,-c,-3).

print=basic,extent,center,3d

What do you think about that?

[...]

Speaking about v.info to add -g flag and content control by

print=basic,extent,topo

[...]

Same can be done for r.info

print=range,resolution,type,region,units,datum,title

r.info print=range,type

would print only range and type.

r.info -g print=units,title

would print units and title in shell script style.

?

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

Hi,

2011/1/18 Hamish <hamish_b@yahoo.com>:

[...]

Index: grass/branches/releasebranch_6_4/lib/gis/parser.c

--- a/grass/branches/releasebranch_6_4/lib/gis/parser.c
+++ b/grass/branches/releasebranch_6_4/lib/gis/parser.c
@@ -632,16 +632,16 @@
Opt->key = "input";
Opt->type = TYPE_STRING;
- Opt->key_desc = "name";
+ Opt->key_desc = "path";
Opt->required = YES;
Opt->gisprompt = "old_file,file,input";
- Opt->description = _("Name of input file");
+ Opt->description = _("Path to input file");
break;
case G_OPT_F_OUTPUT:
Opt->key = "output";
Opt->type = TYPE_STRING;
- Opt->key_desc = "name";
+ Opt->key_desc = "path";
Opt->required = YES;
Opt->gisprompt = "new_file,file,output";
- Opt->description = _("Name for output file");
+ Opt->description = _("Path for output file");
break;
case G_OPT_F_SEP:

Hamish:

> and fwiw this one I think makes things more confusing,
> as it implies `dirname` (vs `basename`), when the correct
> usage is like [/path/to/]filename.ext.

Martin:

Sorry I don't understand, this parameter takes path to the
file (relative or absolute)...

it takes both the path and the file name. the path is optional
while the file name is not. The change makes it seem like the
path is required but the filename is optional. in absence of
experience, many new users will take these words literally and
become very confused.

Partly I complain because we have been very loose with "bug fixes
only in the 6.4 branch"; especially parser.c and gisdefs.h need a
very good reason to touch at this point. any change to them is
dangerous.

filename can be seen as a relative path in this case. Anyway you can
be right that `path` can be confusing for the user. No problem for me
to revert this change.

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

Hi,

2011/1/18 Hamish <hamish_b@yahoo.com>:

[...]

> it you got the usage wrong, and you didn't use --quiet, the
> obvious thing you'll do next is to look up the correct one...
> --verbose to get at it is not obvious, so you waste time
> finding the right manual. theoretically the GUIs should
> never get the usage wrong as they get it from the
--interface-description.

OK, you are right that calling G_usage() should not be
related the verbosity level.

I didn't say that. if it is run by default, --quiet should avoid
it. But the suggestion to use '--help' instead of G_usage()
is good & covers both of our concerns I think.

I was always embarrassed with G_usage().

I've learned to accept my many typos without embarrassment :slight_smile:
if you type fast enough people don't notice. :slight_smile:
public thinkos are the embarrassing ones..

$ r.info input=x --v
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
(Name of raster map)

[...]

You need to scroll up to find out what is wrong. Extremely
annoying.

From my personal experience the current behaviour (printed errors and

then usage) is not useful in the most cases. G_usage() usually didn't
give me relevant information and I always needed to scroll up to catch
up error messages (which parameter is missing, which typos I have
done, etc.). In the best case the error messages should be printed
after calling G_usage(). But it would require to store them in the
array and print them later after G_usage() is called.

[...]

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

Hi,

2011/1/26 Martin Landa <landa.martin@gmail.com>:

$ r.info input=x --v
Sorry, <input> is not a valid parameter
ERROR: Required parameter <map> not set:
(Name of raster map)

[...]

You need to scroll up to find out what is wrong. Extremely
annoying.

From my personal experience the current behaviour (printed errors and
then usage) is not useful in the most cases. G_usage() usually didn't
give me relevant information and I always needed to scroll up to catch
up error messages (which parameter is missing, which typos I have
done, etc.). In the best case the error messages should be printed
after calling G_usage(). But it would require to store them in the
array and print them later after G_usage() is called.

please check & review r45212.

Martin

--
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa

Martin:

>> If so, why don't change some "standard" or "conventions" in
>> GRASS7? :wink:

Hamish:

> because there's nothing really wrong with the old way?
> If there seem to many buttons in the GUI for a module,
> maybe the GUI module creation code should try for two columns
> or something? personally I am not too overwhelmed by the
> v.info and g.region 'Print' tabs, IMO they are fine and easy.

Martin:

well, the number of flags is the same as the number of
introduced options (shell parameter in v.info case), so
number of checkboxes in GUI dialogs is not changed.

so it has been made more logically complicated for exactly what
gain then?

I find the "-x option does X, unless you have some modifier
enabled, in which it does Y" a lot more confusing than "the -x
flag does X, and the -y flag does Y". It is one less layer of
abstraction to get your head around, which means easier to learn,
debug, etc.

They are just logically grouped.

they already should be, by the order they come in the code.
just there is no visual breaks between groupings.

but this gives me another idea on how we might find a solution..

so the problem you are trying to fix is not that the the user
is presented with an overwhelming 747-cockpit's worth of options
(such as d.vect can look like), but rather that within tabs it
would help to group collections of similar options together?
?

if so perhaps we could implement a new thing in the module GUIs:
thin 1 pixel wide light blue borders around groups of similar
options/flags within a single tab.

this could either be with a new Option,Flag struct item like
->gui_group or ->gui_subsection, or perhaps it is cleaner and
nicer to parse the existing opt->guisection like:
_("section;subsection;subsubsection")
if additional [;...] are present, the items get grouped.

if ->gui_group or ->gui_subsection was used any opts/flags
within the same tab with the same group would be lumped together
within a group box (optionally with small blue text in a gap in
the top left border of the box with the group's name?).

I'm sure you can figure out what I mean as most gui widget kits
support this & it is quite common, if not I can mock up a
screenshot.

>> I remember discussion about number of flags of g.region.
>> There was suggestion to reduce number of flags as
>> I have done for v.info.
>
> it wasn't just discussion, it was committed and became a
> horrible mess for 2-3 months before it got partially
> reverted/cleaned up, and maybe now is still not as clean and
> clear as it once was.
> IIRC it was having a single -g flag which changed the behavior
> of all the other print flags. partly I'm trying to avoid that
> mess from happening again. a good idea perhaps, but it just
> didn't work in practice and made the entire GIS harder to use
> because it introduced a new paradigm.

Probably time to make it better :wink:

as above, the modifier-flag way of doing it adds another layer of
complication to learning the modules and in the code, which are
both something I am not in favour of doing if we can help it.
keep it simple.

Beside joking I would incline to keep -p/-g flag and
content control by the parameter.

-p Print in human readable format
-g Print in shell script style

Anyway there only few flags in g.region which could be
possible grouped into options of the new parameter (-e,-c,-3).

print=basic,extent,center,3d

What do you think about that?

as modifiers, or just to remove some flags?
g.region is an extreme case and I'd have to eat something to get
my blood sugar up before really being able to think about it
clearly. :slight_smile:

Speaking about v.info to add -g flag and content control by
print=basic,extent,topo
[...]
Same can be done for r.info
print=range,resolution,type,region,units,datum,title

r.info print=range,type
would print only range and type.

r.info -g print=units,title
would print units and title in shell script style.

?

it's the same thing that was tried earlier for g.region. :frowning:
also it means offering non-shell script style for everything
which is current shell style only.
is:
max: 1.2345
any clearer than
max=1.2345
in the output window? Maybe a tiny bit but I'm not sure.

in general, as you may have guessed :slight_smile: I don't like the modifier
flag way of doing things at all.

one thing it would depend upon (and would be useful anyway) is
the ability to ctrl-click options in the GUI option pulldown menu
allow multiple=yes in the GUI. Can that be done currently?
still to do that is a trick which must be learned.
easy to figure it out from experience, but experience !=
intuition.

another argument against all this is that it forces English
language onto something which was previously fully translated.
sure there are other tool= options already which are locked in
English, but we shouldn't make things any worse.

and finally, again, it's a major PITA for command script writers
to have to type out the full textual description when the single
letters already do the job in a very succinct way. we've got so
many things that don't work properly, I'd love if we could focus
on those instead of reinventing stuff which already does work
well.

best,
Hamish

ps- this reminds me of another issue which is not already solved
in a good way :wink: and would be useful: the idea of a DEG_FORMAT=
{D|DM|DMS} g.gisenv variable to control if you get decimal
degrees, DDD:MM.MMMMh, or DDD:MM:SS.SSSSh style output.
presumably worked into G_format_northing() and G_format_easting()
and friends.