Add rename and delete of Location and Mapset in Datacatalog #771
Conversation
…this option is disabled. So far rename mapset and location is implemented for all cases.
gui/wxpython/datacatalog/tree.py
Outdated
| from startup.guiutils import NewMapsetDialog | ||
|
|
||
| from grass.pydispatch.signal import Signal | ||
|
|
||
| from grass.script import core as grass |
gui/wxpython/datacatalog/tree.py
Outdated
| new_location_path = os.path.join(gisenv()['GISDBASE'], | ||
| self.selected_location[0].label) |
There was a problem hiding this comment.
Does this work for you? When I enter any name, I always get Location <definitevely-not-an-existing-directory> already exists in GRASS database.
…elete mapset or location in guiutils.py that uses function from utils.py. Created LocationDialog and edited MapsetDialog.
gui/wxpython/startup/utils.py
Outdated
|
|
||
|
|
||
| def get_default_mapset_name(): | ||
| """Returns default name for mapset.""" | ||
| try: | ||
| defaultName = getpass.getuser() | ||
| defaultName.encode('ascii') | ||
| except UnicodeEncodeError: | ||
| # raise error if not ascii (not valid mapset name) | ||
| defaultName = 'user' | ||
|
|
||
| return defaultName |
There was a problem hiding this comment.
The function is a good fit here. It is totally non-gui and fits with the other functions in this file.
| locations to detect existing GRASS Database. | ||
|
|
There was a problem hiding this comment.
An empty line (without whitespace) is actually desired here. It separates a "brief" from a longer description of the function.
| @@ -48,8 +47,7 @@ def get_possible_database_path(): | |||
|
|
|||
| def create_database_directory(): | |||
| """Creates the standard GRASS GIS directory. | |||
|
|
|||
gui/wxpython/startup/guiutils.py
Outdated
| def location_exists(database, location): | ||
| """Returns True whether location path exists.""" | ||
| location_path = os.path.join(database, location) | ||
| if os.path.exists(location_path): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
I don't know how mapset_exists() got here but both mapset_exists() and location_exists() should be in utils.py.
| return False | ||
|
|
||
|
|
||
| def create_mapset_interactively(guiparent, grassdb, location): |
There was a problem hiding this comment.
Can you please take these new functions, paste them to a separate file, apply Black, and paste them back here? Perhaps check with Flake8 with default settings while you are at it. I see issues here and there, but this will likely just solve them automatically.
gui/wxpython/datacatalog/tree.py
Outdated
| gisenv()['GISDBASE'], | ||
| self.selected_location[0].label, | ||
| self.selected_mapset[0].label) | ||
| self.SwitchLocationMapset(new_mapset=newmapset) |
There was a problem hiding this comment.
Why switch always? I though we talked about renaming not being trigger for switching. Switching is only needed when current mapset or current location are being renamed.
gui/wxpython/datacatalog/tree.py
Outdated
| def SwitchLocationMapset(self, selected_mapset=None, selected_location=None, | ||
| new_mapset=None, new_location=None): | ||
| genv = gisenv() | ||
| if (selected_location == genv['LOCATION_NAME'] | ||
| and selected_mapset == genv['MAPSET']): | ||
| print("bla") | ||
| self.changeMapset.emit(mapset=new_mapset) | ||
| if (selected_location == genv['LOCATION_NAME'] | ||
| and not selected_mapset): | ||
| print("bla2") | ||
| self.changeLocation.emit(location=new_location) | ||
| self.UpdateCurrentLocationMapsetNode() | ||
| self.ExpandCurrentMapset() | ||
| self.RefreshItems() |
There was a problem hiding this comment.
I would need you to document your intentions here in the code to understand what this is supposed to solve. Documentation and comments describing the intent are often useful even when writing the code as they force us to think about the code is supposed to do. For example, for a line add_to_list(abc) we should aim at answering questions such as: Why we are adding to the list? What is the intended outcome? What are the possible outcomes? What are the assumptions about abc or other things before add_to_list() is called?
How is this function different from OnSwitchLocationMapset()? Can there be only one function? Or two functions and one calling the other?
gui/wxpython/gis_set.py
Outdated
| self.bmapset.Bind(wx.EVT_BUTTON, self.OnCreateMapset) | ||
| self.bmapset.Bind(wx.EVT_BUTTON, self.CreateMapset) |
gui/wxpython/gis_set.py
Outdated
| def CreateMapset(self, event): | ||
| """Create new mapset""" | ||
| gisdbase = self.tgisdbase.GetValue() | ||
| location = self.listOfLocations[self.lblocations.GetSelection()] | ||
| try: | ||
| mapset = create_mapset_interactively(self, gisdbase, location) | ||
| self.OnSelectLocation(None) | ||
| self.lbmapsets.SetSelection(self.listOfMapsets.index(mapset)) | ||
| self.bstart.SetFocus() | ||
| return True | ||
| except Exception as e: | ||
| GError(parent=self, | ||
| message=_("Unable to create new mapset: %s") % e, | ||
| showTraceback=False) | ||
| return False | ||
|
|
There was a problem hiding this comment.
If you move this to where the old function was in the file, you will get a better view of your changes in the diff. (A more substantial change would be needed to create some better ordering of the functions here, so just keeping what is here seems to be the best course of action.)
gui/wxpython/gis_set.py
Outdated
|
|
||
| dlg.Destroy() | ||
| try: | ||
| delete_mapset_interactively(self, self.gisbase, location, mapset) |
There was a problem hiding this comment.
gisbase vs gisdbase see https://grass.osgeo.org/grass78/manuals/variables.html
…ot working. OnCreateMapset in gis_set.py moved to the initial place and names of functions changed to 'OnSomething'. Formatting in guiutils.py and utils.py corrected using Black and mapset_exists, location_exists and def_defalt_mapset_name functions moved to utils.py.
|
After 4th commit: I have still a problem with deleting mapset in gis_set.py. Do not know why it looks for file in dist.x86_64-pc-linux-gnu. And this file really does not exist.
The second problem is to switch to current mapset when selecting current mapset for renaming. The program still falls down. |
It looks for it there because you told it to do so, did you read my comment with the link I sent you? |
gui/wxpython/datacatalog/tree.py
Outdated
| def OnSwitchLocationMapset(self, event): | ||
| """Switch to location and mapset""" | ||
| self._SwitchLocationMapset() | ||
|
|
||
| def _SwitchLocationMapset(self): | ||
| """Switch to location and mapset""" | ||
| genv = gisenv() | ||
| if self.selected_location[0].label == genv['LOCATION_NAME']: | ||
| self.changeMapset.emit(mapset=self.selected_mapset[0].label) | ||
| else: | ||
| self.changeLocation.emit(mapset=self.selected_mapset[0].label, location=self.selected_location[0].label) | ||
| self.changeLocation.emit(mapset=self.selected_mapset[0].label, | ||
| location=self.selected_location[0].label) | ||
| self.UpdateCurrentLocationMapsetNode() | ||
| self.ExpandCurrentMapset() | ||
| self.RefreshItems() |
There was a problem hiding this comment.
This looks like a good start. Now you can maybe make use of the option of calling _SwitchLocationMapset() with different parameters based on the the context. However, still:
I would need you to document your intentions here in the code to understand what this is supposed to solve. Documentation and comments describing the intent are often useful even when writing the code as they force us to think about the code is supposed to do. For example, for a line
add_to_list(abc)we should aim at answering questions such as: Why we are adding to the list? What is the intended outcome? What are the possible outcomes? What are the assumptions aboutabcor other things beforeadd_to_list()is called?
There was a problem hiding this comment.
Vashek, ok, I have a function OnRenameMapset that calls function rename_mapset_interactively and returns new mapset name. I think this is implemented right.
But in the case of "if (self.selected_mapset[0].label == gisenv()['MAPSET'])" I need to switch my old mapset to the new one because my current mapset has been actually changed by renaming. But not literally so I need to self.selected_mapset[0].label = newmapset and then call the self._SwitchLocationMapset() function which can be generally the same for both functions OnRenameMapset, OnRenameLocation.
Where is the mistake in my thinking?
There was a problem hiding this comment.
...and _SwitchLocationMapset() uses the currently selected location/mapset to switch to. That all makes sense and should work. So, we have to continue to explore the actual behavior:
When I test renaming the current mapset in GUI, it freezes, but first it shows an error message "Error in g.mapset" saying that the mapset "old_name" does not exist. So, it seems g.mapset is used for the switch and it requires the old mapset to exist.
When I create the same situation in the command line and use g.mapset, I get the same behavior. Also I confirmed that g.mapset is used in ChangeLocationMapset() in datacatalog/frame.py to switch the mapset.
The conclusion here is that mapset cannot be changed when the current does not exist, or in our case, we cannot switch into the one with the new name after we renamed it as long as we are using g.mapset. g.mapset checks correctness of the state of the database connection, i.e., the current mapset, but we are of course breaking that correctness by our rename, so g.mapset is not the right tool for the task.
Therefore, for this ticket, use the same check to prevent user from renaming the current location or mapset. This is something which could be added in the future, but it is a separate issue/additional feature.
The freeze with "Error in g.mapset" window is possibly a separate issue too, but something we should get back to (at least open an issue - please do that if you can reproduce it outside of your PR, e.g., by deleting a the mapset directory in a file manager right before you switch to it in the data catalog).
Oh. There are two similar variables gisbase and gisdbase and I omitted "d" by mistake and did not notice that before. |
…t.py.Deleting of mapset in gis_set.py corrected.
…t mapset or location.
There was a problem hiding this comment.
Great, this is almost finished! Just couple things to fix (and later merge with #761).
gui/wxpython/startup/guiutils.py
Outdated
| newmapset = dlg.GetValue() | ||
| try: | ||
| rename_mapset(grassdb, location, mapset, newmapset) | ||
| return newmapset |
There was a problem hiding this comment.
When you return here, it never goes to dlg.Destroy() (which may leave some stuff behind).
gui/wxpython/startup/guiutils.py
Outdated
| newlocation = dlg.GetValue() | ||
| try: | ||
| rename_location(grassdb, location, newlocation) | ||
| return newlocation |
| message=_("Unable to create new mapset: %s") % err, | ||
| showTraceback=False, | ||
| ) | ||
| return mapset |
There was a problem hiding this comment.
What happens when you press the Cancel button in the Create new mapset dialog?
gui/wxpython/startup/guiutils.py
Outdated
|
|
||
| def _locationAlreadyExists(self, ctrl): | ||
| message = _( | ||
| "Location '{}' already exists. Please consider to use " |
gui/wxpython/gis_set.py
Outdated
| @@ -565,7 +565,7 @@ def OnWizard(self, event): | |||
| defineRegion.Destroy() | |||
|
|
|||
| if gWizard.user_mapset: | |||
| self.OnCreateMapset(event) | |||
| self.CreateMapset(event) | |||
gui/wxpython/gis_set.py
Outdated
| GError(parent=self, | ||
| message=_("Unable to rename location: %s") % e, | ||
| showTraceback=False) | ||
| return False |
There was a problem hiding this comment.
Since this is an event handler (On...(self, event): used as ...Bind(wx.EVT_..., self...), nobody is checking any return value. That would make sense for function which is directly called and then you would have to consider if returning True/False or some other way is the best way to report success/failure (examples of this are rename_location_interactively() and rename_location()).
The code is wrong anyway because it is returning false on failure, but nothing on success (defaults to None in Python, but bad practice not being explicit reason being exactly this situation).
Just remove the return False/True. This applies to the other event handlers (typically On...(self, event):)
…pressing Cancel button in Create, Rename functions corrected. True/false returns removed.
…-Location-and-Mapset
…-Add-rename-and-delete-of-Location-and-Mapset
No description provided.