-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add "Add Feature Collection" button, buildout menu tooling #231
Conversation
…function and updating UI elements to use it for localization.
d137034
to
34d9236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very little changes to push this through. Mostly flagged items to be addressed in future refactor. Perhaps just the commented buttons to be removed?
@@ -112,36 +112,54 @@ def initGui(self): | |||
parent=self.iface.mainWindow(), | |||
triggered=self._run_cmd_set_cloud_project, | |||
) | |||
add_fc_button = QtWidgets.QAction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm wondering if when we should implement the sub-menus to avoid having a long list of functions at the top level. We can punt beyond this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if when we should implement the sub-menus to ...
Not sure what you're saying here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ee_plugin/ee_plugin.py
Outdated
# Add actions to the menu | ||
for action in [ | ||
ee_user_guide_action, | ||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is none used for some whitespace or something? Wondering if we should extract this to another function that potentially takes a list of actions, and figures out the formatting. With the sub-menu logic, we could also maybe use a dictionary that maps to the sub-menu/level desired? Again, we can refactor after this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is something that I was unsure about. If you look below, you'll see:
if action:
menu.addAction(action)
else:
menu.addSeparator()
So basically, None
represents a separator. That feels like an awkward convention but wasn't sure what would be better. I also am unsure that we want to keep the plugin menu and toolbar menu completely in sync as we are doing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 6036c7c, I abstracted how the menu is setup.
ee_plugin/ui/forms.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm wondering if we would like to have a single file per form to avoid them being to clustered in the future. Is listing all these files under the ui
folder a good idea? That's the approach I took in #229.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also be under a new python package called forms in terms of project structure 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think separating the forms and possibly their handler into separate files seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zacdezgeo what do you think about 864a1e5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! Probably a nit, but can we add the form
prefix to any form to help differentiate from the other UI files and have the forms listed next to each other when we add more?
This commit attempts to establish a new pattern where every form is written as a separate module in ee_plugin.ui with a form and callback function.
5123411
to
3e76ba0
Compare
58d486e
to
6036c7c
Compare
6a9bca3
to
7bf6634
Compare
What I'm changing
This PR builds out a menu for the
add_vector_layer()
tooling that we currently have within the system:qgis-earthengine-plugin/ee_plugin/utils.py
Lines 202 to 248 in 08e7bb8
How I did it
While adding a menu for this one function, I experimented with several techniques for creating UIs within QGIS. This included using QtDesigner to generate a
.ui
file, which could be parsed in Python or converted to Python. Ultimately, I elected not to go with either of these methods, as the.ui
files would likely prove a challenge when attempting to build & maintain cohesively styled UIs within the plugin, and the Python conversions were very crudely constructed, which looked as if they would make for a maintenance challenge. Instead, I opted to build out some helper functions that could be used to build out consistent UI patterns that would be easily read & maintained.