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 datacatalog: Add multiple GRASS databases #761

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Jul 2, 2020

Currently, only the current database is in the GUI datacatalog. To make this more useful, user needs to be able to add additional ones.

@lindakarlovska lindakarlovska marked this pull request as draft July 2, 2020 14:09
@petrasovaa
Copy link
Contributor

You don't want to create a new database here, the goal is to add an existing database (meaning existing directory with locations (or possibly empty)). It should behave like the Browse button in the current start up screen.

Comment on lines 586 to 594
if node.data['type'] == 'grassdb':
font.SetWeight(wx.FONTWEIGHT_BOLD)
elif node.data['type'] in ('location', 'mapset'):
if node in (self.current_location_node, self.current_mapset_node):
if node in (self.current_grassdb_node, self.current_location_node, self.current_mapset_node):
font.SetWeight(wx.FONTWEIGHT_BOLD)
Copy link
Contributor

Choose a reason for hiding this comment

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

This part needs to be fixed, it would highlight all grassdbs

Comment on lines 1176 to 1169
def _popupMenuGrassDb(self):
"""Create popup menu for grass db"""
raise NotImplementedError()

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead create empty menu, it should not throw error.

Comment on lines 971 to 987
def CreateGrassDb(self):
"Create new grass database"
root = self._model.root

dlg = NewGrassDbDialog(
parent=self,
)
if dlg.ShowModal() == wx.ID_OK:
db = dlg.GetValue()
try:
create_database_directory(db)
except OSError as err:
GError(parent=self,
message=_("Unable to create new grass database: %s") % err,
showTraceback=False)
self.InsertGrassDb(name=db,
root_node=root)
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

self.OnSetRestriction, wx.ITEM_CHECK)
self.OnSetRestriction, wx.ITEM_CHECK),
("createGrassDb", icons['locked'],
self.parent.OnCreateGrassDb)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should call something like OnAddGrassDB which opens a standard file dialog to select a directory and then inserts the new node and reloads data.

@petrasovaa
Copy link
Contributor

None of the changes outside of datacatalog/ dir should be needed.

@petrasovaa petrasovaa closed this Jul 3, 2020
@petrasovaa petrasovaa reopened this Jul 3, 2020
@petrasovaa petrasovaa added gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related labels Jul 3, 2020
@petrasovaa petrasovaa added this to In progress in New GUI Startup via automation Jul 3, 2020
@petrasovaa petrasovaa linked an issue Jul 3, 2020 that may be closed by this pull request
Comment on lines 73 to 83
def MakeButton(self, text, id=wx.ID_ANY, size=(-1, -1),
parent=None, tooltip=None):
"""Generic button"""
if not parent:
parent = self
button = Button(parent=parent, id=id, label=text,
size=size)
if tooltip:
button.SetToolTip(tooltip)
return button

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, this should be a toolbar button, not a standard button.

@petrasovaa
Copy link
Contributor

You need to fix OnGetItemFont and OnGetItemImage. Since you call RefreshNode on root, and root doesn't have type, it fails.
Another problem is that the initial db is added without a name, so SearchNodes in GetCurrentDbLocationMapsetNode fails.
What is still missing is reloading the content of the newly added database, for now reloading everything will work (ReloadTreeItems). For future we need more sophisticated reloading of only the newly added database.

@petrasovaa
Copy link
Contributor

I added a new icon grassdb-add to master, so you can rebase to master and use that icon.

@lindakarlovska lindakarlovska force-pushed the wxGUI-datacatalog-Add-multiple-GRASS-databases branch from 071734e to 300fee1 Compare July 5, 2020 05:22
@@ -1,119 +0,0 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove this file from the repository.

self._orig_model = self._model
self._model.SortChildren(self._model.root)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why sort here?

@@ -407,7 +416,7 @@ def get_first_child(node):
p = Process(
target=getLocationTree,
args=(
genv['GISDBASE'],
self.selected_grassdb[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

use self.current_grassdb_node

else:
self.changeLocation.emit(mapset=self.selected_mapset[0].label, location=self.selected_location[0].label)
self.UpdateCurrentLocationMapsetNode()
self.changeGrassDb.emit(mapset=self.selected_mapset[0].label,
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing to location in the same or in a different db is the same, so changeGrassDb is not needed, just use changeLocation but add the grassdb parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you mean that? grassdb is not the parameter in changeLocation function, I need to add it but I do not know how. I sought in signal.py but I think you have meant it differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Search for changeLocation.connect, that's how signal handlers are called.

Comment on lines 847 to 852
def InsertGrassDb(self, name):
"""Insert grass db into model and refresh tree"""
self._model.SortChildren(self._model.root)
self.ReloadTreeItems()

Copy link
Contributor

Choose a reason for hiding this comment

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

As I tried to explain, this should still add the node because in the future you would add a function to reload just the db here. Also add the db here to _grassdatabases. Call ReloadTreeItems from the OnAddDB handler.

@@ -32,6 +32,9 @@
'locked': MetaIcon(
img='locked',
label=_("Click to allow editing other mapsets")),
'addGrassDB': MetaIcon(
img='grassdb-add',
label=_("Click to add new grass database"))
Copy link
Contributor

Choose a reason for hiding this comment

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

uppercase GRASS

lindakladivova added 5 commits July 9, 2020 04:54
… functions edited especially GetCurrentDbLocationMapsetNode, OnSwitchDbLocationMapset,_isCurrent. Added InsertGrassDb function. Added OnCreateGrassDb to DataCatalogToolbar. create_database_directory was made more general in order to draw up NewGrassDbDialog class with exceptions.
…atacatalog/ dir are needed. Function OnAddGrassDB created that called function InsertGrassDb.
…ction ReloadTreeItems. Added toolbar button with particular image.
…l selected db, location and mapset. OnGetItemFont function changed.
@lindakarlovska lindakarlovska force-pushed the wxGUI-datacatalog-Add-multiple-GRASS-databases branch from daef1fe to 223a72c Compare July 9, 2020 12:21
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.

Generally, delete prints, run flake8. More specific comments are in the code.

gisrc2, env2 = gscript.create_environment(
gisenv()['GISDBASE'], self.copy_location[i].label, mapset=self.copy_mapset[i].label)
self.selected_grassdb[i].label,
Copy link
Contributor

Choose a reason for hiding this comment

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

self.copy_grassdb[i].label

mapset=self.selected_mapset[0].label):

if self.selected_grassdb[0] == self.copy_grassdb[i]:
if self.selected_location[0] == self.copy_location[i]:
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of the condition here is to test if you are copying/moving map in a different location, in that case you need to reproject. If it's within the same location you don't need to reproject. If the location is in a different database, that doesn't make difference, you still need to reproject. So just remove that test you added.

@@ -761,7 +798,7 @@ def OnPasteMap(self, event):
element=self.copy_layer[i].data['type'])
if not new_name:
continue
gisdbase = gisenv()['GISDBASE']
gisdbase = self.selected_grassdb[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you need to distinguish between the database from where we reproject and to which database we reproject.

@@ -499,6 +516,8 @@ def OnRightClick(self, node):
self._popupMenuMapset()
elif self.selected_location[0] and not self.selected_mapset[0] and len(self.selected_location) == 1:
self._popupMenuLocation()
elif self.selected_grassdb[0] and not self.selected_location[0] and not self.selected_mapset[0] and len(self.selected_grassdb) == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

and not self.selected_mapset[0] is redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that only for the line starting elif self.selected_grassdb[0]

if dlg.ShowModal() == wx.ID_OK:
grassdatabase = dlg.GetPath()
self.tree.InsertGrassDb(name=grassdatabase)
self.tree.ReloadTreeItems()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we in the end decided to put reloadtreeitems in insertgrassdb

self._orig_model = self._model
self._model.RemoveNode(self._model.root)
self.InitTreeItems()
self.UpdateCurrentLocationMapsetNode()
self.UpdateCurrentDbLocationMapsetNode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this at the end of _initTreeItems just before RefreshItems so that is is properly refreshed.

@lindakarlovska
Copy link
Contributor Author

After sixth commit:
Switching to mapset in different grassdb still does not work. Do not know how to add grassdb parameter to self.changeLocation.emit

@petrasovaa
Copy link
Contributor

Copying between different databases doesn't work, you need to provide 2 grassdb to CatalogReprojectionDialog

…ching between two GRASS db still does not work. Than copying of layers in CatalogReprojectionDialog corrected. OnRightClick function also edited.
@lindakarlovska
Copy link
Contributor Author

Copying between different databases doesn't work, you need to provide 2 grassdb to CatalogReprojectionDialog

I have corrected this but still have a problem with the switching between databases. There is the same error as when we tried to switch to current mapset in #771:
Screenshot from 2020-07-11 03-42-23
No not know how to figure that out.

@petrasovaa
Copy link
Contributor

Copying between different databases doesn't work, you need to provide 2 grassdb to CatalogReprojectionDialog

I have corrected this but still have a problem with the switching between databases. There is the same error as when we tried to switch to current mapset in #771:
Screenshot from 2020-07-11 03-42-23
No not know how to figure that out.

You also need to fix it in lmgr/frame.py

self.ChangeLocationMapset(location=location,
self.tree.changeLocation.connect(lambda mapset, location, dbase:
self.ChangeDbLocationMapset(dbase=dbase,
location=location,
Copy link
Contributor

Choose a reason for hiding this comment

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

fix indentation

"Current location is <%(loc)s>.\n"
"Current mapset is <%(mapset)s>."
) %
{'dbase': dbase, 'loc': location, 'mapset': mapset})
if location:
Copy link
Contributor

Choose a reason for hiding this comment

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

if dbase and location are passed, it's switching twice

@petrasovaa petrasovaa marked this pull request as ready for review July 13, 2020 02:32
@petrasovaa petrasovaa merged commit 423ccab into OSGeo:master Jul 13, 2020
New GUI Startup automation moved this from In progress to Done Jul 13, 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] wxGUI datacatalog: Add multiple GRASS databases
3 participants