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

[Bugfix] North arrow / image should account for item rotation too. #33540

Merged
merged 21 commits into from
Dec 31, 2019

Conversation

roya0045
Copy link
Contributor

@roya0045 roya0045 commented Dec 28, 2019

Description

This is my proposal to fix #33482 .

The changes implemented are the following:

Override the rotateitem method in qgslayoutitemmap to add the rotation signal.

Change the mapRotationChanged signals by adding the rotation value of the item.

Backport for 3.10 is suggested.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

@roya0045
Copy link
Contributor Author

PR might not be ready for review, haven't tested, just made it so I don't forget when I get back to work.

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one!

@@ -447,7 +447,7 @@ void QgsLayoutItemMap::setMapRotation( double rotation )
mMapRotation = rotation;
mEvaluatedMapRotation = mMapRotation;
invalidateCache();
emit mapRotationChanged( rotation );
emit mapRotationChanged( rotation + itemRotation() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

The mapRotationChanged signal is used quite heavily in other areas of code outside of the north arrow, so I'm reluctant to change it's current behavior here. I'd suggest instead hooking into the existing QgsLayoutItem::rotationChanged signal, i.e. by making the picture item listen out for both the mapRotationChanged AND rotationChanged signals (i.e. adding new connections to rotationChanged wherever QgsLayoutItemPicture currently uses mapRotationChanged )

Also note that you'll need to update QgsLayoutItemPicture::updateMapRotation() -- it doesn't use the rotation value from the mapRotationChanged signal in any case, so it needs to be updated to base the picture orientation on mRotationMap->mapRotation() + mRotationMap->rotation().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused as to why the value provided was useless, I saw the function and thought it was overloaded somewhere else to accept the given value and couldn't find that overloaded method. + the holiday lack of sleep doesn't help. I was debating simply changing the function at first but didn't want to alter some core component if it could be prevented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not useless, it's just not used by the north arrow code. Check the QgsLayoutItemPicture::updateMapRotation() slot, it doesn't have an argument accepting the rotation value used in the signal.

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Nearly there!

src/core/layout/qgslayoutitemmap.cpp Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator

There's also one connection missed in QgsLayoutItemPicture::finalizeRestoreFromXml()

@nyalldawson nyalldawson added backport release-3_10 Squash! Remember to squash this PR, instead of merging or rebasing Bug Either a bug report, or a bug fix. Let's hope for the latter! labels Dec 30, 2019
@nyalldawson nyalldawson added the Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks label Dec 30, 2019
@nyalldawson nyalldawson merged commit 1bf7e72 into qgis:master Dec 31, 2019
nyalldawson added a commit to nyalldawson/QGIS that referenced this pull request Dec 31, 2019
nyalldawson added a commit that referenced this pull request Dec 31, 2019
qgis-bot pushed a commit to qgis-bot/QGIS that referenced this pull request Dec 31, 2019
nyalldawson pushed a commit that referenced this pull request Dec 31, 2019
@roya0045 roya0045 deleted the NA-fix branch December 31, 2019 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks Squash! Remember to squash this PR, instead of merging or rebasing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

North Arrow - bug when rotating Map in composer
2 participants