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

[FEATURE] [Identify tool] Present referencing relations (in addition … #57863

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Jun 24, 2024

…to referenced ones), and support them on arbitrary nesting level

Fixes #48776

Previously only referenced relations where presented when exploring a feature, now referenced ones are presented as well. Do that only when the user explicit unfolds a node, to avoid potential 'explosion' of the number of features in the tree in case of a huge database where all features would be connected through relations. If through relations a feature already present in an ancestor node is detected, it is omitted from the related features.

image

Add setting parameters to disable showing relations

image

Add a contextual menu item "Identify Feature" to be able to "re-center" the result of the identification tree on a nested feature, to limit the nesting depth.

image
==>

image

…to referenced ones), and support them on arbitrary nesting level

Fixes qgis#48776

Previously only referenced relations where presented when exploring a feature,
now referenced ones are presented as well. Do that only when the user
explicit unfolds a node, to avoid potential 'explosion' of the number of
features in the tree in case of a huge database where all features would
be connected through relations. If through relations a feature already
present in an ancestor node is detected, it is omitted from the related
features.

Add setting parameters to disable showing referenced and referencing
relations.

Add a contextual menu item "Explore Feature" to be able to "re-center"
the result of the identification tree on a nested feature, to limit the
nesting depth.
@github-actions github-actions bot added this to the 3.40.0 milestone Jun 24, 2024
Copy link

github-actions bot commented Jun 24, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 76a0c7f)

QFont italicFont;
italicFont.setItalic( true );
relationItem->setFont( 0, italicFont );
relationItem->setData( 0, Qt::UserRole, QVariant::fromValue( qobject_cast<QObject *>( relation.referencedLayer() ) ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks dangerous -- do we do this elsewhere? I think it'd be safer to use the layer id here instead . Otherwise there could be a crash if the child layer is removed before the relation is expanded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks dangerous -- do we do this elsewhere?

yes, for example

layItem->setData( 0, Qt::UserRole, QVariant::fromValue( qobject_cast<QObject *>( vlayer ) ) );
. But they also connected to QgsIdentifyResultsDialog::layerDestroyed() to remove nodes from the tree, which was missing for the relations. I've added this connection and enhanced layerDestroyed() to remove items that are linked to a layer, but not necessarily as top items.

<bool>true</bool>
</property>
<property name="text">
<string>Show referenced relations</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<string>Show referenced relations</string>
<string>Show Referenced Relations</string>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and merged as a single setting as suggested

src/ui/qgsidentifyresultsbase.ui Outdated Show resolved Hide resolved
@@ -0,0 +1,270 @@
/***************************************************************************
testqgsidentifyresultsdialog.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's other tests for the dialog in the identify map tool tests. Maybe we should put these new ones in there, and then rename to just testqgsidentify.cpp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nyalldawson
Copy link
Collaborator

I also have some ui suggestions:

Add setting parameters to disable showing referenced and referencing relations.

What about having just a single action for "show relations", which applies to both? I'm worried about UI complexity. Honestly a decade later of using relations in qgis and I'm STILL confused by the referencing/referenced terminology 😝

Add a contextual menu item "Explore Feature" to be able to "re-center" the result of the identification tree on a nested feature, to limit the nesting depth.

How about "Identify Feature" instead? We don't use "explore" anywhere else, so I'm concerned this action won't be immediately understandable.

@nyalldawson nyalldawson added Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Jun 24, 2024
@qgis-bot
Copy link
Collaborator

@rouault
This pull request has been tagged as requiring documentation.

A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged.

Please update the description (not the comments) with helpful description and screenshot to help the work from documentors.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

@qgis-bot
Copy link
Collaborator

@rouault

This pull request has been tagged for the changelog.

  • The description will be harvested so please provide a "nearly-ready" text for the final changelog
  • If possible, add a nice illustration of the feature. Only the first one in the description will be harvested (GIF accepted as well)
  • If you can, it's better to give credits to your sponsor, see below for different formats.

You can edit the description.

Format available for credits
  • Funded by NAME
  • Funded by URL
  • Funded by NAME URL
  • Sponsored by NAME
  • Sponsored by URL
  • Sponsored by NAME URL

Thank you!

@rouault
Copy link
Contributor Author

rouault commented Jun 24, 2024

What about having just a single action for "show relations", which applies to both? I

done

I'm STILL confused by the referencing/referenced terminology 😝

This both reassures and worries me as I'm also deeply confused :-)

How about "Identify Feature" instead? We don't use "explore" anywhere else, so I'm concerned this action won't be immediately understandable.

done

@nyalldawson nyalldawson merged commit 02aa7af into qgis:master Jun 25, 2024
28 checks passed
@qgis-bot
Copy link
Collaborator

@rouault
A documentation ticket has been opened at qgis/QGIS-Documentation#9148
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog Items that are queued to appear in the visual changelog - remove after harvesting Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

many to many relations using 3rd join table not allowing expansion in identify tool
3 participants