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

Replace connect to 'layerWillBeRemoved' by 'layersWillbeRemoved' in Q… #3189

Closed
wants to merge 1 commit into from

Conversation

dmarteau
Copy link
Contributor

@dmarteau dmarteau commented Jun 9, 2016

…gsEditorWidgetRegistry.

QgsEditorWidgetRegistry record every layer added to the QgsMapLayerRegistry by listening to signal 'layerWasAdded'. But layerWillBeRemoved is emitted only for layers owned by
QgsMapLayerRegistry while layersWillBeRemoved is emitted for all layers added to QgsMapLayerRegistry.

In the context of qgi server the layers are not owned by QgsMapLayerRegistry and thus layerWillBeRemoved is never called and connections to layers never released.

@@ -327,14 +327,16 @@ void QgsEditorWidgetRegistry::writeMapLayer( QgsMapLayer* mapLayer, QDomElement&
layerElem.appendChild( editTypesNode );
}

void QgsEditorWidgetRegistry::mapLayerWillBeRemoved( QgsMapLayer* mapLayer )
void QgsEditorWidgetRegistry::mapLayersWillBeRemoved( const QList<QgsMapLayer*>& layers )
Copy link
Member

Choose a reason for hiding this comment

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

did you run scripts/prepare-commit.sh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I didn't.

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 fixing my PR branch

@dmarteau dmarteau force-pushed the master branch 2 times, most recently from 348bde5 to 73bfe93 Compare June 9, 2016 17:36
…gsEditorWidgetRegistry.

    QgsEditorWidgetRegistry record every layer added to the
    QgsMapLayerRegistry by listening to signal 'layerWasAdded'.
    But layerWillBeRemoved is emitted only for layers owned by
    QgsMapLayerRegistry while layersWillBeRemoved is emitted for
    all layers added to QgsMapLayerRegistry.
@dmarteau
Copy link
Contributor Author

dmarteau commented Jun 9, 2016

Am I wrong or it seems that the travis build for QT5 python 3.3 fails for something not related to this PR ?

@nyalldawson
Copy link
Collaborator

Isn't this a bug? Logically, why should layersWillBeRemoved be emitted but not layerWillBeRemoved for non-owned layers. I think we should fix the API here instead.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 10, 2016

On 10/06/16 01:36, Nyall Dawson wrote:

Isn't this a bug? Logically, why should layersWillBeRemoved be emitted
but not layerWillBeRemoved for non-owned layers. I think we should fix
the API here instead.

Same thinking here

@m-kuhn
Copy link
Member

m-kuhn commented Jun 10, 2016

Am I wrong or it seems that the travis build for QT5 python 3.3 fails for something not related to this PR ?

Yes, the test ogcutils test seems to be flaky. Maybe @rouault knows what could potentially be going wrong there?

https://travis-ci.org/qgis/QGIS/jobs/136515197#L992-L993

@rouault
Copy link
Contributor

rouault commented Jun 10, 2016

@m-kuhn I cannot reproduce locally. I've added in 12bb1f7 more debug output in case this happens again

@m-kuhn
Copy link
Member

m-kuhn commented Jun 10, 2016

@rouault yes, it's only occasionally, I'll ping you if I see this happen again with more debug output.

@rldhont
Copy link
Contributor

rldhont commented Jun 10, 2016

@nyalldawson @m-kuhn @dmarteau I have created a new PR to update the QgsMapLayerRegistry.
#3194

We can probably close this one ?

@m-kuhn m-kuhn closed this Jun 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants