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: Use Map Display settings in other Map Display applications #2155

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Feb 3, 2022

This PR prepares for Map Display statusbar refactoring. It removes some statusbar widgets that we do not need to refactor since they are already used in Map Display settings, particularly:

  • statusbar::SbShowRegion
  • statusbar::SbAlignExtent
  • statusbar::SbResolution
  • statusbar::SbProjection

Subsequently, it makes Map Display settings button part of other Map Display panels such as MapSwipe, IClass, Image2Target, Photo2Target and GCP.

@lindakarlovska lindakarlovska marked this pull request as draft February 3, 2022 15:58
@lindakarlovska lindakarlovska added enhancement New feature or request GUI wxGUI related labels Feb 3, 2022
@lindakarlovska lindakarlovska added this to the 8.2.0 milestone Feb 3, 2022
Comment on lines 174 to 186
def GetProperty(self, name):
"""Returns property"""
if self.HasProperty("projection"):
return self.statusbarManager.GetProperty(name)
else:
if not self.HasProperty("projection"):
return self.mapWindowProperties.useDefinedProjection
elif not self.HasProperty("resolution"):
return self.mapWindowProperties.resolution
elif not self.HasProperty("alignExtent"):
return self.mapWindowProperties.alignExtent
elif not self.HasProperty("region"):
return self.mapWindowProperties.region
else:
print("bla")
return self.statusbarManager.GetProperty(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't makes sense, I think I missed this during review of #2087. You call e.g. GetProperty("region") and you get useDefinedProjection. See SetProperty above as well.

Copy link
Contributor Author

@lindakarlovska lindakarlovska Feb 7, 2022

Choose a reason for hiding this comment

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

Yes, that's true, thank you @petrasovaa . I was aware of the fact that there is probably a mistake because it crashed but did not have enough time to look at it more precisely. I will do so now. You are right that the problem has already started in #2087.

if self.HasProperty("projection"):
self.statusbarManager.SetProperty(name, value)
else:
if name == "projection":
Copy link
Contributor Author

@lindakarlovska lindakarlovska Feb 7, 2022

Choose a reason for hiding this comment

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

These GetProperty and SetProperty are temporary solutions since we need self.mapFrane.GetProperty("projection") in some statusbar classes such as SbGoTo and so on...

@lindakarlovska lindakarlovska marked this pull request as ready for review February 7, 2022 12:05
@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Feb 7, 2022

I know that we talked on videocall about the "monitor" background (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/monitor-create.png) that would be more suitable for Map Display settings than the "map" background (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/map-settings.png). However, if we use the monitor icon for Map Display Settings we probably need to make it from beginning in Inkscape.. I remember that we took some icons from QGIS but if I looked correctly QGIS does not have any monitor svg icon like that ... So I would probably put it off for another PR where we can also create a new vector icon for "Render map" and add create-monitor.svg to create-monitor.png file. @petrasovaa what do you think?

@petrasovaa
Copy link
Contributor

I know that we talked on videocall about the "monitor" background (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/monitor-create.png) that would be more suitable for Map Display settings than the "map" background (https://github.com/OSGeo/grass/blob/main/gui/icons/grass/map-settings.png). However, if we use the monitor icon for Map Display Settings we probably need to make it from beginning in Inkscape.. I remember that we took some icons from QGIS but if I looked correctly QGIS does not have any monitor svg icon like that ... So I would probably put it off for another PR where we can also create a new vector icon for "Render map" and add create-monitor.svg to create-monitor.png file. @petrasovaa what do you think?

Yes, I assumed you would need to create a new one. Unfortunately I found the monitor one only as png:
https://github.com/qgis/QGIS/blob/master/images/themes/default/propertyicons/settings.svg
https://github.com/Cracert/GIS-icons/blob/master/24x24/monitor.png
Separate PR is good.

gui/wxpython/iclass/frame.py Outdated Show resolved Hide resolved
Comment on lines 176 to 186
if name == "projection":
return self.mapWindowProperties.useDefinedProjection
elif name == "resolution":
return self.mapWindowProperties.resolution
elif name == "alignExtent":
return self.mapWindowProperties.alignExtent
elif name == "region":
return self.mapWindowProperties.showRegion
else:
return self.statusbarManager.GetProperty(name)

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use hasattr/getattr here instead of those ifs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried that in the last commit but a bug with useDefinedProjection apparently appeared. The "UseDefinedProjection" toggle button does not work. Now I am going to bed, I will look at it more deeply tomorrow.

@lindakarlovska
Copy link
Contributor Author

I have tested all map display apps and everything works good. I think we could merge it.

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.

2 participants