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 relationships between tables #1756

Merged
merged 8 commits into from
Aug 27, 2024
Merged

Add relationships between tables #1756

merged 8 commits into from
Aug 27, 2024

Conversation

evetion
Copy link
Member

@evetion evetion commented Aug 25, 2024

Fixes #1648
Fixes #1755 (by updating the edge style)
Fixes #1688 (node_id has to be unique)

Also sets the node_id and edge_id to be unique, so they are autogenerated in the edit forms.

The current changes also will make the snapping + edge creation based on coordinates obsolete, as the form now looks like this:

Screenshot 2024-08-25 at 13 45 11

There are buttons to open a form with the Node, a highlighter to blink the node on the map, and a map selection panel, where one can click the desired node.

@evetion evetion requested a review from visr August 25, 2024 12:40
Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

This will make large model analysis so much easier!

The current changes also will make the snapping + edge creation based on coordinates obsolete

The code for this is still there, with self.edge_layer.editingStopped.connect(self.connect_nodes). How do you mean obsolete? That people will get geometries from IDs rather than IDs from snapped geometries? But the geometries are not automatically created right? If I fill in just IDs in the windows from your screenshot, I get ValueError: Null geometry cannot be converted to a polyline..

This will need some updates of https://ribasim.org/tutorial/qgis.html and the test plan #1691 as well..

@evetion
Copy link
Member Author

evetion commented Aug 27, 2024

That people will get geometries from IDs rather than IDs from snapped geometries?

Mostly that we will get the IDs from the user, and not from the snapped geometries. We need to think about that workflow. Geometry creation is still needed, but maybe snap the first and last points to the chosen node_ids in the form?

Anyway, that discussion is not blocking for this PR. I will update the documentation though.

def set_unique(self, name: str) -> None:
layer = self.layer
index = layer.fields().indexFromName(name)
setup = QgsEditorWidgetSetup(
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to keep a separation between Qgis things (in the widgets folder), and core things that can be unit tested. It feels like this is being compromised with this new code as codecode also tells us there are no unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I merely copied the existing structure here, the method above also uses an QgsEditorWidgetSetup. That said, I agree that the QGIS plugin can use some spring cleaning to separate concerns and document the architecture, as its a bit maddening to work with. Then again, I don't expect that writing C++ code in Python will get more fun.

Also, codecode?

rel.setReferencingLayer(from_layer.id())
rel.setReferencedLayer(to_layer_id)
rel.setName(name)
rel.setStrength(rel.RelationStrength.Composition) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type ignored in this case? Version mismatch between the stubs?

Copy link
Member Author

Choose a reason for hiding this comment

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

The stubs do not seem to be aware of enums linked in a class.

docs/guide/qgis.qmd Outdated Show resolved Hide resolved
docs/guide/qgis.qmd Outdated Show resolved Hide resolved
@visr visr merged commit 6733e79 into main Aug 27, 2024
20 of 21 checks passed
@visr visr deleted the feat/qgis-relations branch August 27, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants