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: An action should be required before modifying other mapsets #848

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Jul 29, 2020

While "allow editing" is OFF, no mapsets or location can be renamed or deleted. When ON we are allowed to rename and delete all mapsets and locations besides current ones.

…nctions adjusted to 'Allow renaming and deleting other mapsets or locations only when the allow editing mode is on.
@lindakarlovska
Copy link
Contributor Author

There is a problem after renaming location/mapset, the tree is not updated (only renamed but not highlighted in bold). It should be renamed and refreshed in the function _renameNode but it is not happening. Not sure how to correct it.

@petrasovaa petrasovaa added this to PRs in progress in New GUI Startup via automation Jul 29, 2020
@petrasovaa petrasovaa added gsoc Reserved for Google Summer of Code student(s) GUI wxGUI related labels Jul 29, 2020
@petrasovaa petrasovaa linked an issue Jul 29, 2020 that may be closed by this pull request
@petrasovaa
Copy link
Contributor

There is a problem after renaming location/mapset, the tree is not updated (only renamed but not highlighted in bold). It should be renamed and refreshed in the function _renameNode but it is not happening. Not sure how to correct it.

Could you be more specific, I don't seem to have the issue you describe, so maybe I don't test the exact thing you mention.

@lindakarlovska lindakarlovska marked this pull request as draft July 30, 2020 12:45
…ed to be able to handle conditions: current mapset and location cannot be renamed or deleted even if the self._restricted mode is False.
@lindakarlovska lindakarlovska marked this pull request as ready for review August 2, 2020 07:24
@mlennert
Copy link
Contributor

mlennert commented Aug 3, 2020

While "allow editing" is ON, no mapsets or location can be renamed or deleted. When OFF we are allowed to rename and delete all mapsets and locations besides current ones.

This sounds counterintuitive to me: if allow editing is "ON", it should be possible to rename and delete mapsets and locations, when it is "OFF" it shouldn't be.

De facto, this is what I see currently: when the lock is closed I cannot edit, when it is open I can. So probably just a question of formulation ?

@mlennert
Copy link
Contributor

mlennert commented Aug 3, 2020

There is a problem after renaming location/mapset, the tree is not updated (only renamed but not highlighted in bold). It should be renamed and refreshed in the function _renameNode but it is not happening. Not sure how to correct it.

It seems to me from just trying, that this is due to the fact that the highlighted node is defined by number, but that after renaming, the order to nodes can change because of alphabetical ordering. If you rename the node to something that doesn't change its position it remains the highlighted one, but otherwise it is the node that now has the same position number that is highlighted. So you would probably need a mechanism to find out which position the node is in after renaming and change the highlighting if needed.

@mlennert
Copy link
Contributor

mlennert commented Aug 3, 2020

Currently, I still can rename a mapset that I do not own. I cannot remember what we decided at the meeting. Should this be allowed ? Obviously, this works because I can also rename on disk, so GRASS GIS just reflects this...

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Aug 4, 2020

Currently, I still can rename a mapset that I do not own. I cannot remember what we decided at the meeting. Should this be allowed ? Obviously, this works because I can also rename on disk, so GRASS GIS just reflects this...

Yes, it should not be allowed, but this issue was not addressed so far because it is related to #801. And before that PR I need to complete #849.

@lindakarlovska
Copy link
Contributor Author

While "allow editing" is ON, no mapsets or location can be renamed or deleted. When OFF we are allowed to rename and delete all mapsets and locations besides current ones.

This sounds counterintuitive to me: if allow editing is "ON", it should be possible to rename and delete mapsets and locations, when it is "OFF" it shouldn't be.

De facto, this is what I see currently: when the lock is closed I cannot edit, when it is open I can. So probably just a question of formulation ?

Oops.. true. Corrected. :-)

@lindakarlovska
Copy link
Contributor Author

There is a problem after renaming location/mapset, the tree is not updated (only renamed but not highlighted in bold). It should be renamed and refreshed in the function _renameNode but it is not happening. Not sure how to correct it.

It seems to me from just trying, that this is due to the fact that the highlighted node is defined by number, but that after renaming, the order to nodes can change because of alphabetical ordering. If you rename the node to something that doesn't change its position it remains the highlighted one, but otherwise it is the node that now has the same position number that is highlighted. So you would probably need a mechanism to find out which position the node is in after renaming and change the highlighting if needed.

Moritz, I think it works fine. I had this problem while renaming or deleting current mapset or location but eventually we did not allow to do that. I think when renaming or deleting other mapsets or locations it works fine.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Aug 4, 2020

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

@mlennert
Copy link
Contributor

mlennert commented Aug 4, 2020

There is a problem after renaming location/mapset, the tree is not updated (only renamed but not highlighted in bold). It should be renamed and refreshed in the function _renameNode but it is not happening. Not sure how to correct it.

It seems to me from just trying, that this is due to the fact that the highlighted node is defined by number, but that after renaming, the order to nodes can change because of alphabetical ordering. If you rename the node to something that doesn't change its position it remains the highlighted one, but otherwise it is the node that now has the same position number that is highlighted. So you would probably need a mechanism to find out which position the node is in after renaming and change the highlighting if needed.

Moritz, I think it works fine. I had this problem while renaming or deleting current mapset or location but eventually we did not allow to do that. I think when renaming or deleting other mapsets or locations it works fine.

I could see the issue with nodes other than the currently used. E.g. renaming mapsets in a different location.

@mlennert
Copy link
Contributor

mlennert commented Aug 4, 2020

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge

And to create another PR with the multiple locations actions.

What still needs to be done on this PR ?

The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

@mlennert
Copy link
Contributor

mlennert commented Aug 4, 2020

The highlighting behavior seems to be independent from this PR anyway, so let's consider this as a different issue. See an example here:

simplescreenrecorder-2020-08-04_12 30 42

@lindakarlovska
Copy link
Contributor Author

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge

And to create another PR with the multiple locations actions.

What still needs to be done on this PR ?

The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

The highlighting behavior seems to be independent from this PR anyway, so let's consider this as a different issue. See an example here:

simplescreenrecorder-2020-08-04_12 30 42

Moritz,
your example works differently than mine. When the "Allow editing other mapsets" button is locked, you should not be able to edit mapsets and locations at all. (Options are grey and not clickable). It is strange that you can do that. In my version renaming and deleting when "Allow editing other mapsets" is locked do not work.
The second thing in your example is that after renaming, the current mapset is not changed. It means that the mapset which is highlighted is probably still in Location Belgique31370 and this behavior seems correct to me.

@lindakarlovska
Copy link
Contributor Author

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge
And to create another PR with the multiple locations actions.
What still needs to be done on this PR ?
The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

The highlighting behavior seems to be independent from this PR anyway, so let's consider this as a different issue. See an example here:
simplescreenrecorder-2020-08-04_12 30 42

Moritz,
your example works differently than mine. When the "Allow editing other mapsets" button is locked, you should not be able to edit mapsets and locations at all. (Options are grey and not clickable). It is strange that you can do that. In my version renaming and deleting when "Allow editing other mapsets" is locked do not work.
The second thing in your example is that after renaming, the current mapset is not changed. It means that the mapset which is highlighted is probably still in Location Belgique31370 and this behavior seems correct to me.

Oh. Sorry I get it. You meant the blue highlighting. :-) That is true. It does not work. But I share the same opinion that it belongs to another issue.

@lindakarlovska
Copy link
Contributor Author

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge

And to create another PR with the multiple locations actions.

What still needs to be done on this PR ?

The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

I think this PR is OK now. Ready to merge.

@mlennert
Copy link
Contributor

mlennert commented Aug 4, 2020

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge
And to create another PR with the multiple locations actions.
What still needs to be done on this PR ?
The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

The highlighting behavior seems to be independent from this PR anyway, so let's consider this as a different issue. See an example here:
simplescreenrecorder-2020-08-04_12 30 42

Moritz,
your example works differently than mine. When the "Allow editing other mapsets" button is locked, you should not be able to edit mapsets and locations at all. (Options are grey and not clickable). It is strange that you can do that. In my version renaming and deleting when "Allow editing other mapsets" is locked do not work.

Yes, that is why I said that this issue is independent of this PR: my example was done without this PR to show that the issue was already there. Without this PR, the lock icon didn't have any effect.

@mlennert
Copy link
Contributor

mlennert commented Aug 4, 2020

Within this PR I made controls whether mapset/location is current or not (and it is prepared for gislock and ownership controls which will be implemented in the future). And I have one idea which could be also made within this PR => "Deleting multiple locations" paradigm.
Moritz, what do you think about it? It is not much work because function delete_location_interactively is now in this PR implemented in the very same way as delete_mapsets_interactively.
I could establish the different PR but I need changes made in this PR...

I think it is always better to follow the KISS principle and make each pull request do one thing only. So I can think it would be better to finish this one and merge
And to create another PR with the multiple locations actions.
What still needs to be done on this PR ?
The issue with the highlighting is a nuisance but not a blocker. So, one option would be to merge this PR soon, but to file an issue about the highlight (unless it is only a local problem for me).

I think this PR is OK now. Ready to merge.

Ok, I will merge. We can always open issues if they come up.

@mlennert mlennert merged commit 94f7353 into OSGeo:master Aug 4, 2020
New GUI Startup automation moved this from PRs in progress to Done Aug 4, 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] An action should be required before modifying other mapsets
4 participants