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

[project] Avoid needlessly dirtying when written value does not change #8470

Merged
merged 1 commit into from Nov 14, 2018

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Nov 13, 2018

Description

@nyalldawson , this PR avoids needlessly dirtying project when exporting a layout by checking if the export path is modified instead of always re-writing the last export path into the project.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@nirvn nirvn changed the title [ui] Avoid needlessly dirtying project when exporting layouts [project] Avoid needlessly dirtying when written value does not change Nov 13, 2018
@nirvn
Copy link
Contributor Author

nirvn commented Nov 13, 2018

Upon advise from @nyalldawson , I went for a lower level solution, which avoids needless dirtying of project in other parts of QGIS (for e.g., for an impractical example: opening and closing a decoration dialog won't dirty the project if none of the settings are modified).

@nirvn
Copy link
Contributor Author

nirvn commented Nov 13, 2018

Note: it also improves the way isDirtyChanged is emitted, whereas before it was a/ emitted prior to the entry was written, and b/ emitted irrespective of whether the entry was actually written.

@nyalldawson
Copy link
Collaborator

@nirvn

Upon advise from @nyalldawson , I went for a lower level solution, which avoids needless dirtying of project in other parts of QGIS

...and that's why you make the big bucks ;)

@nirvn nirvn force-pushed the designerpath branch 2 times, most recently from c1995e8 to f8c4ee2 Compare November 13, 2018 06:10
@nirvn
Copy link
Contributor Author

nirvn commented Nov 13, 2018

@nyalldawson , OK to merge (and backport) upon travis turning green?

@nyalldawson
Copy link
Collaborator

Better leave it a day or so for wider feedback, QgsProject is critical stuff

@nirvn
Copy link
Contributor Author

nirvn commented Nov 13, 2018

@wonder-sk , @m-kuhn , your review would be appreciated here.

*/
QgsProjectProperty *addKey_( const QString &scope,
const QString &key,
QgsProjectPropertyKey *rootProperty,
const QVariant &value )
const QVariant &value,
bool &propertiesModified )
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use directly setDirty inside addKey_ when necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luipir , because we'd have to pass on a QgsProject pointer to do that, which doesn't seem to be a good idea to begin with :)

Copy link
Contributor

Choose a reason for hiding this comment

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

has ok, I supposed addKey_ was a private QgsProject member, sorry for the noise

@wonder-sk
Copy link
Member

Looks good to me

@nirvn nirvn merged commit 3b892c9 into qgis:master Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants