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

Avoid modal dialogs in relation reference / relation editor #47392

Merged
merged 9 commits into from
Feb 21, 2022
Merged

Avoid modal dialogs in relation reference / relation editor #47392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f971a47
Do dialogs not open modal since otherwise the canvas are blocked and …
signedav Feb 14, 2022
bc43eee
showing form not modal to be able to controll the mapcanvas when addi…
signedav Feb 16, 2022
776265e
don't hide the parent dialog widget when it's QgisApp main window. Th…
signedav Feb 16, 2022
a470950
better readability for the the same
signedav Feb 16, 2022
7dac040
reenter lost override keyword and some docs
signedav Feb 16, 2022
033b02b
fix typo
signedav Feb 16, 2022
a762caf
fix python test classes inheriting QgsVectorLayerTools
signedav Feb 17, 2022
b00b774
delete QgsFeatureAction when the addFeatureFinished signal is called
signedav Feb 18, 2022
9b6e559
clearer naming of parent and parent's parent of the current object
signedav Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion python/core/auto_generated/qgstrackedvectorlayertools.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,21 @@ class QgsTrackedVectorLayerTools : QgsVectorLayerTools
Constructor for QgsTrackedVectorLayerTools.
%End

virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature ) const;
virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature, QWidget *parentWidget = 0, bool showModal = true, bool hideParent = false ) const;

%Docstring
This method calls the addFeature method of the backend :py:class:`QgsVectorLayerTools`

:param layer: The layer to which the feature should be added
:param defaultValues: Default values for the feature to add
:param defaultGeometry: A default geometry to add to the feature
:param feature: A pointer to the feature
:param parentWidget: The widget calling this function to be passed to the used dialog
:param showModal: If the used dialog should be modal or not
:param hideParent: If the parent widget should be hidden, when the used dialog is opened

:return: ``True`` in case of success, ``False`` if the operation failed/was aborted
%End
virtual bool startEditing( QgsVectorLayer *layer ) const;

virtual bool stopEditing( QgsVectorLayer *layer, bool allowCancel ) const;
Expand Down
5 changes: 4 additions & 1 deletion python/core/auto_generated/vector/qgsvectorlayertools.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ in your application.
QgsVectorLayerTools();


virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues = QgsAttributeMap(), const QgsGeometry &defaultGeometry = QgsGeometry(), QgsFeature *feature /Out/ = 0 ) const = 0;
virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues = QgsAttributeMap(), const QgsGeometry &defaultGeometry = QgsGeometry(), QgsFeature *feature /Out/ = 0, QWidget *parentWidget = 0, bool showModal = true, bool hideParent = false ) const = 0;
%Docstring
This method should/will be called, whenever a new feature will be added to the layer

:param layer: The layer to which the feature should be added
:param defaultValues: Default values for the feature to add
:param defaultGeometry: A default geometry to add to the feature
:param parentWidget: The widget calling this function to be passed to the used dialog
:param showModal: If the used dialog should be modal or not
:param hideParent: If the parent widget should be hidden, when the used dialog is opened

:return: - ``True`` in case of success, ``False`` if the operation failed/was aborted
- feature: Updated feature after adding will be written back to this
Expand Down
32 changes: 29 additions & 3 deletions src/app/qgsfeatureaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ QgsAttributeDialog *QgsFeatureAction::newDialog( bool cloneFeature )
connect( pb, &QAbstractButton::clicked, a, &QgsFeatureAction::execute );
}
}

return dialog;
}

Expand Down Expand Up @@ -168,7 +167,7 @@ bool QgsFeatureAction::editFeature( bool showModal )
return true;
}

bool QgsFeatureAction::addFeature( const QgsAttributeMap &defaultAttributes, bool showModal, QgsExpressionContextScope *scope SIP_TRANSFER )
bool QgsFeatureAction::addFeature( const QgsAttributeMap &defaultAttributes, bool showModal, QgsExpressionContextScope *scope SIP_TRANSFER, bool hideParent )
{
if ( !mLayer || !mLayer->isEditable() )
return false;
Expand Down Expand Up @@ -254,7 +253,6 @@ bool QgsFeatureAction::addFeature( const QgsAttributeMap &defaultAttributes, boo
}
else
{

QgsAttributeDialog *dialog = newDialog( false );
// delete the dialog when it is closed
dialog->setAttribute( Qt::WA_DeleteOnClose );
Expand All @@ -270,6 +268,12 @@ bool QgsFeatureAction::addFeature( const QgsAttributeMap &defaultAttributes, boo
setParent( dialog ); // keep dialog until the dialog is closed and destructed
connect( dialog, &QgsAttributeDialog::finished, this, &QgsFeatureAction::addFeatureFinished );
dialog->show();

if ( hideParent )
{
connect( this, &QgsFeatureAction::addFeatureFinished, this, &QgsFeatureAction::unhideParentWidget );
hideParentWidget();
}
mFeature = nullptr;
return true;
}
Expand Down Expand Up @@ -314,6 +318,28 @@ void QgsFeatureAction::onFeatureSaved( const QgsFeature &feature )
}
}

void QgsFeatureAction::hideParentWidget()
{
QWidget *dialog = parentWidget();
if ( dialog )
{
QWidget *triggerWidget = dialog->parentWidget();
if ( triggerWidget && triggerWidget->window()->objectName() != QStringLiteral( "QgisApp" ) )
triggerWidget->window()->setVisible( false );
}
}

void QgsFeatureAction::unhideParentWidget()
{
QWidget *dialog = parentWidget();
if ( dialog )
{
QWidget *triggerWidget = dialog->parentWidget();
if ( triggerWidget )
triggerWidget->window()->setVisible( true );
}
}

QgsFeature QgsFeatureAction::feature() const
{
return mFeature ? *mFeature : QgsFeature();
Expand Down
7 changes: 6 additions & 1 deletion src/app/qgsfeatureaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,13 @@ class APP_EXPORT QgsFeatureAction : public QAction
* and override with values in defaultAttributes if provided.
*
* \param defaultAttributes Provide some default attributes here if desired.
* \param scope Scope of the expression
* \param showModal If the used dialog should be modal or not
* \param hideParent If the parent widget should be hidden, when the used dialog is opened
*
* \returns TRUE if feature was added if showModal is true. If showModal is FALSE, returns TRUE in every case
*/
bool addFeature( const QgsAttributeMap &defaultAttributes = QgsAttributeMap(), bool showModal = true, QgsExpressionContextScope *scope = nullptr );
bool addFeature( const QgsAttributeMap &defaultAttributes = QgsAttributeMap(), bool showModal = true, QgsExpressionContextScope *scope = nullptr, bool hideParent = false );

/**
* Sets whether to force suppression of the attribute form popup after creating a new feature.
Expand All @@ -79,6 +82,8 @@ class APP_EXPORT QgsFeatureAction : public QAction

private slots:
void onFeatureSaved( const QgsFeature &feature );
void unhideParentWidget();
void hideParentWidget();

private:
QgsAttributeDialog *newDialog( bool cloneFeature );
Expand Down
10 changes: 5 additions & 5 deletions src/app/qgsguivectorlayertools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@
#include "qgsvectordataprovider.h"
#include "qgsvectorlayer.h"

bool QgsGuiVectorLayerTools::addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feat ) const
bool QgsGuiVectorLayerTools::addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feat, QWidget *parentWidget, bool showModal, bool hideParent ) const
{
QgsFeature *f = feat;
if ( !feat )
f = new QgsFeature();

f->setGeometry( defaultGeometry );
QgsFeatureAction a( tr( "Add feature" ), *f, layer );
a.setForceSuppressFormPopup( forceSuppressFormPopup() );
const bool added = a.addFeature( defaultValues );
QgsFeatureAction *a = new QgsFeatureAction( tr( "Add feature" ), *f, layer, QString(), -1, parentWidget );
a->setForceSuppressFormPopup( forceSuppressFormPopup() );
connect( a, &QgsFeatureAction::addFeatureFinished, a, &QObject::deleteLater );
const bool added = a->addFeature( defaultValues, showModal, nullptr, hideParent );
if ( !feat )
delete f;

return added;
}

Expand Down
6 changes: 5 additions & 1 deletion src/app/qgsguivectorlayertools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,14 @@ class QgsGuiVectorLayerTools : public QgsVectorLayerTools
* \param layer The layer to which the feature should be added
* \param defaultValues Default values for the feature to add
* \param defaultGeometry A default geometry to add to the feature
* \param feat A pointer to the feature
* \param parentWidget The widget calling this function to be passed to the used dialog
* \param showModal If the used dialog should be modal or not
* \param hideParent If the parent widget should be hidden, when the used dialog is opened
*
* \returns TRUE in case of success, FALSE if the operation failed/was aborted
*/
bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feat = nullptr ) const override;
bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feat = nullptr, QWidget *parentWidget = nullptr, bool showModal = true, bool hideParent = false ) const override;

/**
* This should be called, whenever a vector layer should be switched to edit mode. If successful
Expand Down
4 changes: 2 additions & 2 deletions src/core/qgstrackedvectorlayertools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@
#include "qgsvectorlayer.h"


bool QgsTrackedVectorLayerTools::addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature ) const
bool QgsTrackedVectorLayerTools::addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature, QWidget *parentWidget, bool showModal, bool hideParent ) const
{
QgsFeature *f = feature;
if ( !feature )
f = new QgsFeature();

const_cast<QgsVectorLayerTools *>( mBackend )->setForceSuppressFormPopup( forceSuppressFormPopup() );

if ( mBackend->addFeature( layer, defaultValues, defaultGeometry, f ) )
if ( mBackend->addFeature( layer, defaultValues, defaultGeometry, f, parentWidget, showModal, hideParent ) )
{
mAddedFeatures[layer].insert( f->id() );
if ( !feature )
Expand Down
15 changes: 14 additions & 1 deletion src/core/qgstrackedvectorlayertools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,20 @@ class CORE_EXPORT QgsTrackedVectorLayerTools : public QgsVectorLayerTools
*/
QgsTrackedVectorLayerTools() = default;

bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature ) const override;
/**
* This method calls the addFeature method of the backend QgsVectorLayerTools
*
* \param layer The layer to which the feature should be added
* \param defaultValues Default values for the feature to add
* \param defaultGeometry A default geometry to add to the feature
* \param feature A pointer to the feature
* \param parentWidget The widget calling this function to be passed to the used dialog
* \param showModal If the used dialog should be modal or not
* \param hideParent If the parent widget should be hidden, when the used dialog is opened
*
* \returns TRUE in case of success, FALSE if the operation failed/was aborted
*/
bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues, const QgsGeometry &defaultGeometry, QgsFeature *feature, QWidget *parentWidget = nullptr, bool showModal = true, bool hideParent = false ) const override;
bool startEditing( QgsVectorLayer *layer ) const override;
bool stopEditing( QgsVectorLayer *layer, bool allowCancel ) const override;
bool saveEdits( QgsVectorLayer *layer ) const override;
Expand Down
5 changes: 4 additions & 1 deletion src/core/vector/qgsvectorlayertools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@ class CORE_EXPORT QgsVectorLayerTools : public QObject
* \param defaultValues Default values for the feature to add
* \param defaultGeometry A default geometry to add to the feature
* \param feature Updated feature after adding will be written back to this
* \param parentWidget The widget calling this function to be passed to the used dialog
* \param showModal If the used dialog should be modal or not
* \param hideParent If the parent widget should be hidden, when the used dialog is opened
* \returns TRUE in case of success, FALSE if the operation failed/was aborted
*
*/
virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues = QgsAttributeMap(), const QgsGeometry &defaultGeometry = QgsGeometry(), QgsFeature *feature SIP_OUT = nullptr ) const = 0;
virtual bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &defaultValues = QgsAttributeMap(), const QgsGeometry &defaultGeometry = QgsGeometry(), QgsFeature *feature SIP_OUT = nullptr, QWidget *parentWidget = nullptr, bool showModal = true, bool hideParent = false ) const = 0;

// TODO QGIS 4: remove const qualifier

Expand Down
6 changes: 3 additions & 3 deletions src/gui/editorwidgets/qgsrelationreferencewidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,8 +536,8 @@ void QgsRelationReferenceWidget::openForm()
return;

QgsAttributeEditorContext context( mEditorContext, mRelation, QgsAttributeEditorContext::Single, QgsAttributeEditorContext::StandaloneDialog );
QgsAttributeDialog attributeDialog( mReferencedLayer, new QgsFeature( feat ), true, this, true, context );
attributeDialog.exec();
QgsAttributeDialog *attributeDialog = new QgsAttributeDialog( mReferencedLayer, new QgsFeature( feat ), true, this, true, context );
attributeDialog->show();
}

void QgsRelationReferenceWidget::highlightFeature( QgsFeature f, CanvasExtent canvasExtent )
Expand Down Expand Up @@ -908,7 +908,7 @@ void QgsRelationReferenceWidget::entryAdded( const QgsFeature &feat )
}
}

if ( mEditorContext.vectorLayerTools()->addFeature( mReferencedLayer, attributes, f.geometry(), &f ) )
if ( mEditorContext.vectorLayerTools()->addFeature( mReferencedLayer, attributes, f.geometry(), &f, this, false, true ) )
{
QVariantList attrs;
for ( const QString &fieldName : std::as_const( mReferencedFields ) )
Expand Down
4 changes: 2 additions & 2 deletions src/gui/qgsabstractrelationeditorwidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ QgsFeatureIds QgsAbstractRelationEditorWidget::addFeature( const QgsGeometry &ge
// n:m Relation: first let the user create a new feature on the other table
// and autocreate a new linking feature.
QgsFeature finalFeature;
if ( !vlTools->addFeature( mNmRelation.referencedLayer(), QgsAttributeMap(), geometry, &finalFeature ) )
if ( !vlTools->addFeature( mNmRelation.referencedLayer(), QgsAttributeMap(), geometry, &finalFeature, this, false, true ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

By passing showModal=false the add feature dialog is opened not blocking with open instead of exec.
This causes addFeature to return immediately and all consecutively logic around finalFeature fails. The first feature id returned by QgsAbstractRelationEditorWidget::addFeature will not be valid.
In multiedit mode, only the first feature will get the values edited in the add feature dialog.

I don't know how to solve this without changing the return type of QgsAbstractRelationEditorWidget::addFeature or make the dialog modal (blocking with exec) again.

@signedav do you maybe have an idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I see, you don't need to change the QgsAbstractRelationEditorWidget::addFeature and just don't rely on the finalFeature. Instead waiting for the return by a signal. I know it's not that nice, but I would really like not making it modal because this brings lot's of other trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I also thing the dialog should stay non-modal. How can I wait on a signal without blocking the gui thread? Some sort of wait & process events, do you know of an example where we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't you just use normal signal slot and return after the call for addFeature?

Copy link
Contributor

Choose a reason for hiding this comment

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

But then it means QgsAbstractRelationEditorWidget::addFeature will not be able to return the correct Ids, because vlTools->addFeature returns immediately while the dialog is still open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah now I get it. Hm... With QEventLoop the GUI thread gets blocked, so we cannot proceed on the other dialog?

But anyway I feel medium comfortable with a wait. What are the alternatives? Only to change the return type as you mentioned. This breaks the API - but did it once work on an LTR at all`? Can you check that?
And yes it needs changes on multi edit (what I belief you know better than me).

return QgsFeatureIds();

addedFeatureIds.insert( finalFeature.id() );
Expand Down Expand Up @@ -280,7 +280,7 @@ QgsFeatureIds QgsAbstractRelationEditorWidget::addFeature( const QgsGeometry &ge
keyAttrs.insert( fields.indexFromName( fieldPair.referencingField() ), mFeatureList.first().attribute( fieldPair.referencedField() ) );

QgsFeature linkFeature;
if ( !vlTools->addFeature( mRelation.referencingLayer(), keyAttrs, geometry, &linkFeature ) )
if ( !vlTools->addFeature( mRelation.referencingLayer(), keyAttrs, geometry, &linkFeature, this, false, true ) )
return QgsFeatureIds();

addedFeatureIds.insert( linkFeature.id() );
Expand Down
12 changes: 7 additions & 5 deletions src/gui/qgsrelationeditorwidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,10 @@ void QgsRelationEditorWidget::addFeatureGeometry()
mMapToolDigitize->setLayer( layer );

// window is always on top, so we hide it to digitize without seeing it
window()->setVisible( false );
if ( window()->objectName() != QStringLiteral( "QgisApp" ) )
{
window()->setVisible( false );
}
setMapTool( mMapToolDigitize );

connect( mMapToolDigitize, &QgsMapToolDigitizeFeature::digitizingCompleted, this, &QgsRelationEditorWidget::onDigitizingCompleted );
Expand Down Expand Up @@ -780,16 +783,15 @@ void QgsRelationEditorWidget::onKeyPressed( QKeyEvent *e )
{
if ( e->key() == Qt::Key_Escape )
{
window()->setVisible( true );
window()->raise();
window()->activateWindow();
unsetMapTool();
}
}

void QgsRelationEditorWidget::mapToolDeactivated()
{
window()->setVisible( true );
window()->raise();
window()->activateWindow();

if ( mEditorContext.mainMessageBar() && mMessageBarItem )
{
mEditorContext.mainMessageBar()->popWidget( mMessageBarItem );
Expand Down
5 changes: 4 additions & 1 deletion tests/src/gui/testqgsrelationreferencewidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,11 @@ void TestQgsRelationReferenceWidget::testIdentifyOnMap()
// referenced layer
class DummyVectorLayerTools : public QgsVectorLayerTools // clazy:exclude=missing-qobject-macro
{
bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &, const QgsGeometry &, QgsFeature *feat = nullptr ) const override
bool addFeature( QgsVectorLayer *layer, const QgsAttributeMap &, const QgsGeometry &, QgsFeature *feat = nullptr, QWidget *parentWidget = nullptr, bool showModal = true, bool hideParent = false ) const override
{
Q_UNUSED( parentWidget );
Q_UNUSED( showModal );
Q_UNUSED( hideParent );
feat->setAttribute( QStringLiteral( "pk" ), 13 );
feat->setAttribute( QStringLiteral( "material" ), QStringLiteral( "steel" ) );
feat->setAttribute( QStringLiteral( "diameter" ), 140 );
Expand Down
4 changes: 2 additions & 2 deletions tests/src/python/test_qgsrelationeditwidget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def test_add_feature_geometry(self):
# Mock vector layer tool to just set default value on created feature
class DummyVlTools(QgsVectorLayerTools):

def addFeature(self, layer, defaultValues, defaultGeometry):
def addFeature(self, layer, defaultValues, defaultGeometry, parentWidget=None, showModal=True, hideParent=False):
f = QgsFeature(layer.fields())
for idx, value in defaultValues.items():
f.setAttribute(idx, value)
Expand Down Expand Up @@ -441,7 +441,7 @@ def setValues(self, values):
"""
self.values = values

def addFeature(self, layer, defaultValues, defaultGeometry):
def addFeature(self, layer, defaultValues, defaultGeometry, parentWidget=None, showModal=True, hideParent=False):
"""
Overrides the addFeature method
:param layer: vector layer
Expand Down