wxgui: v.distance to_column option inaccessible

[mlennert]

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.

[martinl]

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:

to_field->guidep = to_column;

Any ideas?

Martin

[martinl]

Replying to 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.

[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..

[mlennert]

Replying to 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().

Moritz

[hamish]

Replying to 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.

Hamish

[martinl]

Replying to martinl:

Replying to 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.

[martinl]

Replying to mlennert:

Replying to 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:

<parameter name="to_layer">
    <guidep>to_column</guidep>
</parameter>

The this information can be parsed by menuform.py.

Martin

[martinl]

Replying to martinl:

Replying to martinl:

Replying to 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.

sprintf(buf, "%s,%s", to_column->key, to_column1->key);
to->field->guidep = G_store(buf);

[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.

Martin

[martinl]

guidep patch

[martinl]

Replying to 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

age,element,prompt

Probably 'element' could discarded?

[martinl]

Replying to martinl:

Replying to 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?

[hamish]

(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.

best, Hamish

[martinl]

Replying to 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).

Martin

[martinl]

Replying to martinl:

[...]

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.

[martinl]

Replying to hamish:

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.

Martin

[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.

[mlennert]

Replying to 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.

Moritz

[hamish]

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.

Hamish