Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wxGUI: Add new mapset action to data catalog #731

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Jun 23, 2020

No description provided.

…after a right click on a location. Two problems: the mapset is not created after confirmation in the dialog and the dialog box is not displayed for first location in the data tree.
@lindakarlovska lindakarlovska marked this pull request as draft June 23, 2020 20:02
@petrasovaa petrasovaa added gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related labels Jun 24, 2020
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this with @wenzeslaus and there are couple things we would like you do, basically move some of the functionality from gis_set.py to guiutils and utils, specifically:

  • validation: validate legal name, ogr and if mapset already exists
    • create GenericMultiValidator in gui_core/widgets, which will work as GenericValidator, but you can pass tuple of conditions and callbacks
    • NewMapsetDialog will the have the callbacks defined inside the class (_nameValidaionFailed, the ogr check and also test whether the mapset already exists)
    • ogr: we decided we shouldn't allow to create ogr mapset from GUI at all, so adjust the message
    • create a simple function in guiutils to check if mapset exists (using os.path.exists), this will be then used for the validation
  • getDefaultMapsetName should be a standalone function in startup/utils

After this, adjust the code in gis_set.py to accommodate the new changes.

This allows to keep the tree.py cleaner.

Note: the goal here is to only create a mapset, not switch to it.

parent=self,
default=self._getDefaultMapsetName(),
validation_failed_handler=self._nameValidationFailed,
help_hanlder=self.OnHelp,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need the help handler here

else:
return False

def CreateNewMapset(self, mapset):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function would be basically just reduced to calling create_mapset.

Comment on lines 916 to 939
self.listOfMapsets = GetListOfMapsets(self.gisdbase,
self.selected_location[0])
if mapset in self.listOfMapsets:
GMessage(parent=self,
message=_("Mapset <%s> already exists.") % mapset)
return False

if mapset.lower() == 'ogr':
dlg1 = wx.MessageDialog(
parent=self,
message=_(
"Mapset <%s> is reserved for direct "
"read access to OGR layers. Please consider to use "
"another name for your mapset.\n\n"
"Are you really sure that you want to create this mapset?") %
mapset,
caption=_("Reserved mapset name"),
style=wx.YES_NO | wx.NO_DEFAULT | wx.ICON_QUESTION)
ret = dlg1.ShowModal()
dlg1.Destroy()
if ret == wx.ID_NO:
dlg1.Destroy()
return False

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be dealt with by the validator inside the dialog

return False

try:
create_mapset(self.gisdbase, self.selected_location[0], mapset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no self.gisdbase, you can get it e.g. as gisenv()['GISDBASE']

Comment on lines 942 to 945
self.OnSelectLocation(None)
self.lbmapsets.SetSelection(self.listOfMapsets.index(mapset))
self.bstart.SetFocus()

Copy link
Contributor

@petrasovaa petrasovaa Jun 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete, copied from startup screen. Instead you will need to add the mapset node into the tree, see e.g. InsertLayer() in tree.py how it is done for layers (it should be simpler for mapsets).

"""Create popup menu for locations"""
menu = Menu()
genv = gisenv()
currentLocation, currentMapset = self._isCurrent(genv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used, it is actually causing error when right clicking on current location, just remove it

lindakladivova added 2 commits June 25, 2020 05:12
… and callbacks. Defined callback and condition functions in NewMapsetDialog. Adjusted ogr massage in order to not allow to create ogr mapset. Deleted help handler. Created standalone function mapset_exists in utils.py. Renamed getDefaultMapsetName to get_defalt_mapset_name() and moved to utils.py. Adjusted the code in gis_set.py and in tree.py.
…ction mapset_exists moved from utils.py to guiutils.py.
@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jun 25, 2020

We discussed this with @wenzeslaus and there are couple things we would like you do, basically move some of the functionality from gis_set.py to guiutils and utils, specifically:

* validation: validate legal name, ogr and if mapset already exists
  
  * create GenericMultiValidator in gui_core/widgets, which will work as GenericValidator, but you can pass tuple of conditions and callbacks
  * NewMapsetDialog will the have the callbacks defined inside the class (_nameValidaionFailed, the ogr check and also test whether the mapset already exists)
  * ogr: we decided we shouldn't allow to create ogr mapset from GUI at all, so adjust the message
  * create a simple function in guiutils to check if mapset exists (using os.path.exists), this will be then used for the validation

* getDefaultMapsetName should be a standalone function in startup/utils

After this, adjust the code in gis_set.py to accommodate the new changes.

This allows to keep the tree.py cleaner.

Note: the goal here is to only create a mapset, not switch to it.

Now two problems appears:

  • the mapset is created after confirmation in the dialog but ignore GenericMultiValidator
  • after adding mapset in datacatalog, this error appears:
    Screenshot from 2020-06-25 05-49-31

mapset = dlg.GetValue()
try:
create_mapset(self.gisdbase, self.location, mapset)
self.InsertMapset(name=mapset, location_node=self.location,
Copy link
Contributor

@petrasovaa petrasovaa Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

location_node is a of type class DictNode (treemodel.py), not string. Why to pass twice mapset?
You need to pass location node (probably self.selected_location[0] should work). Then most of the code in InsertMapset is to be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to simplify InsertMapset function. And not sure what parameter name means. I think it is necessary to include it but in this case it does not make much sense.

self.InsertMapset(name=mapset, location_node=self.location,
mapset_name=mapset)
return True
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good practice to make the exception more specific if you can. For example, put it only around create_mapset and if you can see what type of exception it throws, it's better to limit it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me some example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For type of error, either just simply try it in a python shell if that's simple, or check Python documentation. You can use OSError here.
Please also read e.g. https://realpython.com/python-exceptions/ and https://realpython.com/the-most-diabolical-python-antipattern/ (ignore the logging part), that should clarify the syntax and motivation.

gui/wxpython/gis_set.py Outdated Show resolved Hide resolved
@petrasovaa petrasovaa linked an issue Jun 25, 2020 that may be closed by this pull request
…dator, corrected OnCreateMapset function in tree.py to be able to create mapsets, refactoring of code.
GError(parent=self,
message=_("Unable to create new mapset: %s") % e,
showTraceback=False)
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar here, I don't think you need to return T/F here

found_element = found_element[0] if found_element else None
found = self._model.SearchNodes(parent=found_element, name=name)
if len(found) == 0:
self._model.AppendNode(parent=found_element, label=name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parent is parent node (here location node), label is mapset name and type is type of node ('mapset' here)

Copy link
Contributor Author

@lindakarlovska lindakarlovska Jun 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a problem with parameters "name" and "mapset_name":
self.InsertMapset(name=mapset,
location_node=self.selected_location[0],
mapset_name=mapset)
I know that it is not possible to have two same parameters but do not know what else to insert for 'name' .
Now I am in this situation. I can create and refresh mapset but the tree has another branch:
Screenshot from 2020-06-28 01-46-27
I spent several hours on that and do not know how to solve it. I is probably simple but I cannot figure that out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what's the problem here, it is pretty simple:

    def InsertMapset(self, name, location_node):
        """Insert mapset into model and refresh tree"""
        self._model.AppendNode(parent=location_node, label=name,
                               data=dict(type='mapset', name=name))
        self._model.SortChildren(location_node)
        self.RefreshNode(location_node, recursive=True)

Comment on lines 809 to 813
found_element = self._model.SearchNodes(
parent=location_node, type='mapset', name=mapset_name)
found_element = found_element[0] if found_element else None
found = self._model.SearchNodes(parent=found_element, name=name)
if len(found) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this

elif self.selected_type[0] and len(self.selected_type) == 1:
self._popupMenuElement()
elif self.selected_mapset[0] and not self.selected_type[0] and len(self.selected_mapset) == 1:
self._popupMenuMapset()
elif self.selected_location[0] and not self.selected_type[0] and len(self.selected_location) == 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why the selected_type, I think it should be selected_mapset instead, but I am not sure, please test that.

…ation, specified more particular exceptions, changed InsertMapset function.
message = _(
"Mapset <%s> already exists. Please consider to use "
"another name for your location.") % {
'name': ctrl.GetValue()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test and fix the message formatting.

"Name <%s> is reserved for direct "
"read access to OGR layers. Please use "
"another name for your mapset.") % {
'name': ctrl.GetValue()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test and fix the message formatting.

message=_("Unable to create new mapset: %s") % err,
showTraceback=False)
print(mapset)
print(self.selected_location[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print statements

@lindakarlovska lindakarlovska marked this pull request as ready for review June 29, 2020 14:18
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still minor unresolved issues, see the comments.

message = _(
"Mapset <%s> already exists. Please consider to use "
"another name for your location.") % {
ctrl.GetValue()}
Copy link
Contributor

@petrasovaa petrasovaa Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obviously not right, please fix again. Use format function.
Existing mapset path_020

It should look like Mapset <anna> already exists...

message = _(
"Name '{}' is not a valid name for location or mapset. "
"Please use only ASCII characters excluding characters {} "
"and space.").format(ctrl.GetValue(),'/"\'@,=*~')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: missing space after comma

@wenzeslaus wenzeslaus added this to In progress in New GUI Startup via automation Jul 1, 2020
@petrasovaa petrasovaa merged commit eac40fa into OSGeo:master Jul 1, 2020
New GUI Startup automation moved this from In progress to Done Jul 1, 2020
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Feat] Add New Mapset action to the Data catalog
3 participants