Opened 5 years ago

Closed 5 years ago

#2437 closed enhancement (fixed)

order parameters in g.remove window

Reported by: pvanbosgeo Owned by: grass-dev@…
Priority: normal Milestone: 7.0.1
Component: Default Version: unspecified
Keywords: g.list, g.remove, g.mlist, g.mremove Cc:
CPU: Unspecified Platform: All

Description

On the 'required' tab of the g.remove function you first need to select the data type from a list. Below one need to type the map name search pattern.

I have no clue if that is specific for my computer, but for the latter (the map name search pattern) I have to scroll down as the list of data types is longer then the height of the g.remove screen.

I only ever need to select raster or vector (and I guess those are amongst the most used by the majority of users), so the scrolling to get to the bottom could be avoided by first being presented by the map name search pattern box and than the data type. For me that order makes more sense anyway, but that is just a personal preference.But having rsi problems, anything that could reduce the use of the mouse, however little, would be welcome (I know, I can use the command line more, and I try as much as possible).

Change History (31)

comment:1 Changed 5 years ago by annakrat

Yes, I am not satisfied either, I always overlook it and I go to the Pattern tab and use the exclude instead of the pattern option. So beside the change of order, we should also consider renaming the 'Pattern' tab to 'Exclusion' perhaps? Or move exclude option to Optional? I personally don't use exclusion very often.

Changing the order between type and pattern would mean that you would write:

g.remove "mymap_*" type=rast

instead of

g.remove rast pattern="mymap_*"

unless we do some special trick in the GUI which I would try to avoid. Would this change of the first parameter do any harm?

comment:2 Changed 5 years ago by hcho

I think that the current order makes it easier to implement a list of maps drop down. It's better to restrict map types first and then search for and list related maps in the drop down. Since we moved away from single option per map type, we may need some trick in the GUI.

comment:3 in reply to:  2 ; Changed 5 years ago by annakrat

Replying to hcho:

I think that the current order makes it easier to implement a list of maps drop down. It's better to restrict map types first and then search for and list related maps in the drop down. Since we moved away from single option per map type, we may need some trick in the GUI.

done in r62191, testing is welcome of course.

The order - type, pattern - makes sense. I removed some unused types (#2440) so the pattern is now hopefully visible without scrolling. So we might perhaps close this ticket after testing and backporting.

comment:4 in reply to:  3 ; Changed 5 years ago by neteler

Replying to annakrat:

done in r62191, testing is welcome of course.

There is a small issue now: clicking on "Map name exclusion pattern" in the GUI windows of g.remove now prints the word "element" into the terminal:

GRASS 7.1.svn (nc_spm_08_grass7):~/software/grass71 > g.remove 
element
element

(clicked twice in this test).

comment:5 in reply to:  4 Changed 5 years ago by annakrat

Replying to neteler:

Replying to annakrat:

done in r62191, testing is welcome of course.

There is a small issue now: clicking on "Map name exclusion pattern" in the GUI windows of g.remove now prints the word "element" into the terminal:

Sorry, I forgot a print statement, removed in r62193.

comment:6 Changed 5 years ago by hcho

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

comment:7 in reply to:  6 ; Changed 5 years ago by annakrat

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

If it is not in the interface of the module, gui can't know it should be multiple. As you wrote earlier, multiple pattern is handled by the module, which is the problem.

comment:8 in reply to:  7 ; Changed 5 years ago by annakrat

Replying to annakrat:

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

Hm, it started to output warnings about illegal character *. Not sure what to do about that...

comment:9 in reply to:  8 ; Changed 5 years ago by annakrat

Replying to annakrat:

Replying to annakrat:

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

Hm, it started to output warnings about illegal character *. Not sure what to do about that...

I will probably have to revert this and do test for this specific case in the gui which I wanted to avoid. However, the problem with multiple selection is not just problem of the gui, but also pygrass can't currently handle it simply because g.list interface breaks rules.

comment:10 in reply to:  9 ; Changed 5 years ago by hcho

Replying to annakrat:

Replying to annakrat:

Replying to annakrat:

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

Hm, it started to output warnings about illegal character *. Not sure what to do about that...

I will probably have to revert this and do test for this specific case in the gui which I wanted to avoid. However, the problem with multiple selection is not just problem of the gui, but also pygrass can't currently handle it simply because g.list interface breaks rules.

Hmm... One way would be to change the type of pattern=/exclude= to multiple and use their answer variable, not answers. The help message would be pattern=string[,string,...], which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string. But, it works both in GUI and command line.

comment:11 in reply to:  10 ; Changed 5 years ago by hcho

Replying to hcho:

Replying to annakrat:

Replying to annakrat:

Replying to annakrat:

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

Hm, it started to output warnings about illegal character *. Not sure what to do about that...

I will probably have to revert this and do test for this specific case in the gui which I wanted to avoid. However, the problem with multiple selection is not just problem of the gui, but also pygrass can't currently handle it simply because g.list interface breaks rules.

Hmm... One way would be to change the type of pattern=/exclude= to multiple and use their answer variable, not answers. The help message would be pattern=string[,string,...], which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string. But, it works both in GUI and command line.

If GUI allows for both multiple map selection and manual pattern typing.

comment:12 in reply to:  11 Changed 5 years ago by hcho

Replying to hcho:

Replying to hcho:

Replying to annakrat:

Replying to annakrat:

Replying to annakrat:

Replying to hcho:

I just noticed two issues with the new g.list/remove GUI map selectors. They should not append @ mapset name and should support multiple selection.

I fixed appending the mapset in r62220.

Hm, it started to output warnings about illegal character *. Not sure what to do about that...

I will probably have to revert this and do test for this specific case in the gui which I wanted to avoid. However, the problem with multiple selection is not just problem of the gui, but also pygrass can't currently handle it simply because g.list interface breaks rules.

Hmm... One way would be to change the type of pattern=/exclude= to multiple and use their answer variable, not answers. The help message would be pattern=string[,string,...], which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string. But, it works both in GUI and command line.

If GUI allows for both multiple map selection and manual pattern typing.

Still the @MAPSET problem needs to be fixed in GUI.

comment:13 in reply to:  10 ; Changed 5 years ago by glynn

Replying to hcho:

which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string.

There seems to be two problems here.

One is that the meaning of pattern=/exclude= may change depending upon whether -r/-e are used. I thought that we had already learned that making the semantics of one option change according to the value given for other options is a bad idea.

The other is that there seems to be confusion over the meaning of pattern=/exclude= in the absence of -r/-e. Is it a list of map names, or a glob pattern? It can't be both unless both G_parser() and the GUI recognise "list of map names or string" as a distinct type. "list of map names" won't allow the use of glob patterns because G_parser() will complain about illegal characters and "string" won't work insofar as the GUI won't provide a browser interface for it.

One solution would be to add a separate option (e.g. names=) which accepts multiple valid map names, mark pattern= and names= as mutually exclusive. names= would support multiple options and some form of browsing, pattern= would be a plain string. Similarly for exclusion.

Another solution would be rename g.remove to e.g. g.mremove, remove the pretence about pattern=/exclude= being something other than "arbitrary string", and add a g.remove script which would be a thin wrapper around g.mremove (possibly the only difference would be to re-declare the types of the pattern=/exclude= options).

comment:14 in reply to:  13 ; Changed 5 years ago by wenzeslaus

Keywords: g.list g.mlist g.mremove added

Replying to glynn:

Replying to hcho:

which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string.

There seems to be two problems here.

One is that the meaning of pattern=/exclude= may change depending upon whether -r/-e are used. I thought that we had already learned that making the semantics of one option change according to the value given for other options is a bad idea.

The other is that there seems to be confusion over the meaning of pattern=/exclude= in the absence of -r/-e. Is it a list of map names, or a glob pattern? It can't be both unless both G_parser() and the GUI recognise "list of map names or string" as a distinct type. "list of map names" won't allow the use of glob patterns because G_parser() will complain about illegal characters and "string" won't work insofar as the GUI won't provide a browser interface for it.

I generally agree, I perhaps mentioned this several times. For the future, we need some higher level types. Some types are in "standard options" but they are not exposed. Another example of higher level types would be binary file and text file (as opposed to any file), GUI is now providing direct/interactive input box for binary files. With "color rules file" we can provide a GUI editor with preview besides the textual input.

However, for this case, I think that the following is better option.

One solution would be to add a separate option (e.g. names=) which accepts multiple valid map names, mark pattern= and names= as mutually exclusive. names= would support multiple options and some form of browsing, pattern= would be a plain string. Similarly for exclusion.

This is quite simple and it also solves the problem I was afraid of: users seeing the option named pattern and not willing to use it because they want to input a map name not some pattern they don't understand. Option named names fits well with this use case. maps would be more consistent but it would not be precise and names goes well with pattern.

Another solution would be rename g.remove to e.g. g.mremove, remove the pretence about pattern=/exclude= being something other than "arbitrary string", and add a g.remove script which would be a thin wrapper around g.mremove (possibly the only difference would be to re-declare the types of the pattern=/exclude= options).

This might go against the reasons for doing this whole thing, reducing the complexity of interface and maintenance cost by reducing the number of modules:

Or were there some other reasons?

comment:15 in reply to:  14 Changed 5 years ago by hcho

Replying to wenzeslaus:

Replying to glynn:

Replying to hcho:

which can be misleading because it doesn't take multiple pattern strings, but it only takes multiple map names and a single pattern string.

There seems to be two problems here.

One is that the meaning of pattern=/exclude= may change depending upon whether -r/-e are used. I thought that we had already learned that making the semantics of one option change according to the value given for other options is a bad idea.

The other is that there seems to be confusion over the meaning of pattern=/exclude= in the absence of -r/-e. Is it a list of map names, or a glob pattern? It can't be both unless both G_parser() and the GUI recognise "list of map names or string" as a distinct type. "list of map names" won't allow the use of glob patterns because G_parser() will complain about illegal characters and "string" won't work insofar as the GUI won't provide a browser interface for it.

I generally agree, I perhaps mentioned this several times. For the future, we need some higher level types. Some types are in "standard options" but they are not exposed. Another example of higher level types would be binary file and text file (as opposed to any file), GUI is now providing direct/interactive input box for binary files. With "color rules file" we can provide a GUI editor with preview besides the textual input.

However, for this case, I think that the following is better option.

One solution would be to add a separate option (e.g. names=) which accepts multiple valid map names, mark pattern= and names= as mutually exclusive. names= would support multiple options and some form of browsing, pattern= would be a plain string. Similarly for exclusion.

This is quite simple and it also solves the problem I was afraid of: users seeing the option named pattern and not willing to use it because they want to input a map name not some pattern they don't understand. Option named names fits well with this use case. maps would be more consistent but it would not be precise and names goes well with pattern.

I agree with you that this solution is better than the other because the main reason of replacing g.list/remove was to reduce redundancy. These two additional options would act as a wrapper around the pattern options. names= and exclude_names= should solve this issue in GUI. In command line, it's up to the user which one to use for a list of map names because the pattern options can handle a list of map names as well.

Another solution would be rename g.remove to e.g. g.mremove, remove the pretence about pattern=/exclude= being something other than "arbitrary string", and add a g.remove script which would be a thin wrapper around g.mremove (possibly the only difference would be to re-declare the types of the pattern=/exclude= options).

This might go against the reasons for doing this whole thing, reducing the complexity of interface and maintenance cost by reducing the number of modules:

Or were there some other reasons?

comment:16 Changed 5 years ago by hcho

I just added names= and ignores= in r62244. These options take a list of filenames and convert it to a glob pattern.

comment:17 Changed 5 years ago by annakrat

I noticed that this:

g.remove type=rast names=test@user1

gives error:

WARNING: Illegal filename <test@user1>. Character <@> not allowed.
ERROR: Illegal filenames not allowed in the names option.

In the previous version of g.remove, full name was working. I like the previous behavior more but of course the mapset is not really needed.

comment:18 in reply to:  17 ; Changed 5 years ago by glynn

Replying to annakrat:

In the previous version of g.remove, full name was working. I like the previous behavior more but of course the mapset is not really needed.

Even so, a fully-qualified name should be accepted provided that the mapset is the current mapset. Code which executes GRASS modules should be able to use fully-qualified names throughout.

comment:19 in reply to:  18 ; Changed 5 years ago by hcho

Replying to glynn:

Replying to annakrat:

In the previous version of g.remove, full name was working. I like the previous behavior more but of course the mapset is not really needed.

Even so, a fully-qualified name should be accepted provided that the mapset is the current mapset. Code which executes GRASS modules should be able to use fully-qualified names throughout.

This behavior is now fixed in r62303.

comment:20 in reply to:  19 ; Changed 5 years ago by wenzeslaus

Replying to hcho:

Replying to glynn:

Replying to annakrat:

In the previous version of g.remove, full name was working. I like the previous behavior more but of course the mapset is not really needed.

Even so, a fully-qualified name should be accepted provided that the mapset is the current mapset. Code which executes GRASS modules should be able to use fully-qualified names throughout.

This behavior is now fixed in r62303.

About "use fully-qualified names throughout" I though that it does not apply to the output (see also mailing list, nabble link). When removing a map, it is of course questionable if we are executing our write access (as in case of output) or if we are using existing map (which we can specify with mapset).

comment:21 in reply to:  19 Changed 5 years ago by annakrat

Replying to hcho:

This behavior is now fixed in r62303.

Thanks. We are getting close to finishing g.remove/g.list interface. Does anyone has any opinion on the GUI layout? Related discussion: http://lists.osgeo.org/pipermail/grass-dev/2014-October/071179.html

The problem of the current layout is that when you want to do the basic operation (removing one raster/vector map) you have go into 3 tabs (Required, Names, Optional).

comment:22 in reply to:  20 Changed 5 years ago by hcho

Replying to wenzeslaus:

Replying to hcho:

Replying to glynn:

Replying to annakrat:

In the previous version of g.remove, full name was working. I like the previous behavior more but of course the mapset is not really needed.

Even so, a fully-qualified name should be accepted provided that the mapset is the current mapset. Code which executes GRASS modules should be able to use fully-qualified names throughout.

This behavior is now fixed in r62303.

About "use fully-qualified names throughout" I though that it does not apply to the output (see also mailing list, nabble link). When removing a map, it is of course questionable if we are executing our write access (as in case of output) or if we are using existing map (which we can specify with mapset).

I would say in this case, we're dealing with "input" or "existing" maps and g.remove should allow @MAPSET. IMO, names without @MAPSET only apply to "new" maps.

comment:23 in reply to:  20 Changed 5 years ago by glynn

Replying to wenzeslaus:

About "use fully-qualified names throughout" I though that it does not apply to the output

No.

You're only supposed to be able to create, remove, replace or modify maps in the current mapset. But that doesn't mean that you can't use a fully-qualified name, only that the @mapset part has to be the current mapset.

Requiring the use of unqualified names in such cases imposes a burden on scripts, as they need to know that a map name refers to an output. When map names are obtained from the output of other modules, they may be provided as fully-qualified names, requiring the script to strip the mapset.

Also, an unqualified name doesn't necessarily refer to a map in the current mapset. It refers to the first such map found in the mapset search path. At present, the current mapset is forcibly added to the head of the search path. This is a workaround for bugs in numerous modules and scripts which assume that this will always be the case, but it shouldn't be relied upon.

comment:24 Changed 5 years ago by annakrat

I changed the layout in r62405. I moved options names, type and flag -f into one guisection Basic. Some feedback is welcome.

Also, do we really need option ignore? Isn't exclude option enough?

Another suggestion is to use -f only for pattern option. When using names, it wouldn't require force flag for the actual removing of the maps. Any opinion on that?

comment:25 in reply to:  24 ; Changed 5 years ago by hcho

Replying to annakrat:

I changed the layout in r62405. I moved options names, type and flag -f into one guisection Basic. Some feedback is welcome.

Also, do we really need option ignore? Isn't exclude option enough?

Just like pattern= and names=, exclude= takes a glob or regex pattern while ignore= takes a list of names. For example, exclude=a? and ignore=a? are different. ignore=a? is expanded to a glob pattern a\?. Also, in GUI, multiple map selection cannot be supported for exclude= (a single string option), which was the main reason why names= and ignore= were added initially. I think both names= and ignore= are useful in GUI.

Another suggestion is to use -f only for pattern option. When using names, it wouldn't require force flag for the actual removing of the maps. Any opinion on that?

IMO, that's not a good idea because of inconsistent behaviors. If we want any confirmation when using patterns, the force flag should be used if "any" of pattern= or exclude= is used.

# Patterns used => use -f flag
g.remove -f rast pattern=... exclude=...
g.remove -f rast pattern=... ignore=...
g.remove -f rast names=... exclude=...

# No patterns used => no -f flag; this is inconsistent and can be confusing when using patterns.
g.remove rast names=... ignore=...

If it's annoying to run g.remove GUI twice, what about this? In a single GUI run, show output and ask the user if they want to delete them.

comment:26 in reply to:  25 ; Changed 5 years ago by annakrat

Replying to hcho:

Another suggestion is to use -f only for pattern option. When using names, it wouldn't require force flag for the actual removing of the maps. Any opinion on that?

IMO, that's not a good idea because of inconsistent behaviors. If we want any confirmation when using patterns, the force flag should be used if "any" of pattern= or exclude= is used.

# Patterns used => use -f flag
g.remove -f rast pattern=... exclude=...
g.remove -f rast pattern=... ignore=...
g.remove -f rast names=... exclude=...

# No patterns used => no -f flag; this is inconsistent and can be confusing when using patterns.
g.remove rast names=... ignore=...

I mostly agree, although inconsistent doesn't necessarily mean not intuitive.

If it's annoying to run g.remove GUI twice, what about this? In a single GUI run, show output and ask the user if they want to delete them.

I am generally against implementing any special things for one module (which has autogenerated dialog).

comment:27 in reply to:  26 ; Changed 5 years ago by mlennert

Replying to annakrat:

I am generally against implementing any special things for one module (which has autogenerated dialog).

+1

Moritz

comment:28 in reply to:  27 ; Changed 5 years ago by hcho

Replying to mlennert:

Replying to annakrat:

I am generally against implementing any special things for one module (which has autogenerated dialog).

+1

Moritz

Yeah.. +1 too. What about just getting rid of that flag completely and using g.list instead? While updating python scripts, I found it's just too much typing to add flags="f" every time and it doesn't make much sense in scripting anyway. Since the API of g.remove is compatible with g.list API as far as search patterns are concerned, it's a matter of changing module names. Is it too dangerous?

comment:29 in reply to:  28 ; Changed 5 years ago by annakrat

Replying to hcho:

Replying to mlennert:

Replying to annakrat:

I am generally against implementing any special things for one module (which has autogenerated dialog).

+1

Moritz

Yeah.. +1 too. What about just getting rid of that flag completely and using g.list instead? While updating python scripts, I found it's just too much typing to add flags="f" every time and it doesn't make much sense in scripting anyway. Since the API of g.remove is compatible with g.list API as far as search patterns are concerned, it's a matter of changing module names. Is it too dangerous?

I am afraid it is. Especially when users are used to it from the previous interface. Using -f flag only when using pattern/exclude would make more sense if want to avoid using it every time, but I understand why you don't like it. Some more input from others would be appreciated...

comment:30 in reply to:  29 Changed 5 years ago by mlennert

Replying to annakrat:

Replying to hcho:

Replying to mlennert:

Replying to annakrat:

I am generally against implementing any special things for one module (which has autogenerated dialog).

+1

Moritz

Yeah.. +1 too. What about just getting rid of that flag completely and using g.list instead? While updating python scripts, I found it's just too much typing to add flags="f" every time and it doesn't make much sense in scripting anyway. Since the API of g.remove is compatible with g.list API as far as search patterns are concerned, it's a matter of changing module names. Is it too dangerous?

I am afraid it is. Especially when users are used to it from the previous interface. Using -f flag only when using pattern/exclude would make more sense if want to avoid using it every time, but I understand why you don't like it. Some more input from others would be appreciated...

I don't mind having the -f flag as extra security, even though it does add typing an extra 3 characters to the former use of g.remove.

+1 for keeping the -f for all types of map selection.

Moritz

comment:31 Changed 5 years ago by wenzeslaus

Milestone: 7.1.07.0.1
Resolution: fixed
Status: newclosed

Original issue is solved and the interface changed for 7.0.0.

Closing. (Trac forces milestone 7.0.1 but this should have been closed for 7.0.0.)

Open a new ticket if you have further concerns.

Note: See TracTickets for help on using tickets.