#671: wxgui: v.distance to_column option inaccessible
----------------------------------+-----------------------------------------
Reporter: mlennert | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: major | Milestone: 6.4.0
Component: wxGUI | Version: 6.4.0 RCs
Keywords: v.distance to_column | Platform: Linux
Cpu: Unspecified |
----------------------------------+-----------------------------------------
In GRASS6.4 RC5, self-compiled deb packages for Ubuntu Intrepid, I cannot
access the to_column field in the GUI window. The dropdown list button is
desactivated and neither can I enter a column name by hand.
The reason is that the module contains more {{{G_OPT_V_FIELD}}} parameters
- in this case 'from_layer' and 'to_layer'. There is no way how to
determine what layer is connected with 'to_column' paramater. Generally
speaking all modules which have more 'layer/table/database/...' parameters
are affected. The current approach is simple: e.g. update all
{{{G_OPT_DB_COLUMN}}} parameters when {{{G_OPT_V_FIELD}}} parameter value
is changed.
I am not sure how to solve this problem. One approach could be to add new
attribute to the {{{Option}}} structure to track special dependencies. In
the case of v.select:
Replying to [comment:1 martinl]:
> I am not sure how to solve this problem. One approach could be to add
new attribute to the {{{Option}}} structure to track special dependencies.
In the case of v.select:
>
> {{{
> to_field->guidep = to_column;
> }}}
Better to say, {{{guidep}}} should be an array of pointers to the other
{{{Option}}} structures.
I don't doubt the usefulness, but it seems slightly incongruous that the
input parameters should be allowed to parse for available columns before
G_parser() has had a change to run..
Replying to [comment:3 hamish]:
> I don't doubt the usefulness, but it seems slightly incongruous that the
input parameters should be allowed to parse for available columns before
G_parser() has had a change to run..
AFAIU, it's the same logic as the supported_formats() function in
r.out.gdal, also run before the call to G_parser().
Replying to [comment:4 mlennert]:
> AFAIU, it's the same logic as the supported_formats() function
> in r.out.gdal, also run before the call to G_parser().
and the same criticism applies.
actually r.out.gdal is much worse -- Martin can do magic in the module-
launch GUI plucking off the map name and doing outside processing that map
separate to the running of the actual module. So maybe his suggestion is
not so bad after all.
still not sure, seems like a potential method to introduce circular
dependency problems.
Replying to [comment:2 martinl]:
> Replying to [comment:1 martinl]:
> > I am not sure how to solve this problem. One approach could be to add
new attribute to the {{{Option}}} structure to track special dependencies.
In the case of v.select:
> >
> > {{{
> > to_field->guidep = to_column;
> > }}}
>
> Better to say, {{{guidep}}} should be an array of pointers to the other
{{{Option}}} structures.
Of course nonsense, instead of pointer to {{Option}} should be pointer to
as string containing option key.
Replying to [comment:4 mlennert]:
> Replying to [comment:3 hamish]:
> > I don't doubt the usefulness, but it seems slightly incongruous that
the input parameters should be allowed to parse for available columns
before G_parser() has had a change to run..
>
> AFAIU, it's the same logic as the supported_formats() function in
r.out.gdal, also run before the call to G_parser().
I don't think so, it's not the same issue. In this case e.g. for XML
output the parser returns:
Replying to [comment:6 martinl]:
> Replying to [comment:2 martinl]:
> > Replying to [comment:1 martinl]:
> > > I am not sure how to solve this problem. One approach could be to
add new attribute to the {{{Option}}} structure to track special
dependencies. In the case of v.select:
> > > {{{
> > > to_field->guidep = to_column;
> > > }}}
To make things more simple, let's switch from array to a string, where
option keys are separated by comma, e.g.
The patch for trunk attached. If {{{guidependency}}} is not defined, the
current approach would be applied, so e.g. for layer/columns - if 'layer'
value is changed, all G_OPT_DB_COLUMNS will be updated. In the case that
'layer' option has defined {{{guidependency}}} attribute, only mentioned
options will be updated.
Replying to [comment:9 martinl]:
> The patch for trunk attached. If {{{guidependency}}} is not defined, the
current approach would be applied, so e.g. for layer/columns - if 'layer'
value is changed, all G_OPT_DB_COLUMNS will be updated. In the case that
'layer' option has defined {{{guidependency}}} attribute, only mentioned
options will be updated.
Any objections to commit the changes to trunk? BTW, we could simplify
{{{gisprompt}}} attribute. Currently
Replying to [comment:10 martinl]:
> Replying to [comment:9 martinl]:
> Any objections to commit the changes to trunk? BTW, we could simplify
{{{gisprompt}}} attribute.
Done in r38339. I would suggest to backport it to devbr6. I am not sure
about relbr64. It's quite significant change. If not backported, how to
"fix" the bug for 6.4.0. Allowing write mode for column's comboboxes if
module has more {{{G_OPT_V_FIELD}}} options?
(wating more than 1 day for comments before deep changes would be nice..)
> I would suggest to backport it to devbr6.
> I am not sure about relbr64.
the whole point about a stable branch is that we don't go making deep
changes in it, we just fix bugs in the least invasive way possible. So
please, no. otherwise we have to start the stability cycle all over again.
this is deep enough that from my POV that goes for devbr6 too.
trunk is the place for this, if it is to happen at all.
> If not backported, how to "fix" the bug for 6.4.0.
let the user type the column name themselves in a text box. it's more
work, but so be it.
Replying to [comment:12 hamish]:
> (wating more than 1 day for comments before deep changes would be
nice..)
You are right, I was too quick.
> > I would suggest to backport it to devbr6.
> > I am not sure about relbr64.
>
> the whole point about a stable branch is that we don't go making deep
changes in it, we just fix bugs in the least invasive way possible. So
please, no. otherwise we have to start the stability cycle all over again.
>
> this is deep enough that from my POV that goes for devbr6 too.
> trunk is the place for this, if it is to happen at all.
OK, I made column select writable in devbr6 (r38350) and relbr64 (r38351).
> OK, I made column select writable in devbr6 (r38350) and relbr64
(r38351).
Anyway, there are some remaining issues, e.g. 'to_layer' in v.distance is
not writable (wx.CHOICE), etc. Fixed in trunk, but not in the other
branches.
> the whole point about a stable branch is that we don't go making deep
changes in it, we just fix bugs in the least invasive way possible. So
please, no. otherwise we have to start the stability cycle all over again.
Right.
> this is deep enough that from my POV that goes for devbr6 too.
OK, I just think that it's quite important for wxGUI dialogs (not only
v.distance). It's main reason why I was suggesting to backport it to
devbr6.
Replying to [comment:16 martinl]:
> Closing this ticket as fixed in trunk and "partly fixed" in
devbr6/relbr64.
>
> Martin
>
> PS: Still not sure if to backport the bugfix from trunk to devbr6.
It seems to work well in grass7, so I would plead for backporting it.
However, there is one issue: would it be possible to have a select list
that is also writable ? The problem arises with modules such as
d.vect.thematic, where you have to select a column on which you wish to
base your thematic map, but you can also use an expression, i.e.
col1/col2. In the current form of the column selector, it is impossible to
write such expressions in the GUI.
> It seems to work well in grass7, so I would plead for backporting it.
How well it works or how useful it is isn't really the issue. I am not
really convinced that we should be changing the 'struct Option' definition
in gis.h for 6.5. While we haven't guaranteed ABI/API compatibility in the
past during stable branches, IMO the less of that we do the better.
especially at this late stage of 6.x.