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

Do not center to geometry when highlighting when geometry does not fit the screen #3665

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ValentinBuira
Copy link
Contributor

This PR fix the ux-unfriendly behavior when in some scenario (concave shaped, or too big) the user would highlight a feature and the extend would either not move to the feature or move quite a lot.

The new behaviors works by not jumping to the feature if the feature can not be fully contained within the current extend size

Example both jumping and not jumping in the same video:
Screencast from 2024-10-25 11-46-47.webm

Fix #3559

Copy link

github-actions bot commented Oct 25, 2024

Pull Request Test Coverage Report for Build 11556198782

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 340 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 60.431%

Files with Coverage Reduction New Missed Lines %
input/app/inpututils.cpp 340 51.19%
Totals Coverage Status
Change from base Build 11500665800: -0.03%
Covered Lines: 7885
Relevant Lines: 13048

💛 - Coveralls

@ValentinBuira ValentinBuira changed the title Do not center to geometry if feature is outside of screen bounds Do not center to geometry when highlighting when geometry does not fit the screen Oct 25, 2024
Copy link
Contributor

@VitorVieiraZ VitorVieiraZ left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have tested it, and it works just fine. There are just some code-related details left to cover in the review.

app/inpututils.cpp Outdated Show resolved Hide resolved
app/qml/map/MMMapController.qml Outdated Show resolved Hide resolved
@ValentinBuira
Copy link
Contributor Author

There is one things left that bother me, it's that in some cases you, since we don't offset the view, you can hide the highlighted feature with the pop-up, see the attached video

Screencast.from.2024-10-28.15-34-54.webm

I am working on a fix but I can do it in a separate PR

@@ -316,6 +316,25 @@ QPointF InputUtils::geometryCenterToScreenCoordinates( const QgsGeometry &geom,
return screenPoint;
}

bool InputUtils::canExtentContainGeometry( const QgsGeometry &geom, InputMapSettings *mapSettings )
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to rename canExtentContainGeometry to extentContainGeometry

* Returns the true if the geometry could fully be contained in the current screen otherwise false
* Geometry must be in canvas CRS
*/
Q_INVOKABLE bool canExtentContainGeometry( const QgsGeometry &geom, InputMapSettings *mapSettings );
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe worth to have simple unit test ?

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.

Do not center to geometry if feature is outside of screen bounds
4 participants