Opened 14 years ago

Closed 12 years ago

#1051 closed defect (fixed)

wxgui: SEARCH_PATH corruption

Reported by: msieczka Owned by: hamish
Priority: minor Milestone: 6.4.1
Component: wxGUI Version: svn-releasebranch64
Keywords: mapsets Cc: grass-dev@…
CPU: All Platform: All

Description

  1. In wxgui wizard, create a location with 2 mapsets inside (mapset1, mapset2).
  1. Enter mapset2.
  1. Settings > GRASS working environment > Mapset access: mark mapset1.

The mapset search path in mapset2 is wrong:

$ g.gisenv get=MAPSET
mapset2

g.mapsets -p
mapset1 PERMANENT mapset2

If maps of the same names exist in mapset1 and mapset2, GRASS will pick the one from mapset1 although the current mapset is mapset2. This might lead to various troubles.

The bug is propably in gui/wxpython/gui_modules/preferences.py.

g.mapsets.tcl (g.mapsets -s) doesn't have this bug.

Attachments (2)

utils.py.diff (2.9 KB ) - added by msieczka 14 years ago.
preferences.py.diff (2.9 KB ) - added by msieczka 14 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by msieczka, 14 years ago

Priority: majorcritical

in reply to:  description comment:2 by glynn, 14 years ago

Replying to msieczka:

The mapset search path in mapset2 is wrong:

$ g.gisenv get=MAPSET
mapset2

g.mapsets -p
mapset1 PERMANENT mapset2

If maps of the same names exist in mapset1 and mapset2, GRASS will pick the one from mapset1 although the current mapset is mapset2. This might lead to various troubles.

This isn't necessarily a problem. GRASS itself doesn't care whether the current mapset is first in the search path, or even if it's in the search path at all.

OTOH, it's probably a good idea for the GUI interface to place additional mapsets after the current mapset, at least by default.

However: I'd like to take this opportunity to remind programmers that an unqualified input map name does NOT necessarily refer to the map of that name in the current mapset, but to the first map with that name found in the mapset search path (which may or may not have the current mapset first).

If you intend to refer specifically to a map in the current mapset, you need to specify the mapset.

by msieczka, 14 years ago

Attachment: utils.py.diff added

by msieczka, 14 years ago

Attachment: preferences.py.diff added

comment:3 by msieczka, 14 years ago

Replying to glynn:

Replying to msieczka:

If maps of the same names exist in mapset1 and mapset2, GRASS will pick the one from mapset1 although the current mapset is mapset2. This might lead to various troubles.

This isn't necessarily a problem. GRASS itself doesn't care whether the current mapset is first in the search path, or even if it's in the search path at all.

According to my tests, the SEARCH_PATH file, if present, overrides the default "current mapset 1st, PERMANENT 2nd, other mapsets next" behaviour, up to even excluding any mapset from the search path. Mapsets' order in the SEARCH_PATH file determines the search order too. Do the steps 1-3 in #comment:description. Now:

  1. Create a vector map: v.random out=point n=1.
  1. Start a new session in mapset1.
  1. Create a vector map of the same name as in mapset2, i.e. do "v.random out=point n=1".
  1. Back in mapset2 troubles begin:

a) v.info picks the map from mapset1 although we are in mapset2:

$ v.info -h point
WARNING: 'vector/point' was found in more mapsets (also found in <mapset2>)
WARNING: Using <point@mapset1>
WARNING: 'vector/point' was found in more mapsets (also found in <mapset2>)
WARNING: Using <point@mapset1>
COMMAND: v.random output="point" n=1 zmin=0.0 zmax=0.0
GISDBASE: /home/grassdata
LOCATION: test_location MAPSET: mapset1 USER: pok DATE: Sun May 16 11:15:28 2010

b) same does g.region:

$ g.region vect=point
WARNING: 'vector/point' was found in more mapsets (also found in <mapset2>)
WARNING: Using <point@mapset1>

c) same does v.to.rast:

$ v.to.rast use=cat in=point out=point_rast
WARNING: 'vector/point' was found in more mapsets (also found in <mapset2>)
WARNING: Using <point@mapset1>
Loading data...
Reading features...
 100%
Writing raster map...
 100%
Converted areas: 0 of 0
Converted points/lines: 1 of 1
v.to.rast complete

Any module I tried prefers the "point" vector map from the first mapset in the SEARCH_PATH (mapset1) rather the one in the current mapset (mapset2). I understand what Glynn says in his furher comments in #comment:2, fully second that, but getting back to the original subject: wxGUI's `Settings > GRASS working environment > Mapset access' needs a fix.

I have attached a patch against SVN develbranch6, r42262. Please anybody let me know if it's acceptable. In general, it's purpose is to make 'Mapset access' GUI work same as 'g.mapsets addmapset=/removemapset=', which:

  1. Doesn't allow the user to remove the current mapset from the SEARCH_PATH.
  1. The current mapset is always first in the SEARCH_PATH.
  1. Treats PERMANENT as any other mapset AFAICT. PERMANENT is the second in the search path only if the SEARCH_PATH file is not present.
  1. Doesn't sort mapset names in the SEARCH_PATH file. Thanks to this, the search order depends on g.mapsets add/remove commands sequence, thus can be controlled from GRASS itself.

in reply to:  3 comment:4 by msieczka, 14 years ago

Replying to msieczka:

I understand what Glynn says in his furher comments in #comment:2,

Reading this again I can see I didn't fully understand Glynn's remarks and to big extent I just repeated after him in my recent comment, but anyway the proposed patches are valid IMHO.

in reply to:  3 ; comment:5 by glynn, 14 years ago

Replying to msieczka:

According to my tests, the SEARCH_PATH file, if present, overrides the default "current mapset 1st, PERMANENT 2nd, other mapsets next" behaviour, up to even excluding any mapset from the search path. Mapsets' order in the SEARCH_PATH file determines the search order too.

Yep. If SEARCH_PATH doesn't exist, the default is the current mapset then PERMANENT. If SEARCH_PATH exists, it entirely specifies the search path; no other mapsets are searched, including the current and PERMANENT mapsets.

g.mapsets ensures that the current mapset appears somewhere in the list (if it isn't, it will add it at the beginning), but libgis doesn't care.

I have attached a patch against SVN develbranch6, r42262. Please anybody let me know if it's acceptable. In general, it's purpose is to make 'Mapset access' GUI work same as 'g.mapsets addmapset=/removemapset=', which:

  1. Doesn't allow the user to remove the current mapset from the SEARCH_PATH.
  1. The current mapset is always first in the SEARCH_PATH.

g.mapsets allows other mapsets to be placed ahead of the current mapset. If the current mapset is entirely missing, it will add it to the head of the search path, but it won't move it if it's already present.

  1. Treats PERMANENT as any other mapset AFAICT. PERMANENT is the second in the search path only if the SEARCH_PATH file is not present.

This is implemented in libgis; specifically by G_get_list_of_mapsets() in lib/gis/mapset_nme.c .

The search path shouldn't really be relevant for the GUI, which should always be using qualified map names.

However, to follow up on my previous comment:

If you intend to refer specifically to a map in the current mapset, you need to specify the mapset.

I've probably forgotten this enough times. In fact, I'd guess that almost (?) every script which creates temporary maps subsequently refers to them using an unqualified name.

In retrospect, I'd conclude that having anything ahead of the current mapset in the search path is asking for trouble. So I'm inclined suggest that G_get_list_of_mapsets() should be modified to forcibly place the current mapset at the head of the search path, even if SEARCH_PATH says otherwise.

in reply to:  5 comment:6 by hamish, 14 years ago

Keywords: mapsets added

Replying to glynn:

The search path shouldn't really be relevant for the GUI, which should always be using qualified map names.

as a complete aside, for a while I've had the wish to clean that up a bit in the wx GUI as I find it rather cluttering and hard to read.

perhaps: roads @ PERMANENT or roads (PERMANENT)

both in the map layer-list and in the list of maps selector. or maybe just omit it in the map-selector pull-down list as it is already given by the higher level mapset name in the mapset tree (blue text).

... and pack the (opacity: 100%) [:] stuff on the right edge of the row.

thoughts?

Hamish

comment:7 by msieczka, 14 years ago

Fixed in develbranch6 r42307 and trunk r42308. I hope to backport it to releasebranch_6_4 if it's OK.

comment:8 by neteler, 14 years ago

Please backport if possible.

comment:9 by aghisla, 14 years ago

Resolution: fixed
Status: newclosed

Backported to releasebranch_6_4, r42681.

+1 for Hamish's suggestion.

in reply to:  9 comment:10 by msieczka, 14 years ago

Replying to aghisla:

Backported to releasebranch_6_4, r42681.

Thanks, Anne!

comment:11 by hamish, 14 years ago

Resolution: fixed
Status: closedreopened

some comments re making PERMANENT mapset always available to follow. reopening bug, assigning to me (todo once I clear some time)

comment:12 by hamish, 14 years ago

Cc: grass-dev@… added
Owner: changed from grass-dev@… to hamish
Status: reopenednew

comment:13 by hamish, 14 years ago

Milestone: 6.4.06.4.1
Priority: criticalminor

ok, I've unconfused myself on this and added some new help notes in 6.5svn+trunk and moved away my off-topic wish to #1104.

The one last thing to do is to figure out how to grey-out the top (current) mapset on the list with Enable(item, False). It's already forcibly ticked but this would be more obvious if greyed as well. Any tips?

suggest to backport mapset notes on mapset access rules for 6.4.1.

Hamish

in reply to:  13 comment:14 by hamish, 13 years ago

Replying to hamish:

The one last thing to do is to figure out how to grey-out the top (current) mapset on the list with Enable(item, False). It's already forcibly ticked but this would be more obvious if greyed as well.

Any ideas on how to do that?

suggest to backport mapset notes on mapset access rules for 6.4.1.

done by martinl in r43911.

Hamish

comment:15 by neteler, 12 years ago

Closing since backport has been done. Reopen if needed.

comment:16 by neteler, 12 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.