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: Move statusbar checkboxes related to Map Display to a new Map Display Settings dialog #2031

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Dec 23, 2021

This is the starting point of changes that are gonna lead to complete statusbar combobox removal.

I am creating the new Map Display Settings here, accessible from the map display toolbar and move there four statusbar checkbox items - autoRendering, AlignExtent, Resolution and ShowRegion. I have not moved the last checkbox related to the Projection shown in the status bar since it is more complex and needs the special PR.

Screenshot from 2022-01-02 08-57-03

For above-described purpose, a new file mapdisp/settings.py has been proposed. Here we can prepare checkbox items (and other needed widgets) and then fill the new Map Display settings dialog.

Please feel free to discuss!

Might be other things, also the form of Map Display settings is an important question, for more info please see #2017.

@lindakarlovska lindakarlovska added the GUI wxGUI related label Dec 23, 2021
@lindakarlovska lindakarlovska added this to the 8.2.0 milestone Dec 23, 2021
@lindakarlovska lindakarlovska added the enhancement New feature or request label Dec 23, 2021
pass

@property
def _propertyChanged(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a signal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is a signal, and is returned in the child property. But you are right that we do not need any mapFrame dependency. I have got rid of it.

class ChBItem:
"""Base class for checkbox settings items"""

def __init__(self, parent, mapframe):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, they shouldn't know about mapFrame and use signals or so.

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, I've incorporated giface..

def _property(self):
pass

@_property.setter
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably shouldn't be private when you provide a setter. Or remove the setter.

@lindakarlovska lindakarlovska force-pushed the wxGUI-move-status-bar-combobox-checkboxes-to-new-mapdisplay-settings branch from e9b316e to 93b117f Compare December 28, 2021 19:53
@lindakarlovska lindakarlovska marked this pull request as draft December 28, 2021 19:55
@lindakarlovska
Copy link
Contributor Author

@petrasovaa Save button and Set to default do not work so far, I am working on it. :-)

@lindakarlovska lindakarlovska marked this pull request as ready for review December 30, 2021 12:45
@lindakarlovska
Copy link
Contributor Author

We need to think carefully about the Map Display Settings icon. We could use some from QGIS:
e.g.
https://github.com/qgis/QGIS/blob/master/images/themes/default/mActionMapSettings.svg or
https://github.com/qgis/QGIS/blob/master/images/themes/default/mActionOptions.svg
or I would prefer to create the new one and apply the https://github.com/OSGeo/grass/blob/main/gui/icons/grass/map-settings.png settings theme on the monitor icon (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/monitor-create.png). Any opinions on that?

@petrasovaa
Copy link
Contributor

Overall, I expected fewer changes. It looks like the SbItem-derived classes could be minimally adjusted and just work. The Python properties are not useful here, maybe except mapWindowProperty, they just blow out the code. Also, using underscore means the attribute is private, so you are not supposed to access it from outside (so property doesn't make sense).

I would first keep the statusbar.py untouched, they are used in other standalone map displays.

@petrasovaa
Copy link
Contributor

We need to think carefully about the Map Display Settings icon. We could use some from QGIS: e.g. https://github.com/qgis/QGIS/blob/master/images/themes/default/mActionMapSettings.svg or https://github.com/qgis/QGIS/blob/master/images/themes/default/mActionOptions.svg or I would prefer to create the new one and apply the https://github.com/OSGeo/grass/blob/main/gui/icons/grass/map-settings.png settings theme on the monitor icon (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/monitor-create.png). Any opinions on that?

No strong opinion here, I am fine with the same icon, or perhaps the last solution you suggest.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 1, 2022

Overall, I expected fewer changes. It looks like the SbItem-derived classes could be minimally adjusted and just work. The Python properties are not useful here, maybe except mapWindowProperty, they just blow out the code. Also, using underscore means the attribute is private, so you are not supposed to access it from outside (so property doesn't make sense).

Ok, I will preserve just mapWindowProperty.

I would first keep the statusbar.py untouched, they are used in other standalone map displays.

Ou, you are right. I have not realized that. But I would keep the ChBRender in settings.py because it has the same form as other checkboxes and I think it is more clear to have it together with settings. We might also want to have the auto-render checkbox in Map Display settings.

@lindakarlovska
Copy link
Contributor Author

Overall, I expected fewer changes. It looks like the SbItem-derived classes could be minimally adjusted and just work. The Python properties are not useful here, maybe except mapWindowProperty, they just blow out the code. Also, using underscore means the attribute is private, so you are not supposed to access it from outside (so property doesn't make sense).

Ok, I will preserve just mapWindowProperty.

I would first keep the statusbar.py untouched, they are used in other standalone map displays.

Ou, you are right. I have not realized that. But I would keep the ChBRender in settings.py because it has the same form as other checkboxes and I think it is more clear to have it together with settings. We might also want to have the auto-render checkbox in Map Display settings.

Mmmm as I think about it, I let the statusbar.py entirely as it was - it is probably the best solution now even though we duplicate the code.

gui/wxpython/mapdisp/settings.py Outdated Show resolved Hide resolved
gui/wxpython/mapdisp/settings.py Outdated Show resolved Hide resolved
@petrasovaa
Copy link
Contributor

@petrasovaa Save button and Set to default do not work so far, I am working on it. :-)

I don't think that's what we want. It should behave the same way as the widgets in statusbar, so when you check it, it immediately changes the property. No save buttons there. Set to default may make sense but is perhaps not needed.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 6, 2022

We have only one button Close now, so the way how it works is the same as in the status bar - just local settings.
Výstřižek

@petrasovaa
Copy link
Contributor

The tooltips don't work because the checkbox is not a child of the staticbox, so either remove the staticbox or change the parent of the checkboxes. Perhaps the staticbox might not be needed here.

@lindakarlovska
Copy link
Contributor Author

The tooltips don't work because the checkbox is not a child of the staticbox, so either remove the staticbox or change the parent of the checkboxes. Perhaps the staticbox might not be needed here.

I would keep the Customize Map Display static box because there is the second static box below for Customize status bar with radio buttons planned. So, it would be better to change parent of the checkboxes.

@petrasovaa
Copy link
Contributor

The tooltips don't work because the checkbox is not a child of the staticbox, so either remove the staticbox or change the parent of the checkboxes. Perhaps the staticbox might not be needed here.

I would keep the Customize Map Display static box because there is the second static box below for Customize status bar with radio buttons planned. So, it would be better to change parent of the checkboxes.

So what are the tabs for then?

@lindakarlovska
Copy link
Contributor Author

@petrasovaa

The tooltips don't work because the checkbox is not a child of the staticbox, so either remove the staticbox or change the parent of the checkboxes. Perhaps the staticbox might not be needed here.

I would keep the Customize Map Display static box because there is the second static box below for Customize status bar with radio buttons planned. So, it would be better to change parent of the checkboxes.

So what are the tabs for then?

It is prepared for the Projection settings. You wrote: Regarding the projection, I would remove it from general settings altogether. So you had the notion that the Projection page will be entirely removed from the whole GRASS or just from GUI settings?

I had the feeling from our video call with you, Vashek and Martin that we plan to have a Projection page in Map Display settings (which sets a status bar CRS) and also mentions a global location coordinate system because it is something not visible anywhere and this place would be suitable. But maybe I understand it wrong.

Or maybe you thought that the Projection page in Map Display settings would be actually the Status bar page and we would have the radio buttons for status bar settings here and also the status bar projection. And maybe the global location projection could then be a part of the Map Display page. Probably this paragraph is a better option, or what did (do) you think?

@petrasovaa
Copy link
Contributor

Or maybe you thought that the Projection page in Map Display settings would be actually the Status bar page and we would have the radio buttons for status bar settings here and also the status bar projection

I din't think about it too much yet, but perhaps this would make most sense.

And maybe the global location projection could then be a part of the Map Display page.

Not quite sure what you mean here.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 6, 2022

Or maybe you thought that the Projection page in Map Display settings would be actually the Status bar page and we would have the radio buttons for status bar settings here and also the status bar projection

I din't think about it too much yet, but perhaps this would make most sense.

Good, then I will remove the static box widget and keep the notebook prepared for the Status bar page :-).

And maybe the global location projection could then be a part of the Map Display page.

Not quite sure what you mean here.

I mean that somebody set the CRS e.g. 5514 and then forgot what CRS he set. So, the Map Display settings dialog is, in my opinion, a good place to show it. Just as a non-editable text widget. There were also suggestions in the surveys that users would assume that they read the set CRS somewhere. They mentioned two ways - adding it in the context menu in Location tree mode (probably something like the Info context menu option) or directly as a part of the Location node text. But I think that for the beginning it would be also helpful to add it to Map Display settings.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 6, 2022

Now it looks as follows:
Výstřižek

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.

I would at this point remove the checkboxes from the map Display statusbar.

gui/wxpython/mapdisp/preferences.py Outdated Show resolved Hide resolved
gui/wxpython/mapdisp/preferences.py Outdated Show resolved Hide resolved
gui/wxpython/mapdisp/preferences.py Outdated Show resolved Hide resolved
@lindakarlovska lindakarlovska force-pushed the wxGUI-move-status-bar-combobox-checkboxes-to-new-mapdisplay-settings branch from 2176f19 to 8f9a4cb Compare January 9, 2022 18:34
@lindakarlovska
Copy link
Contributor Author

Now it looks as follows: Výstřižek

After changes, it looks exactly the same.

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.

Almost there! Maybe also make the dialog smaller, we can enlarge it once we need it.

@@ -0,0 +1,329 @@
"""
@package mapdisp.settings
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the settings here but the file is named preferences.py. I would perhaps go with settings here (not strong opinion), so please rename the class as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I changed it to preferences.py because in gui_core it is also called preferences.py. But anyway, settings.py could be anything (very general), I think preferences.py more points on the purpose. I would let it as it is, and change just the @Package name if it okey. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my point was to avoid preferences, because that's used for something else, this is not actually setting any preferences, you changing the actual behavior for specific mp display. Settings is used elsewhere, so that may not be the right choice either. How about rename this to properties.py and use property for naming of the classes, because that's the terminology we use for this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I think properties are better than preferences. Good idea! :-)

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 10, 2022

Almost there! Maybe also make the dialog smaller, we can enlarge it once we need it.

Yes :-)
Výstřižek

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 10, 2022

I have noticed one thing and have not succeeded to find the solution. When you open the dialog for the second time (or any other time) it is smaller (a bit shrank). And the self.GetBestSize() is different. Probably not too important but I am just trying to figure out why it is like that...

@petrasovaa
Copy link
Contributor

I have noticed one thing and have not succeeded to find the solution. When you open the dialog for the second time (or any other time) it is smaller (a bit shrank). And the self.GetBestSize() is different. Probably not too important but I am just trying to figure out why it is like that...

Hard to say, sizing is always a pain... You can try not using scrolled panel, that could simplify the problem little bit.

@lindakarlovska
Copy link
Contributor Author

I have noticed one thing and have not succeeded to find the solution. When you open the dialog for the second time (or any other time) it is smaller (a bit shrank). And the self.GetBestSize() is different. Probably not too important but I am just trying to figure out why it is like that...

Hard to say, sizing is always a pain... You can try not using scrolled panel, that could simplify the problem little bit.

It did the same but maybe worse because theres no scrolling so it is a bit cut off. Generally I think it is rather a detail and when I make a dialog a bit bigger in terms of the height I think nobody will notice. What do you think? I could set up the height of 250.

@petrasovaa petrasovaa merged commit 31421ee into OSGeo:main Jan 12, 2022
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
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 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

2 participants