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.

Sign up for GitHub

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

Jump to bottom

Test and fix QgsRectangle::isNull #53323

Closed
wants to merge 11 commits into from
Closed

Test and fix QgsRectangle::isNull #53323

wants to merge 11 commits into from

Conversation

strk
Copy link
Contributor

@strk strk commented Jun 1, 2023

Adds tests for QgsRectangle::isNull, improve toString to show the difference between Null and Empty, eventually fix the 0,0,0, rectangle case to still be Empty and NOT Null (to match documentation).

Superceeds #47404

Aims at closing #45563

@strk strk mentioned this pull request Jun 1, 2023
Closed
@github-actions github-actions bot added this to the 3.32.0 milestone Jun 1, 2023
@strk strk force-pushed the rectangle-null branch from 635bed5 to f61adf9 Compare June 5, 2023 09:04
strk added 10 commits June 13, 2023 11:40
@strk
Print Null rectangle as Null instead of Empty
ab9b7a7 
Update expected QgsRectangle::toString output on Null rectangle
@strk
Add test for QgsRectangle::isNull
5b7c00a
@strk
Stop considering 0,0,0,0 as a Null QgsRectangle
fc7db82
@strk
Fix TestQgsMultiPoint::boundingBox expectance on boundingbox of empty…
fbb1624 
… multipoint
@strk
Have QgsRectangle::snappedToGrid return a copy of self for Null rects
e9e5a91
@strk
Do not pass -projwin to gdal_translate when given rectangle is Null
941db5c
@strk
The extent of a source with no features is Empty, not Null
839aa25
@strk
Use arguments.extend instead of a set of arguments.append
0367701
@strk
Use members initialization instead of constructor initialization
dfb9ab3 
See https://github.com/qgis/QGIS/pull/53323/files#r1214757823
@strk
Have QgsRectangle::asPolygon return for null rects
636db78 
Update python test accordingly
@strk strk force-pushed the rectangle-null branch from 09726db to 636db78 Compare June 13, 2023 09:40
@github-actions
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 28, 2023
benoitdm-oslandia
benoitdm-oslandia reviewed Jun 28, 2023
View reviewed changes
src/core/geometry/qgsrectangle.h
double mYmin = 0.0;
double mXmax = 0.0;
double mYmax = 0.0;
double mXmin = std::numeric_limits<double>::max();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we not use std::numeric_limits::quiet_nan() instead of std::numeric_limits::max()?

It seems more logical to me.

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 29, 2023
@ptitjano ptitjano mentioned this pull request Jul 4, 2023
Merged
@github-actions
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 13, 2023
@lbartoletti lbartoletti removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 17, 2023
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 1, 2023
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Aug 9, 2023
@strk strk mentioned this pull request Sep 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault left review comments

@nicogodet nicogodet nicogodet left review comments

@benoitdm-oslandia benoitdm-oslandia benoitdm-oslandia left review comments

@lbartoletti lbartoletti lbartoletti approved these changes

Assignees
No one assigned
Labels
stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Milestone
3.32.0
Development

Successfully merging this pull request may close these issues.
5 participants
@strk @rouault @lbartoletti @nicogodet @benoitdm-oslandia