Distinguish mapsets by ownership and lock in Data tab #849

lindakarlovska · 2020-07-29T14:04:42Z

In the startup screen it would be nice if mapsets could be displayed differently (e.g. different colors, for "my mapsets", "my locked mapsets", "others mapsets").

Mapsets owned by different user and locked mapsets will be displayed in gray and distinguished from "normal mapsets" by special label.

…er to get id for label and function OnGetItemNote which should change the node label according to gislock and user cases.

petrasovaa · 2020-07-29T14:28:30Z

lib/python/grassdb/checks.py

+def get_user_if_different(mapset_path):
+    """Return user if different, None otherwise.
+
+    Returns the name as a string or None if nothing was found, so the
+    return value can be used to test if mapset belongs to another user.
+    """
+    if os.environ.get("GRASS_SKIP_MAPSET_OWNER_CHECK", None):
+        # Mapset just needs to be accessible for writing.
+        return os.access(mapset_path, os.W_OK)
+    # Mapset needs to be owned by user.
+    stat_info = os.stat(mapset_path)
+    mapset_uid = stat_info.st_uid
+    if mapset_uid != os.getuid():
+        return os.getuid()
+    return None
+
+


Instead of this, it should be more get_mapset_owner, which will return the owner even if it's the the user.

petrasovaa · 2020-07-29T14:35:41Z

gui/wxpython/datacatalog/tree.py

+    def OnGetItemNote(self, index):
+        """Overriden method to return note for each mapset.
+           Used to distinguish mapsets by lock and ownership."""
+        node = self._model.GetNodeByIndex(index)
+        node.label = node.data['name']
+        if node.data['type'] in ('mapset'):
+            mapset_path = os.path.join(node.parent.parent.data['name'],
+                                       node.parent.data['name'],
+                                       node.data['name'])
+            user = get_user_if_different(mapset_path)
+            if is_mapset_locked(mapset_path):
+                node.label = _("<{label}> (locked)").format(mapset=node.label)
+            if user:
+                node.label = _("<{label}> ({user})").format(mapset=node.label,
+                                                          user=user)
+        return node.label
+


OnGetItemSomething are defined methods in wx that need to be overriden, so you can't just create a new one.

The idea here is to add more info to node.data, e.g. node.data['lock'], and then change DataCatalogNode.label method.

No sure where to add more information to node.data. Where is e.g. node.data['name'] defined?

No sure where to add more information to node.data. Where is e.g. node.data['name'] defined?

I haven't delved deeply into this code, yet, but IIUC, these are defined whenever you create a dictionary for a tree node, e.g. on lines 336ff (in this PR):

mapset_node = self._model.AppendNode( parent=location_node, data=dict(type='mapset', name=mapset, lock=is_mapset_locked(mapset_path), user=different_user))

So, "lock" seems to be defined for mapset nodes whenever they are created (also elsewhere in the code). But AFAICT location_nodes do not include the "lock" key.

OnGetItemSomething are defined methods in wx that need to be overriden, so you can't just create a new one.

The idea here is to add more info to node.data, e.g. node.data['lock'], and then change DataCatalogNode.label method.

IIUC, this is true for many other OnGetItem method that you propose.

…ion and broadening property function.

lindakarlovska · 2020-07-31T16:04:54Z

In my opinion, there is a problem that only for mapset node I defined "lock" and "user" keys. But other nodes do not have these keys. The error is:

petrasovaa · 2020-07-31T16:54:20Z

gui/wxpython/datacatalog/tree.py

+        if node.data['lock'] == True or node.data['user']:
+            text_colour.SetTextColour(wx.SYS_COLOUR_GRAYTEXT)
+        else:
+            text_colour.SetTextColour(wx.SYS_COLOUR_WINDOWTEXT)


You need to return directly the wx.SYS....

Delete '== True '

gui/wxpython/datacatalog/tree.py

petrasovaa · 2020-07-31T16:57:22Z

gui/wxpython/datacatalog/tree.py

-                                data=dict(type='mapset', name=mapset))
+                                data=dict(type='mapset',
+                                          name=mapset,
+                                          lock=locked,


Just pass directly is_mapset_locked result

…appending a mapset node in _reloadLocationNode, _reloadGrassDBNode, GetCurrentDbLocationMapsetNode. Returned directly wx.SYS colour.

mlennert · 2020-08-03T16:01:29Z

In my opinion, there is a problem that only for mapset node I defined "lock" and "user" keys. But other nodes do not have these keys. The error is:

Yes, only mapset_nodes have "lock" defined. I'm not familiar enough with the code to be able to provide you with the optimum solution, but for now, can't you change the code so that it

In my opinion, there is a problem that only for mapset node I defined "lock" and "user" keys. But other nodes do not have these keys. The error is:

I can get further by working around the issue with something like this:

    @property
    def label(self):
        if self.data["type"] == "mapset":
            if self.data["lock"] and self.data["user"]:
                return self.data["name"] + " <user: {}>".format(self.data["user"]) + " (locked)"
            elif self.data["lock"]:
                return self.data["name"] + " (locked)"
            elif self.data["user"]:
                return self.data["name"] + " <user: {}>".format(self.data["user"])
            else:
                return self.data["name"]
        else:
            return self.data["name"]

but that seems a bit overly complicated to me. Maybe there needs to be some differentiation of methods depending on the type of node ?
When using the above code, I get the following error:

Exception in thread
Thread-1
:
Traceback (most recent call last):
  File "/usr/lib/python3.8/threading.py", line 932, in
_bootstrap_inner

self.run()
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/gthread.py", line 99, in run

ret = vars()['callable'](*args, **kwds)
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 467, in
ReloadTreeItems

self._reloadTreeItems()
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 452, in
_reloadTreeItems

self.RefreshItems()
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 371, in
RefreshItems

self.RefreshChildrenRecursively(rootItem)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 395, in
RefreshChildrenRecursively

self.RefreshItemRecursively(child, childIndex)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 402, in
RefreshItemRecursively

item = self.DoRefreshItem(item, itemIndex, hasChildren)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 416, in
DoRefreshItem

self.RefreshTextColour(item, index)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 433, in
RefreshTextColour

self.__refreshAttribute(item, index, 'ItemTextColour')
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 482, in
__refreshAttribute

value = getattr(self, 'OnGet%s'%attribute)(index, *args)
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 647, in
OnGetItemTextColour

text_colour = self.GetTextColour()
AttributeError
:
'DataCatalogTree' object has no attribute 'GetTextColour'

which sounds like an issue linked to what Anna already highlighted above.

…lace. 'lock' and 'user' attribute are now defined only for mapsets. OnGetItemTextColour was corrected to return system colours.

lindakarlovska · 2020-08-04T11:01:31Z

In my opinion, there is a problem that only for mapset node I defined "lock" and "user" keys. But other nodes do not have these keys. The error is:

Yes, only mapset_nodes have "lock" defined. I'm not familiar enough with the code to be able to provide you with the optimum solution, but for now, can't you change the code so that it

In my opinion, there is a problem that only for mapset node I defined "lock" and "user" keys. But other nodes do not have these keys. The error is:

I can get further by working around the issue with something like this:

    @property
    def label(self):
        if self.data["type"] == "mapset":
            if self.data["lock"] and self.data["user"]:
                return self.data["name"] + " <user: {}>".format(self.data["user"]) + " (locked)"
            elif self.data["lock"]:
                return self.data["name"] + " (locked)"
            elif self.data["user"]:
                return self.data["name"] + " <user: {}>".format(self.data["user"])
            else:
                return self.data["name"]
        else:
            return self.data["name"]

but that seems a bit overly complicated to me. Maybe there needs to be some differentiation of methods depending on the type of node ?
When using the above code, I get the following error:

Exception in thread
Thread-1
:
Traceback (most recent call last):
  File "/usr/lib/python3.8/threading.py", line 932, in
_bootstrap_inner

self.run()
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/gthread.py", line 99, in run

ret = vars()['callable'](*args, **kwds)
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 467, in
ReloadTreeItems

self._reloadTreeItems()
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 452, in
_reloadTreeItems

self.RefreshItems()
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 371, in
RefreshItems

self.RefreshChildrenRecursively(rootItem)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 395, in
RefreshChildrenRecursively

self.RefreshItemRecursively(child, childIndex)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 402, in
RefreshItemRecursively

item = self.DoRefreshItem(item, itemIndex, hasChildren)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 416, in
DoRefreshItem

self.RefreshTextColour(item, index)
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 433, in
RefreshTextColour

self.__refreshAttribute(item, index, 'ItemTextColour')
  File "/usr/lib/python3/dist-
packages/wx/lib/mixins/treemixin.py", line 482, in
__refreshAttribute

value = getattr(self, 'OnGet%s'%attribute)(index, *args)
  File "/home/mlennert/SRC/GRASS/grass/dist.x86_64-pc-linux-
gnu/gui/wxpython/datacatalog/tree.py", line 647, in
OnGetItemTextColour

text_colour = self.GetTextColour()
AttributeError
:
'DataCatalogTree' object has no attribute 'GetTextColour'

which sounds like an issue linked to what Anna already highlighted above.

I corrected this above-mentioned issue.

petrasovaa

I think we need to treat the current mapset differently. Right now it is grey (and 'locked') and I think that's confusing, so I would add another key to data - data['current'] (that would be bool) and then use that in label() and OnGetItemTextColour to show it in black and with mymapset (current).

When you call AppendNode and specify data there, I would just use data['current'] = False and then in GetCurrentDbLocationMapsetNode you need to

find all nodes type=mapset and set their data['current'] = False
set the current mapset node data['current'] = True

Setting the other nodes to false is needed because you can switch current mapsets.

petrasovaa · 2020-08-07T18:03:21Z

gui/wxpython/datacatalog/tree.py

+            elif self.data["lock"]:
+                return self.data["name"] + " (locked)"
+            elif self.data["user"]:
+                return self.data["name"] + " <user: {}>".format(self.data["user"])


I would use "(owner: xxx)" and "(in use, owner: xxx)"
We thought "in use" might be more understandable than 'locked'

also, it should be translatable _()

petrasovaa · 2020-08-07T18:10:41Z

gui/wxpython/datacatalog/tree.py

+            # Check mapset ownership
+            different_user = None
+            if not is_mapset_users(mapset_path):
+                different_user = get_mapset_owner(mapset_path)


You should probably have data["owner"] (that would be result of get_mapset_owner) and additionally data["is_different_owner"] (result of is_mapset_users - bool). Then just adjust the label() code. This is more cosmetics, but it makes sense more like that.

"Check mapset ownership" part removed and mapset_node dictionary (lock, is_different_owner, owner) set. All decisions about what label to use are now made in @Property function.

petrasovaa · 2020-08-07T18:16:08Z

gui/wxpython/datacatalog/tree.py

+            name=mapset,
+            type='mapset',
+            lock=is_mapset_locked(mapset_path),
+            user=different_user)


I don't think this is needed, it will search the nodes based on name and mapset, that's enough.

Simplified.

…er, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetKeys.

…er, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetNodes.

lindakarlovska · 2020-08-08T10:30:47Z

I think we need to treat the current mapset differently. Right now it is grey (and 'locked') and I think that's confusing, so I would add another key to data - data['current'] (that would be bool) and then use that in label() and OnGetItemTextColour to show it in black and with mymapset (current).

When you call AppendNode and specify data there, I would just use data['current'] = False and then in GetCurrentDbLocationMapsetNode you need to
1. find all nodes type=mapset and set their data['current'] = False

2. set the current mapset node data['current'] = True
Setting the other nodes to false is needed because you can switch current mapsets.

The first point is implemented in function UpdateMapsetNodes which is then used in _switchDbLocationMapset where I also performed the second point - set the current mapset node data['current'] = True.
Similarly those two lines self.UpdateMapsetNodes() and mapsetItem[0].data['current'] = True, are used in function GetCurrentDbLocationMapsetNode.

wenzeslaus · 2020-08-08T18:10:44Z

lib/python/grassdb/checks.py

-def is_mapset_users(mapset_path):
-    """Check if the mapset belongs to the user"""
+def is_different_mapset_owner(mapset_path):
+    """Check if the mapset belongs to the different owner than the current user."""


Why did you need to change is_mapset_users() into is_different_mapset_owner()? If it is just about clarity of the name then is_current_user_mapset_owner() might be better. If it is just about which case it reports as true, then mapset being owned by the current users is the primary or positive case, so I would have function for that and negate the result if need. (The point here is that this is a general functionality/API, not limited to data catalog.) If you really feel like function for the opposite case is needed, then you can wrap the existing one.

Besides the naming and what it is supposed to do, the current implementation is not correct: The GRASS_SKIP_MAPSET_OWNER_CHECK case does not correspond to is_different_mapset_owner().

I preserved the function is_mapset_users() but renamed it to is_current_user_mapset_owner(). Then I made the wrapping function called is_different_mapset_owner().

gui/wxpython/datacatalog/tree.py

wenzeslaus · 2020-08-08T18:32:24Z

gui/wxpython/datacatalog/tree.py

-                return self.data["name"] + " <user: {}>".format(self.data["user"])
+        if self.data['type'] == 'mapset':
+            if self.data['current']:
+                return self.data['name'] + _(" (current)")


More general way to do this is:

_("{name} (current)").format(name=data["name"])

or making use of the fact data is a dictionary:

_("{data[name]} (current)").format(data=data)

or making use of the fact that the only values we are using are from the dictionary:

_("{name} (current)").format(**data)

It is more flexible and possibly also more readable as you see the basic structure of the string being constructed, but in the other cases it is also likely faster since format is already used there anyway.

Rewritten to _("{name} (current)").format(**data).

…pset_owner, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetNodes." This reverts commit 75b993f.

…served the function is_mapset_users() but renamed to is_current_user_mapset_owner(). New wrapper for opposite case is_different_mapset_owner() created. In function CreateMapset is now used _reloadMapsetNode function. UpdateCurrentMapsetNode is applies only for working with current mapset.UpdateMapsetInfo is the general function for updating mapset attributes.

…es, fix checks.py

…-lock-in-data-tab

gui/wxpython/datacatalog/tree.py

wenzeslaus · 2020-08-11T16:52:07Z

wxGUI/datacatalog: Created too functions get_user_if_different in ord…

959ee18

…er to get id for label and function OnGetItemNote which should change the node label according to gislock and user cases.

petrasovaa reviewed Jul 29, 2020

View reviewed changes

petrasovaa added gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related labels Jul 30, 2020

petrasovaa added this to PRs in progress in New GUI Startup via automation Jul 30, 2020

petrasovaa linked an issue Jul 30, 2020 that may be closed by this pull request

[Feat] Distinguish mapsets by ownership and lock in Data tab #714

Closed

lindakarlovska marked this pull request as draft July 30, 2020 12:45

lindakladivova added 3 commits July 30, 2020 07:47

wxGUI/datacatalog: added function which returns the mapset owner.

85bb5ad

wxGUI/datacatalog: Edits.

0bd553c

wxGUI/datacatalog: The attempt for creating OnGetItemTextColour funct…

4c722ff

…ion and broadening property function.

petrasovaa requested changes Jul 31, 2020

View reviewed changes

wxGUI/datacatalog: Now passed directly is_mapset_locked result while …

12985ac

…appending a mapset node in _reloadLocationNode, _reloadGrassDBNode, GetCurrentDbLocationMapsetNode. Returned directly wx.SYS colour.

lindakarlovska mentioned this pull request Aug 4, 2020

wxGUI: An action should be required before modifying other mapsets #848

Merged

wxGUI/datacatalog: Function get_mapset_owner moved to more suitable p…

55dcbc9

…lace. 'lock' and 'user' attribute are now defined only for mapsets. OnGetItemTextColour was corrected to return system colours.

lindakarlovska added the enhancement New feature or request label Aug 5, 2020

lindakarlovska self-assigned this Aug 5, 2020

lindakarlovska marked this pull request as ready for review August 5, 2020 11:54

petrasovaa requested changes Aug 7, 2020

View reviewed changes

lindakladivova added 2 commits August 8, 2020 05:07

wxGUI/datacatalog: is_mapset_users changed to is_different_mapset_own…

41d7418

…er, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetKeys.

wxGUI/datacatalog: is_mapset_users changed to is_different_mapset_own…

75b993f

…er, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetNodes.

wenzeslaus requested changes Aug 8, 2020

View reviewed changes

wenzeslaus mentioned this pull request Aug 9, 2020

Misleading lock symbol icon in the in the Data tab toolbar #870

Merged

lindakladivova and others added 3 commits August 9, 2020 03:08

Revert "wxGUI/datacatalog: is_mapset_users changed to is_different_ma…

2f91460

…pset_owner, InserMapset edited to insert also other keys, 'current' mapset key implemented with function UpdateMapsetNodes." This reverts commit 75b993f.

do not reload all mapset data, only lock and current; stylistic chang…

ee13597

…es, fix checks.py

Merge branch 'master' into wxGUI-distinguish-mapsets-by-ownership-and…

f49d234

…-lock-in-data-tab

petrasovaa self-requested a review August 11, 2020 16:28

petrasovaa approved these changes Aug 11, 2020

View reviewed changes

petrasovaa reviewed Aug 11, 2020

View reviewed changes

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved

Update gui/wxpython/datacatalog/tree.py

b980178

petrasovaa reviewed Aug 11, 2020

View reviewed changes

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved

petrasovaa reviewed Aug 11, 2020

View reviewed changes

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved

petrasovaa reviewed Aug 11, 2020

View reviewed changes

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved

petrasovaa reviewed Aug 11, 2020

View reviewed changes

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved

2 spaces between mapset name and paranthesis

26e451f

wenzeslaus approved these changes Aug 11, 2020

View reviewed changes

petrasovaa merged commit 2aab3b6 into OSGeo:master Aug 11, 2020

New GUI Startup automation moved this from PRs in progress to Done Aug 11, 2020

wenzeslaus mentioned this pull request Aug 18, 2020

Switch to another mapset when in use (aka force remove lock) #906

Merged

neteler added this to the 8.0.0 milestone Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish mapsets by ownership and lock in Data tab #849

Distinguish mapsets by ownership and lock in Data tab #849

lindakarlovska commented Jul 29, 2020 •

edited

Loading

petrasovaa Jul 29, 2020

petrasovaa Jul 29, 2020

lindakarlovska Jul 30, 2020

mlennert Aug 3, 2020

mlennert Aug 3, 2020

lindakarlovska commented Jul 31, 2020

petrasovaa Jul 31, 2020

lindakarlovska Aug 1, 2020

petrasovaa Jul 31, 2020

lindakarlovska Aug 1, 2020

mlennert commented Aug 3, 2020

lindakarlovska commented Aug 4, 2020

petrasovaa left a comment

petrasovaa Aug 7, 2020

lindakarlovska Aug 8, 2020

petrasovaa Aug 7, 2020

lindakarlovska Aug 8, 2020

petrasovaa Aug 7, 2020

lindakarlovska Aug 8, 2020

lindakarlovska commented Aug 8, 2020

wenzeslaus Aug 8, 2020

lindakarlovska Aug 9, 2020

wenzeslaus Aug 8, 2020

lindakarlovska Aug 9, 2020

wenzeslaus commented Aug 11, 2020

Distinguish mapsets by ownership and lock in Data tab #849

Distinguish mapsets by ownership and lock in Data tab #849

Conversation

lindakarlovska commented Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindakarlovska commented Jul 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlennert commented Aug 3, 2020

lindakarlovska commented Aug 4, 2020

petrasovaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindakarlovska commented Aug 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenzeslaus commented Aug 11, 2020

lindakarlovska commented Jul 29, 2020 •

edited

Loading