[GRASS-dev] [GRASS GIS] #2509: d.mon output overwrite

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:19 martinl]:

> * direct rendering is a nice feature,

It's a bit more than that, given that it's the only way to actually get
graphics out of GRASS.

> Can we have consensus on these points above?

r63747 requires that either MONITOR or GRASS_RENDER_IMMEDIATE be set.
Additionally, GRASS_RENDER_IMMEDIATE=default can be used to select the
default driver (cairo if available, otherwise PNG).

> If so let's focus on implementation of the second issue and after
releasing 7.0 we can discuss better implementation of monitors rather than
using hacks like $MONITOR. Make sense to you?

Personally, I'd rather see it done before 7.0 so that it doesn't become
something we're stuck with supporting for the next decade. Getting rid of
legacy cruft was supposed to be a key point of 7.0, and now we have new
cruft added before it's even released.

I would rather see something like:

If MONITOR is set, it names a program which will be executed with the
complete argument list of the original program (including `argv[0]`). So
e.g. if MONITOR is set to "wx-monitor", then running "d.rast
elevation.dem" would execute "wx-monitor d.rast elevation.dem" then
terminate (or would execute it using execve(), which has essentially the
same result).

IOW, MONITOR simply causes the display library to delegate everything to
some other program. This could write to a file or communicate with wxGUI
via a socket (or DDE etc) or whatever. This keeps the details (and any
dependencies, which might be platform-specific) out of the display
library.

Future changes or extensions wouldn't require any modifications the
display library, which hopefully guarantees that the important case (i.e.
immediate rendering, which is where everything ends up eventually) won't
get broken by such changes.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:20&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by martinl):

Replying to [comment:20 glynn]:

> r63747 requires that either MONITOR or GRASS_RENDER_IMMEDIATE be set.
Additionally, GRASS_RENDER_IMMEDIATE=default can be used to select the
default driver (cairo if available, otherwise PNG).

I took liberty to do backport including related changes to relbr70 as
r63753

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:21&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by wenzeslaus):

Hi Glynn and Martin, one of the biggest problems of the old state was that
it was not documented, so it was not clear what are the possibilities and
common usage patterns. Can you please documented as much as you can now?
For example, it would be good to document how to effectively use the
system from Python scripts and from external applications such as QGIS
(which may not posses features such as `d.rast.num`). I think the proper
place would be `displayintro.html` file.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:22&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by martinl):

Replying to [comment:20 glynn]:

> If MONITOR is set, it names a program which will be executed with the
complete argument list of the original program (including `argv[0]`). So
e.g. if MONITOR is set to "wx-monitor", then running "d.rast
elevation.dem" would execute "wx-monitor d.rast elevation.dem" then
terminate (or would execute it using execve(), which has essentially the
same result).

I have implemented that in r64401 and document it in r64403. BTW, now we
could remove D_save_command() from trunk which is after this modifications
empty, any objections? Currently it's working only for file-based monitors
(cairo, png), implementation for wxGUI monitors is in progress.

I have update `d.mon` which writes currently only one variable, ie.
`MONITOR`. Support files including python renderer are stored in .tmp dir.
Also `d.erase` and `d.redraw` have been updated to the new architecture.

Testing welcomed.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:23&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:23 martinl]:

> > If MONITOR is set, it names a program which will be executed with the
complete argument list of the original program (including `argv[0]`). So
e.g. if MONITOR is set to "wx-monitor", then running "d.rast
elevation.dem" would execute "wx-monitor d.rast elevation.dem" then
terminate (or would execute it using execve(), which has essentially the
same result).
>
> I have implemented that in r64401

Not really. The main issues are:

1. It adds yet more cruft rather than simplifying. Hopefully this is just
a transitional phase until existing modules are updated.

2. The arguments are being concatenated into a string (without correct
quoting) then passed as a single argument. The output from G_recreate()
command may be sufficient for a human-readable description of the command,
but it's not suitable for execution. G_parser() needs to save argv so that
it can be retrieved, and D_open_driver() needs to pass the actual argv
using G_vspawn_ex().

3. The command is required to be a Python script. It should at least
support compiled executables.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:24&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by martinl):

Replying to [comment:24 glynn]:

> > I have implemented that in r64401
>
> Not really. The main issues are:

well, it was meant as an initial step, feel free to improve.

> 1. It adds yet more cruft rather than simplifying. Hopefully this is
just a transitional phase until existing modules are updated.

Can you be more detailed?

> 2. The arguments are being concatenated into a string (without correct
quoting) then passed as a single argument. The output from G_recreate()
command may be sufficient for a human-readable description of the command,
but it's not suitable for execution. G_parser() needs to save argv so that
it can be retrieved, and D_open_driver() needs to pass the actual argv
using G_vspawn_ex().
>
> 3. The command is required to be a Python script. It should at least
support compiled executables.

Right.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:25&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by wenzeslaus):

Replying to [comment:24 glynn]:
> 2. The arguments are being concatenated into a string (without correct
quoting) then passed as a single argument. The output from G_recreate()
command may be sufficient for a human-readable description of the command,
but it's not suitable for execution. G_parser() needs to save argv so that
it can be retrieved, and D_open_driver() needs to pass the actual argv
using G_vspawn_ex().

Passing the arguments as actual individual arguments is what is used, e.g.
in Python:

{{{
python file.py arg1 arg2 arg3
python -c file.py arg1 arg2 arg3
python -m module arg1 arg2 arg3
}}}

Passing commands as one string is probably necessary in some cases (e.g.
when they are stored into a file or when a module would take more
different commands as input) but if it can be avoided, it should be
avoided I think.

Anyway, Martin, it seems that this will be quite an improvement if you are
heading where I think you are heading.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:26&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:25 martinl]:

> > 1. It adds yet more cruft rather than simplifying. Hopefully this is
just a transitional phase until existing modules are updated.
>
> Can you be more detailed?

Rather than redefining MONITOR (as suggested in comment:20), it keeps that
and adds yet another variable, GRASS_RENDER_COMMAND.

The only thing that's actually required is a mechanism to allow all d.*
commands to be "redirected" to another program. And that's all that should
be provided by the display library. Everything else should be handled by
that other program.

So if you want to retain the existing semantics for MONITOR, all
references to it should be removed from the display library. Instead,
wxGUI would set GRASS_RENDER_COMMAND to refer to a program or script which
implements the MONITOR behaviour. Any future changes to the wxGUI monitor
mechanism would only affect wxGUI and its utility programs, not the
display systems.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:27&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by martinl):

Replying to [comment:27 glynn]:

> So if you want to retain the existing semantics for MONITOR, all
references to it should be removed from the display library. Instead,
wxGUI would set GRASS_RENDER_COMMAND to refer to a program or script which
implements the MONITOR behaviour. Any future changes to the wxGUI monitor
mechanism would only affect wxGUI and its utility programs, not the
display systems.

it's not only about wxGUI. The reason for having (GRASS) variable MONITOR
is the module `d.mon` which allows to manage "file-based monitors" (like
cairo output) and also standalone wxGUI monitors. This feature is
requested by many of GRASS users, there is strong interest to have `d.mon`
in GRASS 7.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:28&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:26 wenzeslaus]:

> Passing commands as one string is probably necessary in some cases (e.g.
when they are stored into a file or when a module would take more
different commands as input) but if it can be avoided, it should be
avoided I think.

When a d.* command is redirected to another program, its argument list
needs to be passed as-is.

If you need to store a "command" in a file, then you need to separate the
arguments. There are two options here:

  1. Use a NUL byte as the argument separator. On both Unix and Windows,
any byte except NUL can occur within an argument. NUL cannot occur within
an argument, but can occur within a file. However, if the command is only
one item within the file, then you also need some mechanism to indicate
the end of the command, which leaves the second option:

  2. Use some other byte as the separator and escape or quote argument
values so that occurrences of the separator within an argument can be
distinguished from argument separators.

Ignoring the situation where the separator (or a character used for
escaping or quoting) occurs within an argument is not a "solution". Which
is why G_recreate_command() can't be used here.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:29&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:28 martinl]:

> > So if you want to retain the existing semantics for MONITOR, all
references to it should be removed from the display library. Instead,
wxGUI would set GRASS_RENDER_COMMAND to refer to a program or script which
implements the MONITOR behaviour. Any future changes to the wxGUI monitor
mechanism would only affect wxGUI and its utility programs, not the
display systems.
>
> it's not only about wxGUI. The reason for having (GRASS) variable
MONITOR is the module `d.mon` which allows to manage "file-based monitors"
(like cairo output) and also standalone wxGUI monitors. This feature is
requested by many of GRASS users, there is strong interest to have `d.mon`
in GRASS 7.

Now that you mentioned that, I decided to take a look at d.mon.

I must have missed the discussion about the implementation of this. Can
you tell me where to find it?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:30&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by mlennert):

Replying to [comment:30 glynn]:
> Replying to [comment:28 martinl]:
>
> > > So if you want to retain the existing semantics for MONITOR, all
references to it should be removed from the display library. Instead,
wxGUI would set GRASS_RENDER_COMMAND to refer to a program or script which
implements the MONITOR behaviour. Any future changes to the wxGUI monitor
mechanism would only affect wxGUI and its utility programs, not the
display systems.
> >
> > it's not only about wxGUI. The reason for having (GRASS) variable
MONITOR is the module `d.mon` which allows to manage "file-based monitors"
(like cairo output) and also standalone wxGUI monitors. This feature is
requested by many of GRASS users, there is strong interest to have `d.mon`
in GRASS 7.
>
> Now that you mentioned that, I decided to take a look at d.mon.
>
> I must have missed the discussion about the implementation of this. Can
you tell me where to find it?

display/d.mon ?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:31&gt;
GRASS GIS <http://grass.osgeo.org>

#2509: d.mon output overwrite
-------------------------+--------------------------------------------------
Reporter: martinl | Owner: martinl
     Type: defect | Status: assigned
Priority: normal | Milestone: 7.0.0
Component: Display | Version: unspecified
Keywords: d.mon | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------

Comment(by glynn):

Replying to [comment:31 mlennert]:

> display/d.mon ?

I was hoping for something which explained the general idea behind what's
going on there.

At first glance, this all looks like something of a hack to avoid more
substantial changes to the display system, but I might be overlooking
something.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2509#comment:32&gt;
GRASS GIS <http://grass.osgeo.org>