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

Add "Add Feature Collection" button, buildout menu tooling #231

Merged
merged 54 commits into from
Feb 24, 2025

Conversation

alukach
Copy link
Collaborator

@alukach alukach commented Feb 13, 2025

What I'm changing

This PR builds out a menu for the add_vector_layer() tooling that we currently have within the system:

def add_ee_vector_layer(
eeObject: ee.Element,
name: str,
shown: bool = True,
opacity: float = 1.0,
) -> QgsVectorLayer:
"""
Adds a vector layer properly by converting EE Geometry to a valid GeoJSON FeatureCollection.
"""
# Convert EE geometry into a proper GeoJSON FeatureCollection
geometry_info = eeObject.getInfo()
geojson = {
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"geometry": geometry_info,
"properties": {}, # Empty properties
}
],
}
# Write to a temporary file for QGIS
temp_file = tempfile.NamedTemporaryFile(delete=False, suffix=".geojson")
with open(temp_file.name, "w") as f:
json.dump(geojson, f)
# Use the temp file as the data source
uri = temp_file.name
# Create the vector layer
layer = QgsVectorLayer(uri, name, "ogr")
if not layer.isValid():
print(f"Failed to load vector layer: {name}")
else:
QgsProject.instance().addMapLayer(layer)
if shown is not None:
QgsProject.instance().layerTreeRoot().findLayer(
layer.id()
).setItemVisibilityChecked(shown)
if opacity is not None and layer.renderer():
symbol = layer.renderer().symbol()
symbol.setOpacity(opacity)
layer.triggerRepaint()
return layer

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.

@alukach alukach requested a review from zacdezgeo February 13, 2025 18:28
…function and updating UI elements to use it for localization.
@alukach alukach force-pushed the feature/menu-buildout branch from d137034 to 34d9236 Compare February 13, 2025 18:31
Copy link
Collaborator

@zacdezgeo zacdezgeo left a 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(
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically, have an "EE Tools" or something like at the top level and list the specific forms available under this sub-menu. In a similar way to how other menus work in QGIS:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image How does this look to you?

# Add actions to the menu
for action in [
ee_user_guide_action,
None,
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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 🤷

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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?

@alukach alukach force-pushed the feature/menu-buildout branch from 5123411 to 3e76ba0 Compare February 15, 2025 04:36
@zacdezgeo zacdezgeo mentioned this pull request Feb 16, 2025
@zacdezgeo zacdezgeo self-requested a review February 24, 2025 19:25
@alukach alukach force-pushed the feature/menu-buildout branch from 58d486e to 6036c7c Compare February 24, 2025 21:46
@alukach alukach force-pushed the feature/menu-buildout branch 2 times, most recently from 6a9bca3 to 7bf6634 Compare February 24, 2025 22:50
@alukach alukach merged commit 9196a2f into master Feb 24, 2025
4 checks passed
@alukach alukach deleted the feature/menu-buildout branch February 24, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants