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 gh41477 editbuffer passthrough #41539

Merged
merged 4 commits into from
Feb 24, 2021

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Feb 13, 2021

This started as a fix for #41477 but ended up with quite a refactoring.

The basic idea is to make the pass through buffer that we use in transactions to behave like the standard buffer from an API point of view.

I wrote a new test that covers all the cases and expects the same results for transactional and standard buffers:

  • add/remove features
  • add/remove fields
  • change geometry
  • change attribute

The test also checks the return values of QgsVectorLayerEditBuffer:

  • changedAttributeValues
  • deletedAttributeIds
  • addedAttributes
  • changedGeometries
  • deletedFeatureIds
  • addedFeatures

@elpaso elpaso added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Feb 13, 2021
@github-actions github-actions bot added this to the 3.18.0 milestone Feb 13, 2021
@nyalldawson
Copy link
Collaborator

Shall we defer to post 3.18.0?

@elpaso
Copy link
Contributor Author

elpaso commented Feb 14, 2021

Shall we defer to post 3.18.0?

Definitely. Some of the changes are rather risky.

@elpaso elpaso added the Merge After Thaw For approved PRs which are ready to be merged as soon as master is thawed label Feb 14, 2021
@nyalldawson nyalldawson added the Frozen Feature freeze - Do not merge! label Feb 16, 2021
@nyalldawson nyalldawson modified the milestones: 3.18.0, 3.20 (Feature) Feb 19, 2021
@elpaso
Copy link
Contributor Author

elpaso commented Feb 19, 2021

@nyalldawson I don't think this qualifies as a feature, it is a bugfix. I agree it is safer to wait a cycle but IMO it should eventually land in 3.18.

@nyalldawson nyalldawson added backport release-3_18 and removed Frozen Feature freeze - Do not merge! Merge After Thaw For approved PRs which are ready to be merged as soon as master is thawed labels Feb 19, 2021
@nyalldawson
Copy link
Collaborator

I don't think this qualifies as a feature, it is a bugfix.

Agreed. The "3.20 (feature)" milestone just means the feature freeze date for 3.20. maybe I should rename this as "3.20 (feature freeze)" to clarify?

@m-kuhn
Copy link
Member

m-kuhn commented Feb 24, 2021

Should we merge this and delay 3.18 backport by one cycle to 3.18.2? Or is this safe enough to ship with 3.18.1 already?

@elpaso
Copy link
Contributor Author

elpaso commented Feb 24, 2021

Should we merge this and delay 3.18 backport by one cycle to 3.18.2? Or is this safe enough to ship with 3.18.1 already?

I'm not really sure: the test coverage is decent but there are some changes in the transactions code that make me scary. I suspect that the underlying issues never surfaced because the client code in app wraps all commands into macros and does not perform atomic redo/undo.

I think it's safer if we proceed as you suggest.

@m-kuhn m-kuhn merged commit b3dae9a into qgis:master Feb 24, 2021
@elpaso elpaso deleted the bugfix-gh41477-editbuffer-passthrough branch February 24, 2021 13:44
github-actions bot pushed a commit that referenced this pull request Feb 24, 2021
…hrough

Bugfix gh41477 editbuffer passthrough
elpaso pushed a commit to elpaso/QGIS that referenced this pull request Apr 6, 2021
…assthrough

Bugfix gh41477 editbuffer passthrough
elpaso added a commit to elpaso/QGIS that referenced this pull request Apr 7, 2021
- fix signal not emitted
- fix wrong value emitted in signal
- remove layer clone

The schema editing + rollback is still broken because
fields are not restored to the original state after
the rollback (at least for GPKG).

Followup to qgis#41539
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants