Switch to another mapset when in use (aka force remove lock) #906
Conversation
…ith new custom question dialog. So far not working suggestion.
lindakarlovska
added
gsoc
gui/wxpython/gui_core/dialogs.py
Outdated
| class CustomQuestionDialog(wx.Dialog): | ||
|
|
||
| def __init__(self, parent, message, caption='', label1='', label2='', | ||
| id=wx.ID_ANY, style=wx.DEFAULT_DIALOG_STYLE | wx.RESIZE_BORDER, | ||
| **kwargs): | ||
| """Question Dialog with two custom buttons | ||
|
|
||
| :param parent: window | ||
| """ | ||
| wx.Dialog.__init__(self, parent, id, caption, style=style, **kwargs) | ||
| self.panel = wx.Panel(parent=self, id=wx.ID_ANY) | ||
|
|
||
| self._icon = wx.StaticBitmap( | ||
| self.panel, wx.ID_ANY, | ||
| wx.ArtProvider().GetBitmap( | ||
| wx.ART_QUESTION, | ||
| client=wx.ART_MESSAGE_BOX)) | ||
|
|
||
| self.informLabel = StaticText( | ||
| parent=self.panel, id=wx.ID_ANY, label=message) | ||
| self.btnClose = Button(parent=self.panel, id=wx.ID_NO, | ||
| label=label1) | ||
| self.btnClose.SetFocus() | ||
| self.btnContinue = Button(parent=self.panel, id=wx.ID_YES, | ||
| label=label2) | ||
|
|
||
| self.btnClose.Bind(wx.EVT_BUTTON, self.OnClose) | ||
| self.btnContinue.Bind(wx.EVT_BUTTON, self.OnContinue) | ||
|
|
||
| self.__layout() | ||
|
|
||
| def __layout(self): | ||
| """Do layout""" | ||
| sizer = wx.BoxSizer(wx.VERTICAL) | ||
|
|
||
| btnSizer = wx.BoxSizer(wx.HORIZONTAL) | ||
| btnSizer.Add(self.btnClose, flag=wx.RIGHT, border=5) | ||
| btnSizer.Add(self.btnContinue, flag=wx.RIGHT, border=5) | ||
|
|
||
| bodySizer = wx.BoxSizer(wx.HORIZONTAL) | ||
| bodySizer.Add(self._icon, flag=wx.RIGHT, border=10) | ||
| bodySizer.Add(self.informLabel, proportion=1, flag=wx.EXPAND) | ||
|
|
||
| sizer.Add(bodySizer, proportion=1, | ||
| flag=wx.EXPAND | wx.ALL, border=15) | ||
| sizer.Add(btnSizer, proportion=0, | ||
| flag=wx.ALL | wx.ALIGN_RIGHT, border=5) | ||
|
|
||
| self.panel.SetSizer(sizer) | ||
| sizer.Fit(self) | ||
| self.Layout() | ||
|
|
||
| def OnClose(self, event): | ||
| self.EndModal(wx.ID_NO) | ||
|
|
||
| def OnContinue(self, event): | ||
| self.EndModal(wx.ID_YES) | ||
|
|
||
|
|
There was a problem hiding this comment.
This is not needed just to relabel the buttons... See https://wxpython.org/Phoenix/docs/html/wx.MessageDialog.html
gui/wxpython/startup/guiutils.py
Outdated
| @@ -692,6 +697,69 @@ def delete_grassdb_interactively(guiparent, grassdb): | |||
| return deleted | |||
|
|
|||
|
|
|||
| def switch_mapset_interactively(guiparent, grassdb, location, mapset): | |||
There was a problem hiding this comment.
Let's rename this to can_switch_mapset_interactively and it won't be actually switching the mapset, just checking and removing the lockfile. The signals would be called from tree.py as before. This function could be also used in lmgr/frame.py in OnChangeMapset/Location.
… for overiding labels. Function switch_mapset_interactively renamed to can_switch_mapset_interactively and the most of functionality moved to tree.py. This function is also used in frame.py in functions OnChangeMapset and OnChangeLocation.
|
The functionality within data catalog tree works. The dialog looks as follows: Now I have a problem with functions OnChangeMapset and OnChangeLocation in frame.py. Within this pull request, I would like to make "Change location" and "Change location and mapset" buttons working. The problem is that I probably need to create the DataCatalogTree object because I should update the tree. And also I need to call signals.... |
|
If I recall correctly, Vashek told me that there should be a timestamp logger of gislock in grass.py. I could not find it. --> In the end, I add the timestamp using datatime and getmtime functions. |
lib/python/grassdb/create.py
Outdated
| def get_current_user(): | ||
| """Returns current user.""" |
There was a problem hiding this comment.
Keep the original name, if you want get_current_user, add a new function.
There was a problem hiding this comment.
I think that "get_current_user" is a much more generic name and makes more sense even in create_mapset_interactively function where the default is set to get_current_user(). It means that immediately we know that default mapset name is the current user's name.
By the way, this function is so far used only in guiutils when switching mapset and when creating a new mapset.
There was a problem hiding this comment.
I would let the name get_current_user() but maybe move that function to grassdb/checks. I have noticed that in checks.py there are functions like get_mapset_owner, get_lockfile_if_present which have similar behavior = we try to GET something. And those functions do not perform any checks. Similarly like get_current_user(). It might be the reason why to set up different let's say get.py file.
There was a problem hiding this comment.
The function name should be get_default_mapset_name() because the point of the function is to provide a string which is a name for mapset which should be used by default if there is no more specific name available or user does not provide it, e.g., a default value in a dialog or automatically created mapset.
It should be in grass.grassdb.create because it is relevant when creating new mapset and/or location.
These are exactly the usages when this function is used: default value in mapset creation dialog and (in #868) name of a generated mapset in the startup/demo location.
In create_mapset_interactively() the point of using get_default_mapset_name() is exactly that we want to get a default name for a mapset. We don't particularly care what the name is, i.e., we defer the decision to the implementation of that function. It is the question of semantics. What we emphasize in the API is the purpose of the value, not what the value is. The get_default_mapset_name() documentation can, or probably should, describe what the value is and why it is this way.
The point in making the distinction is that we can change the decision of what the default name is just by modifying this function, not all places where the default name is used. Additionally, the implementation of that function can be and in fact is geared towards that usage, i.e., it is not just purely for getting the current user name. Given its purpose, it always returns some string, so it returns "user" when the actual name was not find. This is a great behavior in context of getting some usable name for mapset, but it is rather unexpected and potentially harmful if the purpose is obtaining a user name. This would become even more prominent when more sophisticated replacement of non-compliant characters is added (see current code or e.g. comments in #897 for motivation). Consequently, using the function given its current implementation for getting user name is wrong.
gui/wxpython/startup/guiutils.py
Outdated
|
|
||
| user = get_current_user() | ||
| lockfile = get_lockfile_if_present(grassdb, location, mapset) | ||
| timestamp = (datetime.datetime.fromtimestamp( |
There was a problem hiding this comment.
You didn't test this properly, now you can't switch to mapset without a lockfile.
There was a problem hiding this comment.
That's strange. I am testing it now and it works fine.
lib/python/grassdb/create.py
Outdated
| def get_current_user(): | ||
| """Returns current user.""" |
There was a problem hiding this comment.
The function name should be get_default_mapset_name() because the point of the function is to provide a string which is a name for mapset which should be used by default if there is no more specific name available or user does not provide it, e.g., a default value in a dialog or automatically created mapset.
It should be in grass.grassdb.create because it is relevant when creating new mapset and/or location.
These are exactly the usages when this function is used: default value in mapset creation dialog and (in #868) name of a generated mapset in the startup/demo location.
In create_mapset_interactively() the point of using get_default_mapset_name() is exactly that we want to get a default name for a mapset. We don't particularly care what the name is, i.e., we defer the decision to the implementation of that function. It is the question of semantics. What we emphasize in the API is the purpose of the value, not what the value is. The get_default_mapset_name() documentation can, or probably should, describe what the value is and why it is this way.
The point in making the distinction is that we can change the decision of what the default name is just by modifying this function, not all places where the default name is used. Additionally, the implementation of that function can be and in fact is geared towards that usage, i.e., it is not just purely for getting the current user name. Given its purpose, it always returns some string, so it returns "user" when the actual name was not find. This is a great behavior in context of getting some usable name for mapset, but it is rather unexpected and potentially harmful if the purpose is obtaining a user name. This would become even more prominent when more sophisticated replacement of non-compliant characters is added (see current code or e.g. comments in #897 for motivation). Consequently, using the function given its current implementation for getting user name is wrong.
gui/wxpython/startup/guiutils.py
Outdated
| user = get_current_user() | ||
| lockfile = get_lockfile_if_present(grassdb, location, mapset) | ||
| timestamp = (datetime.datetime.fromtimestamp( | ||
| os.path.getmtime(lockfile))).replace(microsecond=0) | ||
| if lockfile: | ||
| dlg = wx.MessageDialog( | ||
| parent=guiparent, | ||
| message=_("User {user} is already running GRASS in selected mapset " |
There was a problem hiding this comment.
This gets the current user and than it claims it is the one running the session. Well, that may or may not be true. (It is actually a more useful information when it is not true.)
It seems that the best way of obtaining it is pathlib's Path.owner.
There was a problem hiding this comment.
This gets the current user and than it claims it is the one running the session. Well, that may or may not be true. (It is actually a more useful information when it is not true.)
It seems that the best way of obtaining it is pathlib's
Path.owner.
I made actually the same function as get_mapset_owner, just for gislock owner. Maybe we could make one function get_file_owner instead of get_mapset_owner and get_gislock_owner.
gui/wxpython/lmgr/frame.py
Outdated
| grass.gisenv()['GISDBASE'], | ||
| grass.gisenv()['LOCATION_NAME'], |
There was a problem hiding this comment.
This is actually two module calls. Do gisenv = grass.gisenv() and then use it like elsewhere.
gui/wxpython/startup/guiutils.py
Outdated
| @@ -692,6 +697,58 @@ def delete_grassdb_interactively(guiparent, grassdb): | |||
| return deleted | |||
|
|
|||
|
|
|||
| def can_switch_mapset_interactively(guiparent, grassdb, location, mapset): | |||
There was a problem hiding this comment.
Although this is consistent name with the others, the name is little misleading because it reads as "Can I switch mapset interactively?" when in fact it should be saying more "Check interactively if I can switch mapset". I think can_switch_mapset_interactive() keeps it simple and it expresses well that it is a "can (I) switch mapset? but doing it in interactive manner.
gui/wxpython/startup/guiutils.py
Outdated
| dlg.SetYesNoLabels("&Switch to selected mapset", | ||
| "&Stay in current mapset") |
There was a problem hiding this comment.
The & is what is used to identify a character which will be used with Alt key to "press" the button without using a mouse pointer. They need to be at least different, but I would probably go with "S&witch... and "S&tay....
gui/wxpython/startup/guiutils.py
Outdated
| mapset=mapset, | ||
| lockfile=lockfile, | ||
| timestamp=timestamp), | ||
| caption=_("Lock file found"), |
There was a problem hiding this comment.
The terminology used in #849 is "in use" so "Mapset is in use".
lib/python/grassdb/checks.py
Outdated
| def get_gislock_owner(lock_path): | ||
| """Returns gislock owner name or None if owner name unknown""" | ||
| try: | ||
| path = Path(lock_path) | ||
| return path.owner() | ||
| except KeyError: | ||
| return None |
There was a problem hiding this comment.
I think the best thing to do with this function is to turn it into get_mapset_lock_info(mapset_path). It can get also the timestamp (and there is one other thing which this could get but that's too much for this PR). Given that there will/might be extensions and for the lack of better ideas, the return type should be dict with keys owner and datetime.
Actually, let's add also filename as another key which will remove the need to call get_lockfile_if_present() and you can use the nicer is_mapset_locked().
…github.com/lindakladivova/grass into wxGUI-Switch-to-another-mapset-when-in-use
Describe the solution
The data catalog should inform about the mapset being in use as indicated by the presence of the lock, but offer an option to force removal of the lock and switch to the mapset anyway. The primary choice/button should be the "stay in current mapset" option.
I think the asking twice as it is now in the startup window should be removed and instead information about the lock (i.e., the possible session there) should be provided. Some code for this is already in lib/init/grass.py and is used when using grass db/loc/mapset_which_is_in_use --text in the command line (not a reusable library function, but something to copy-paste and maybe to add to grass.grassdb).
This basically leads to switch_mapset_interactively() function. Using it in the startup window instead of the current code could help in making it more generic.