Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#671 closed defect (fixed)

wxgui: v.distance to_column option inaccessible

Reported by: mlennert Owned by: martinl
Priority: major Milestone: 6.4.0
Component: wxGUI Version: 6.4.0 RCs
Keywords: v.distance to_column Cc: grass-dev@…
CPU: Unspecified Platform: Linux

Description

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.

Attachments (1)

guidep.diff (2.6 KB) - added by martinl 10 years ago.
guidep patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by martinl

Cc: grass-dev@… added
Owner: changed from grass-dev@… to martinl
Status: newassigned

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

comment:2 in reply to:  1 ; Changed 10 years ago by 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.

comment:3 Changed 10 years ago by 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..

comment:4 in reply to:  3 ; Changed 10 years ago by 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

comment:5 in reply to:  4 Changed 10 years ago by 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

comment:6 in reply to:  2 ; Changed 10 years ago by 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.

comment:7 in reply to:  4 Changed 10 years ago by 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

comment:8 in reply to:  6 Changed 10 years ago by 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);

comment:9 Changed 10 years ago by 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

Changed 10 years ago by martinl

Attachment: guidep.diff added

guidep patch

comment:10 in reply to:  9 ; Changed 10 years ago by 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?

comment:11 in reply to:  10 Changed 10 years ago by 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?

comment:12 Changed 10 years ago by 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

comment:13 in reply to:  12 ; Changed 10 years ago by 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

comment:14 in reply to:  13 Changed 10 years ago by 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.

comment:15 in reply to:  12 Changed 10 years ago by 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

comment:16 Changed 10 years ago by martinl

Resolution: fixed
Status: assignedclosed

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.

comment:17 in reply to:  16 ; Changed 10 years ago by 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

comment:18 in reply to:  17 Changed 10 years ago by 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

Note: See TracTickets for help on using tickets.