[GRASS5] [bug #2488] (grass) GRASS 5.7 WISH - improve string reading ability in g.parser

this bug's URL: http://intevation.de/rt/webrt?serial_num=2488

Request number 2488 was commented on by 'guest' (guest user).
Responding to this message will send mail to the requestor.
      
      Request Tracker
      rt@intevation.de

--------------------------------------------------------------
Cc: grass5@grass.itc.it

adding printf("argc=%d\n", argc); to G_parser() highlights the problem

e.g. 5.7's auto-gen Tcl makes:
button .run -text "Run" -command {
...
    set cmd "| $cmd 2>@ stdout"
    catch {open $cmd r} msg
...
}

so:
set cmd {d.text.freetype text="abc 123" path=font.ttf}
gives argc=4 in G_parser

argv[0] is 'd.text.freetype'
argv[1] is 'text="abc'
argv[2] is '123"'
argv[3] is 'path=font.ttf'

and

set cmd {d.text.freetype text="abc_123" path=font.ttf}
gives argc=3 (and works)

so it is never filtered through a shell for option="a b" to be made one item.
The $cmd string is formed correctly; but not passed to G_parser() correctly.

Hamish

-------------------------------------------- Managed by Request Tracker

guest user via RT wrote:

e.g. 5.7's auto-gen Tcl makes:
button .run -text "Run" -command {
...
    set cmd "| $cmd 2>@ stdout"
    catch {open $cmd r} msg
...
}

A good example of how *not* to to construct command lines in Tcl (or,
for that matter, most languages).

A Unix command consists of a list of strings, not a single string.
There are defined boundaries between arguments, and these need to be
preserved. Fortunately, Tcl's open and exec functions accept Tcl
lists, which makes this relatively easy to achieve (unlike e.g.
system()).

Tcl lists should be constructed and manipulated using the appropriate
functions, e.g. list, lappend, concat etc. They should not be
constructed by including variable expansions in a double-quoted
string.

E.g. in the above, the line:

  set cmd "| $cmd 2>@ stdout"

should be written as:

  set cmd [concat | $cmd 2>@ stdout]

I.e.:
  $ tclsh
  % set cmd {d.text.freetype {text=abc 123} path=font.ttf}
  $ puts $cmd
  d.text.freetype {text=abc 123} path=font.ttf
  % foreach x $cmd {puts $x}
  d.text.freetype
  text=abc 123
  path=font.ttf
  % set cmd [concat | $cmd 2>@ stdout]
  % puts $cmd
  | d.text.freetype {text=abc 123} path=font.ttf 2>@ stdout
  % foreach x $cmd {puts $x}
  |
  d.text.freetype
  text=abc 123
  path=font.ttf
  2>@
  stdout
  %

However, that specific change will only help if the original value of
$cmd is already a well-formed list (i.e. where "text=abc 123" is a
single item rather than a pair of items). Given that most of the Tcl
command-line handling (in both tcltkgrass and G_gui()) which I have
seen is similarly broken, there are probably other places which also
need to be fixed.

so:
set cmd {d.text.freetype text="abc 123" path=font.ttf}
gives argc=4 in G_parser

argv[0] is 'd.text.freetype'
argv[1] is 'text="abc'
argv[2] is '123"'
argv[3] is 'path=font.ttf'

Which is correct. You've set cmd to a list which contains four
elements, which correspond to the four argv values above.

and

set cmd {d.text.freetype text="abc_123" path=font.ttf}
gives argc=3 (and works)

so it is never filtered through a shell for option="a b" to be made one item.
The $cmd string is formed correctly; but not passed to G_parser() correctly.

No it isn't formed correctly. exec (and open where the first word is
the pipe symbol) treat their argument as a *list*, not a string. Well,
everything in Tcl is a string, but it has specific semantics for
strings which represent lists, i.e. any elements which contain spaces
will be contained within braces.

If you construct the *list* using list manipulation functions such as
list, lappend, concat etc, Tcl will preserve the list structure. E.g.

  % set arg1 {d.text.freetype}
  % set arg2 {text=abc 123}
  % set arg3 {path=font.ttf}
  % set string "$arg1 $arg2 $arg3"
  % puts $string
  d.text.freetype text=abc 123 path=font.ttf
  % foreach x $string {puts $x}
  d.text.freetype
  text=abc
  123
  path=font.ttf
  % set list [list $arg1 $arg2 $arg3]
  % puts $list
  d.text.freetype {text=abc 123} path=font.ttf
  % foreach x $list {puts $x}
  d.text.freetype
  text=abc 123
  path=font.ttf

--
Glynn Clements <glynn.clements@virgin.net>

Tcl lists should be constructed and manipulated using the appropriate
functions, e.g. list, lappend, concat etc. They should not be
constructed by including variable expansions in a double-quoted
string.

Ok, thanks for the tips Glynn.

I changed append-> lappend in some places and now it is all working,
AFAICT. (& in CVS)

Using special chars ( {}"$@ ) in the text input box works correctly.

Anybody know of anywhere else it breaks?
  e.g. 5.3; d.m; v.digit settings; startup TclTk screen; etc.

cheers,
Hamish

On 8/3/04 12:43 AM, "Hamish" <hamish_nospam@yahoo.com> wrote:

Ok, thanks for the tips Glynn.

I changed append-> lappend in some places and now it is all working,
AFAICT. (& in CVS)

Using special chars ( {}"$@ ) in the text input box works correctly.

Anybody know of anywhere else it breaks?
  e.g. 5.3; d.m; v.digit settings; startup TclTk screen; etc.

Hamish,

Does this mean that the G.PARSER() problem is generally fixed and we can
enter strings with spaces into the tcl dialogs? Hallelujah! Thanks to both
of you!

The same problem affected d.m in the places to do SQL queries.

It also affected the v.digit startup where you specify a command to provide
a background map. Once you are in v.digit, it is then possible to specify a
background map.

It may (or may not) also be related to NVIZ not starting from the
autogenerated GUI. Currently, you have to start NVIZ from the command line
using the -q switch, then add data after it is running. That is, you can't
specify initial raster, vector, ect. files in the startup GUI dialog. If you
try to start NVIZ this way, it will crash.

I don't remember running into this in 5.3, but the GUI is very different in
5.3; G.PARSER() works but doesn't autogenerate the dialog boxes. They are
each coded individually.

Michael
______________________________
Michael Barton, Professor & Curator
School of Human Origins, Cultures, & Societies
Arizona State University
Tempe, AZ 85287-2402
USA

voice: 480-965-6262; fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

> I changed append-> lappend in some places and now it is all working,
> AFAICT. (& in CVS)

...

Does this mean that the G.PARSER() problem is generally fixed and we
can enter strings with spaces into the tcl dialogs?

yup.

The same problem affected d.m in the places to do SQL queries.

works. (Add vector->SQL Query)

It also affected the v.digit startup where you specify a command to
provide a background map. Once you are in v.digit, it is then possible
to specify a background map.

works. (just another Tcl startup menu like all the rest)

It may (or may not) also be related to NVIZ not starting from the
autogenerated GUI. Currently, you have to start NVIZ from the command
line using the -q switch, then add data after it is running. That is,
you can't specify initial raster, vector, ect. files in the startup
GUI dialog. If you try to start NVIZ this way, it will crash.

still broken:
can not find channel named "couldn't execute "nviz2.2_script": no such
file or directory"

I don't remember running into this in 5.3,

It was a problem in 5.3, but without the SQL queries it only affects the
d.text modules AFAIK. Fixed in CVS. Both the 5.3 and 5.7 changes make
the shell command preview un-cut&paste-able for things with spaces in
them, but this never worked before either.. some fancy formatting before
printing the text string could probably move & replace the {}s to "s if
someone was keen to try.

The error is still in grass51/gui/tcltkgrass/main/gui.tcl but that is
in now unused code which should be ripped out???

Hamish

Michael Barton wrote:

It may (or may not) also be related to NVIZ not starting from the
autogenerated GUI. Currently, you have to start NVIZ from the command line
using the -q switch, then add data after it is running. That is, you can't
specify initial raster, vector, ect. files in the startup GUI dialog. If you
try to start NVIZ this way, it will crash.

I'm fairly sure that particular issue isn't related to Tcl/Tk. The
reports of NVIZ crashing unless you use -q indicate that it crashes if
you specify the arguments on the command line, e.g. "nviz elevation=...",
where Tcl/Tk isn't involved.

I don't remember running into this in 5.3, but the GUI is very different in
5.3; G.PARSER() works but doesn't autogenerate the dialog boxes. They are
each coded individually.

The 5.0/5.3 problems are essentially the same bugs, but in different
code. 5.0/5.3 has tcltkgrass, which is a self-contained Tcl/Tk program
which works by executing GRASS commands.

In 5.7, the problems are in G_gui(), which is called by G_parser(),
which is used by most commands to parse the command line. In 5.0/5.3,
if a command was run without arguments, G_parser() would ask the user
for the infomation via the terminal. In 5.7, it generates and runs a
Tcl/Tk program which displays a dialog, then re-executes the command
using the data from the dialog fields as arguments.

In both cases, you have Tcl/Tk code which attempts to construct and
execute a command line; in both cases, the code was wrong, for
essentially the same reasons.

--
Glynn Clements <glynn.clements@virgin.net>

Hamish wrote:

> It may (or may not) also be related to NVIZ not starting from the
> autogenerated GUI. Currently, you have to start NVIZ from the command
> line using the -q switch, then add data after it is running. That is,
> you can't specify initial raster, vector, ect. files in the startup
> GUI dialog. If you try to start NVIZ this way, it will crash.

still broken:
can not find channel named "couldn't execute "nviz2.2_script": no such
file or directory"

Oh. That *is* a Tcl issue.

The immediate error arises from this code in G_gui():

   catch {open $cmd r} msg
  fconfigure $msg -blocking 0
  fileevent $msg readable [ list prnout $msg ]

If the open command succeeds, catch will set msg to the result from
the command (i.e. the channel ID) and return 0. If the command fails,
catch will set msg to the error message and return non-zero.

IOW, msg will only contain a valid channel ID if catch returns zero,
so the above code should probably look something like:

  if {[catch {open $cmd r} msg]} {
    error $msg
  } {
    fconfigure $msg -blocking 0
    fileevent $msg readable [ list prnout $msg ]
  }

[Or even just omit the call to catch, so that any errors are
propagated via Tcl's normal error handling mechanism.]

At least, that will handle errors correctly.

As for the actual error, that's because G_gui() is using the value of
argv[0] from G_parser(), and argv[0] is "nviz2.2_script", which isn't
an executable file.

For G_gui() to work correctly, NVWISH2.2 needs to call G_parser() with
argv[0] set to "nviz".

> I don't remember running into this in 5.3,

It was a problem in 5.3, but without the SQL queries it only affects the
d.text modules AFAIK. Fixed in CVS. Both the 5.3 and 5.7 changes make
the shell command preview un-cut&paste-able for things with spaces in
them, but this never worked before either.. some fancy formatting before
printing the text string could probably move & replace the {}s to "s if
someone was keen to try.

  set string {}
  foreach word $cmd {
    regsub -all -- {'} $word {'\''} newword
    append string {'} $newword {' }
  }

--
Glynn Clements <glynn.clements@virgin.net>

From: Hamish <hamish_nospam@yahoo.com>
Date: Wed, 04 Aug 2004 13:06:08 +1200
To: Michael Barton <michael.barton@asu.edu>
Cc: <grass-bugs@intevation.de>, <grass5@grass.itc.it>
Subject: Re: [GRASS5] [bug #2488] (grass) GRASS 5.7 WISH - improve string
reading ability in g.parser

The error is still in grass51/gui/tcltkgrass/main/gui.tcl but that is
in now unused code which should be ripped out???

Hamish,

This is an enormous improvement in usability across all of GRASS in one fell
swoop. Thanks much.

If you know where this is in gui.tcl and want to remove it, go ahead (or
perhaps comment it out until it can be tested). Or if you tell me where it
is, I can do it. I assume you mean in gui.tcl for 5.7.

Michael

> The error is still in grass51/gui/tcltkgrass/main/gui.tcl but that
> is in now unused code which should be ripped out???

..

I assume you mean in gui.tcl for 5.7.

yes.
http://freegis.org/cgi-bin/viewcvs.cgi/grass51/gui/tcltkgrass/main/gui.tcl

if you tell me where it is, I can do it.

well the execute procedure to begin with (line 721); probably a lot of
the other procedures in there too. I'm not too familiar with it so I
can't say which ones exactly.

Hamish

> > It may (or may not) also be related to NVIZ not starting from the
> > autogenerated GUI. Currently, you have to start NVIZ from the
> > command line using the -q switch, then add data after it is
> > running. That is, you can't specify initial raster, vector, ect.
> > files in the startup GUI dialog. If you try to start NVIZ this
> > way, it will crash.
>
> still broken:
> can not find channel named "couldn't execute "nviz2.2_script": no
> such file or directory"

Oh. That *is* a Tcl issue.

The immediate error arises from this code in G_gui():

   catch {open $cmd r} msg
  fconfigure $msg -blocking 0
  fileevent $msg readable [ list prnout $msg ]

If the open command succeeds, catch will set msg to the result from
the command (i.e. the channel ID) and return 0. If the command fails,
catch will set msg to the error message and return non-zero.

IOW, msg will only contain a valid channel ID if catch returns zero,
so the above code should probably look something like:

  if {[catch {open $cmd r} msg]} {
    error $msg
  } {
    fconfigure $msg -blocking 0
    fileevent $msg readable [ list prnout $msg ]
  }

Ok, committed. [better error handling, not nviz working from GUI]

[...]

> It was a problem in 5.3, but without the SQL queries it only affects
> the d.text modules AFAIK. Fixed in CVS. Both the 5.3 and 5.7 changes
> make the shell command preview un-cut&paste-able for things with
> spaces in them, but this never worked before either.. some fancy
> formatting before printing the text string could probably move &
> replace the {}s to "s if someone was keen to try.

  set string {}
  foreach word $cmd {
    regsub -all -- {'} $word {'\''} newword
    append string {'} $newword {' }
  }

Modified version committed for 5.7. I'm not sure where this goes in
5.3's gui.tcl, so not done there.

Hamish

On Wed, 4 Aug 2004, Glynn Clements wrote:

I don't remember running into this in 5.3, but the GUI is very different in
5.3; G.PARSER() works but doesn't autogenerate the dialog boxes. They are
each coded individually.

The 5.0/5.3 problems are essentially the same bugs, but in different
code. 5.0/5.3 has tcltkgrass, which is a self-contained Tcl/Tk program
which works by executing GRASS commands.

In 5.7, the problems are in G_gui(), which is called by G_parser(),
which is used by most commands to parse the command line. In 5.0/5.3,
if a command was run without arguments, G_parser() would ask the user
for the infomation via the terminal. In 5.7, it generates and runs a
Tcl/Tk program which displays a dialog, then re-executes the command
using the data from the dialog fields as arguments.

I would just add that this happens unless the environment variable GRASS_UI_TERM is set (to any value). If this is true then the old parser behaviour (terminal-based) happens. Also the 5.7 tcltkgrass doesn't attempt to unset this variable if it is already set so if it is set, then the autogenerated dialog boxes don't appear and the GUI doesn't work. A minor issue.

Paul