Skip to content

Expose setAllowCommit to Python (and other functions)#62951

Open
sewe2000 wants to merge 30 commits intoqgis:masterfrom
sewe2000:bug_18101-python-validation-bindings
Open

Expose setAllowCommit to Python (and other functions)#62951
sewe2000 wants to merge 30 commits intoqgis:masterfrom
sewe2000:bug_18101-python-validation-bindings

Conversation

@sewe2000
Copy link
Contributor

@sewe2000 sewe2000 commented Aug 25, 2025

Description

Hi.
I wanted to start a broader discussion about Python validation bindings. I would like to carry on with the idea represented in the PR #41455.

This PR is my solution to the problem. Please feel free to correct me. Any piece of advice is very valuable to me since I'm relatively new to the QGIS codebase.

Solves #53813

@github-actions github-actions bot added this to the 4.0.0 milestone Aug 25, 2025
@sewe2000 sewe2000 changed the title Python layer validation bindings Expose setAllowCommit to Python Aug 25, 2025
@sewe2000 sewe2000 changed the title Expose setAllowCommit to Python Expose setAllowCommit to Python (and extend functionality) Aug 25, 2025
@sewe2000 sewe2000 changed the title Expose setAllowCommit to Python (and extend functionality) Expose setAllowCommit to Python Aug 25, 2025
@sewe2000 sewe2000 changed the title Expose setAllowCommit to Python Expose setAllowCommit to Python (and other functions) Aug 25, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2025

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 Sep 9, 2025
@sewe2000
Copy link
Contributor Author

@qgis

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 11, 2025
@nyalldawson
Copy link
Collaborator

I'm -1 to the current approach used in this PR -- I think it's too complex and fragile to be suitable.

I'd propose an alternative:

  • Introduce an interface class which is modelled after QgsApplicationExitBlockerInterface (Eg something like QgsLayerChangesCommitBlockerInterface). This would have a pure virtual method bool allowCommit( QgsMaplayer* layer ) = 0.
  • Follow the approach used for registering/unregistering application exit blockers (see eg QgsInterface::registerApplicationExitBlocker / QgisInterface::unregisterApplicationExitBlocker ), and their equivalents in QgisApp
  • Only target the changes at the app level, and don't modify any of the core classes. The blocking would happen in the app UI, after the actions which will commit changes to the layer are triggered (but before they the actual commitChanges call occurs). There we'd iterate through all the registered QgsLayerChangesCommitBlockerInterface objects, and if any of them return true, we'd abort the commit (with appropriate GUI feedback -- use the message bar for this).

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

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 stale Uh oh! Seems this work is abandoned, and the PR is about to close. and removed stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Oct 2, 2025
@sewe2000 sewe2000 force-pushed the bug_18101-python-validation-bindings branch from 6833f4a to 19d4630 Compare October 2, 2025 05:59
@sewe2000 sewe2000 marked this pull request as draft October 2, 2025 06:00
@sewe2000 sewe2000 force-pushed the bug_18101-python-validation-bindings branch 2 times, most recently from 5271b22 to be00190 Compare October 2, 2025 11:44
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit d930c71)

🍎 MacOS Qt6 builds

Download MacOS Qt6 builds of this PR for testing.
This app is not notarized, run sudo xattr -d com.apple.quarantine /Applications/QGIS*.app to avoid the warning
(Built from commit d930c71)

@sewe2000 sewe2000 marked this pull request as ready for review October 6, 2025 13:08
@sewe2000 sewe2000 requested a review from nyalldawson October 6, 2025 13:08
@github-actions
Copy link
Contributor

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 Oct 21, 2025
@sewe2000
Copy link
Contributor Author

sewe2000 commented Oct 21, 2025

@nyalldawson I think it's ready to be reviewed

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 21, 2025
@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 5, 2025
@qgis qgis deleted a comment from github-actions bot Nov 5, 2025
@nyalldawson nyalldawson removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 5, 2025
@github-actions
Copy link
Contributor

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.

@sewe2000
Copy link
Contributor Author

@nyalldawson

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 14, 2026
@nyalldawson nyalldawson added the API API improvement only, no visible user interface changes label Jan 27, 2026
@github-actions
Copy link
Contributor

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 Feb 11, 2026
@ostr00000
Copy link
Contributor

What is the status of this PR? is it going to be merged soon, or there is something more needed? (or are they any meta issues like dev team is too busy at the moment)?

I am interested in this feature, but I would like to know about estimated schedule for this change (is it worth to wait for this a bit longer?).

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 11, 2026
@elpaso
Copy link
Contributor

elpaso commented Feb 11, 2026

What is the status of this PR? is it going to be merged soon, or there is something more needed? (or are they any meta issues like dev team is too busy at the moment)?

I am interested in this feature, but I would like to know about estimated schedule for this change (is it worth to wait for this a bit longer?).

I didn't review your PR but I noticed that a couple of tests are failing: this is a blocker for merging in any situation.

return True

my_blocker = MyPluginCommitBlocker()
iface.registerApplicationExitBlocker(my_blocker)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
iface.registerApplicationExitBlocker(my_blocker)
iface.registerLayerChangesCommitBlocker(my_blocker)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't mark things resolved which aren't -- this makes reviewing VERY hard, and is part of the reason this PR is taking so long to get merged 😭

Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert changes

if ( QgsProject::instance()->gpsSettings()->automaticallyCommitFeatures() )
{
if (!QgisApp::instance()->tryCommitChanges(vlayer)) {
vlayer->startEditing();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
vlayer->startEditing();

Comment on lines +11106 to +11109
if (!QgisApp::instance()->tryCommitChanges(vlayer)) {
mSaveRollbackInProgress = false;
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this up a couple of lines, so that it's included in this check:

  if ( !vlayer || !vlayer->isEditable() || !vlayer->isModified() || !tryCommitChanges( vlayer ) )

That'd avoid the extra complication with the mSaveRollbackInProgress handling

Comment on lines +17132 to +17133
messageBar()->pushWarning( tr("Committing changes to the layer is blocked"),
tr("The ability to commit changes to the '%1' layer has been blocked by a plugin or script").arg( layer->name() ));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs for QgsLayerChangesCommitBlockerInterface state "It is safe to use GUI widgets in implementations of this function, including message boxes or custom dialogs with event loops.". I'd leave responsibility for advising users of the situation entirely up to the QgsLayerChangesCommitBlockerInterface implementation -- that way the plugin/script can decide the best way to notify users, and avoid a potentially redundant generic warning showing here.

Comment on lines +332 to +337
/**
* Displays a warning about layer `layer` being blocked from the process of
* committing changes using the messagebar.
*
**/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not resolved

case QMessageBox::Save:
if (!QgisApp::instance()->tryCommitChanges(layer)) {
res = false;
layer->triggerRepaint();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
layer->triggerRepaint();

Co-authored-by: Nyall Dawson <nyall.dawson@gmail.com>
@github-actions github-actions bot added the GRASS label Feb 13, 2026
sewe2000 and others added 11 commits February 13, 2026 14:23
…e.sip.in

Co-authored-by: Nyall Dawson <nyall.dawson@gmail.com>
Co-authored-by: Nyall Dawson <nyall.dawson@gmail.com>
…dings' into bug_18101-python-validation-bindings

# Conflicts:
#	src/app/gps/qgsappgpsdigitizing.cpp
# Conflicts:
#	python/gui/auto_additions/qgisinterface.py
#	python/gui/auto_generated/qgisinterface.sip.in
#	python/gui/gui_auto.sip
…ings

Update branch and fix pre-commit errors
Co-authored-by: Nyall Dawson <nyall.dawson@gmail.com>
remove this nasty space from code-review suggestion at the end of line
previously there were some IDE "improvements"
there was a some error when commiting suggestion from github
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2026

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 Mar 1, 2026
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Mar 2, 2026
@nyalldawson nyalldawson added Frozen Feature freeze - Do not merge! and removed Frozen Feature freeze - Do not merge! labels Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API API improvement only, no visible user interface changes GRASS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants