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: easier switching between locations (GRASS GIS 8) #1598

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented May 27, 2021

When switching to a map layer in a different location this message will appear:

Map layer in different location_004

…to display a layer only if switching is successful.
@lindakarlovska
Copy link
Contributor Author

When switching to a map layer in a different location this message will appears:

Map layer in different location_004

I am not sure about the formulation, maybe the button should named "Stay in current location"?
Please, if you have other ideas how to formulate text, go ahead :-)

Map layer in a different location_007

@lindakarlovska lindakarlovska added GUI wxGUI related enhancement New feature or request labels May 27, 2021
@lindakarlovska lindakarlovska added this to the 8.0.0 milestone May 27, 2021
Copy link
Member

@neteler neteler left a comment

Choose a reason for hiding this comment

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

This is a great solution! Thanks so much @lindakladivova

"To display this map switch to mapset <{1}> first."
"Map <{0}@{1}> is not in the current location. "
"Do you want to switch to the location '{2}'? "
"Your currently displayed layers will disappear."
Copy link
Member

Choose a reason for hiding this comment

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

The reason I used the word "disappear" in the original issue is that it expresses well the confusion a user may have when the layers are unloaded, i.e., something unexpected happened. Furthermore, "disappear" in this context may sound as "underlying data will be deleted". I don't have another suggestion at this point, but Perhaps mentioning again the reason for it in connection with the change in wording can bring some understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something along the lines "All current Map Displays will be closed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And put it on the next line? Now it looks as follows: switch

@mlennert
Copy link
Contributor

mlennert commented May 28, 2021 via email

@@ -1838,30 +1860,30 @@ def SwitchMapset(self, grassdb, location, mapset, show_confirmation=False):
"""
Switch to location and mapset interactively.
"""
if can_switch_mapset_interactive(self, grassdb, location, mapset):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for moving this if out of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that we display a layer only if a switching is successful. I had another idea how to solve this issue - make a optional argument display_layer for SwitchMapset method. But finally I realized that SwitchMapset is something not much related to displaying layer.
Now code also seems to me a bit clearer because SwitchMapset really switch in all cases.
Making a display_layer argument would be much shorter in code but I think not much meaningful in terms of logic.
Or we could return True or False from SwitchMapset but I am not sure If this function should return something, It is more of procedure. What do you think @petrasovaa ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I would be in favor of returning true/false since we need to use can_switch_mapset_interactive anyway every time we want to switch it, so it may be better have it in the SwitchMapset method.

gui/wxpython/datacatalog/tree.py Outdated Show resolved Hide resolved
"To display this map switch to mapset <{1}> first."
"Map <{0}@{1}> is not in the current location. "
"Do you want to switch to the location '{2}'? \n\n"
"All current Map Displays will be closed."
Copy link
Member

Choose a reason for hiding this comment

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

This looks like "will be closed in any case". I think the new paragraph needs to be between "...the current location" and "All current Map...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then I would suggest this formulation:
easier_switch

Copy link
Contributor

@veroandreo veroandreo May 31, 2021

Choose a reason for hiding this comment

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

That message sounds a bit intimidating to me... Can we say something like:

Map <bla> is not in the current location. To be able to display it you need to switch to <blabla> location. 
Note that if you switch there all current Map Displays will be closed. 

Do you want to switch anyway?

Too long? My intention is to "explain" in short that to display a map from a different location the user needs to switch there.

Copy link
Contributor Author

@lindakarlovska lindakarlovska May 31, 2021

Choose a reason for hiding this comment

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

I like your idea more Vero. It is true that it is quite long but we need to fully explain it. @petrasovaa, @wenzeslaus do you agree with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That message sounds a bit intimidating to me... Can we say something like:

Map <bla> is not in the current location. To be able to display it you need to switch to <blabla> location. 
Note that if you switch there all current Map Displays will be closed. 

Do you want to switch anyway?

Too long? My intention is to "explain" in short that to display a map from a different location the user needs to switch there.

I think @veroandreo 's formulation is a bit more appealing. However, I think the button labels are too wordy. What about Cancel (as default) and Switch?

Copy link
Contributor Author

@lindakarlovska lindakarlovska May 31, 2021

Choose a reason for hiding this comment

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

After Nilason's and Vero's notes :-) :
switch

@petrasovaa petrasovaa merged commit d566c37 into OSGeo:master May 31, 2021
@lindakarlovska lindakarlovska deleted the wxGUI-easier-switching-between-locations branch June 1, 2021 16:09
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request GUI wxGUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants