[Qgis-developer] Suppress emitting of signal: Bad practice?

[Matthias Kuhn]

Thank you Tim and Martin,

On Fre 19 Apr 2013 18:02:12 CEST, Tim Sutton wrote:
> Hi
>
> On Fri, Apr 19, 2013 at 10:45 PM, Martin Dobias <wonder.sk at gmail.com> wrote:
>> Hi Matthias
>>
>> On Fri, Apr 19, 2013 at 4:07 PM, Matthias Kuhn <matthias.kuhn at gmx.ch> wrote:
>>> The QgsMapLayerRegistry::removeMapLayers allows to specify a parameter,
>>> which will suppress emitting signals (altough it seems that this is not
>>> even properly working [1]).
>>>
>>> This parameter can lead to strange behavior, where some parts of the
>>> software might not realize, that a layer has been removed and keep
>>> references to no longer available objects.
>>
>> I completely agree with you - it is a bad practice to pass a parameter
>> whether to emit signal or not - it is typically something that leads
>> to wrong design.
>>
>>
>>> So my questions are:
>>> * Does somebody know why removeMapLayer(s) accepts this parameter?
>>> * Do you see any negative effect of removing these or any upside in
>>> keeping these?
>>
>> I remember we have added that to support georeferencer. The workflow
>> with layers in QGIS is currently:
>
> Ah ok I thought that was me :-)
>
>> 1. create Qgs[Vector/Raster]Layer instance
>> 2. add the layer to map registry
>> 3. map registry emits signal, caught by legend widget
>> 4. legend widget adds the layer to the list of layers and updates the
>> list of layers in canvas
>> Georeferencer comes with its own map canvas and its own set of layers.
>> When we add a layer that should be georeferenced, we do not want it to
>> appear also on main canvas - that's where the 'emit signal' parameter
>> comes handy. Setting it to false will avoid loading the layer in the
>> main window.
>>
>> There are surely more ways how to fix the problem, but probably all of
>> them involve slight redesign of how the components are interconnected.
>> Maybe we do not need a map layer registry. Or maybe we could have
>> several map layer registries (instead of just having singleton), one
>> for each canvas in the application. Or maybe we need something more
>> general than just a list of layers: e.g. right now the ordering is
>> defined by legend widget... for a proper logic/gui separation we
>> should keep the layers + ordering + grouping in a data structure (and
>> only let legend widget serve as its view and controller).
>>

I see the problem and a complete redesign of this API is (at least for 
me) at the moment out of scope.

There are two signals emitted for removal (the buggy implementation)
* layerWillBeRemoved (emitted properly, zero or one time)
* layersWillBeRemoved (emitted once or twice)

For addMapLayers (batch method) there are also two signals (properly 
emitted IMO)
* mapLayerAdded
* mapLayersAdded

The batch mode remove signal (layersWillBeRemoved) has already been 
emitted at least once since 1.8 (regardless of theEmitSignal parameter) 
and I don't think there have been any complaints, I'll remove the 
condition around that one.

My (quickfix) proposal would then be:
* Also emit mapLayersAdded (batch mode) unconditionally to allow proper 
tracking of layers to anybody.
* Rename the parameter from "theEmitSignal" to "addToLayerRegistry" and 
rename the signal according to that, so the use of this is 
straightforward communicated to the developer.

Does that sound good to you? A little effort for a cleaner API :-)

>> I am not sure how feasible is to do something about it now, but in any
>> case I would warmly welcome another shift of API freeze, the amount of
>> things we need to fix in API is quite high (e.g. in my pipeline:
>> symbology-ng "V2" renaming, feature iterators + requests API fixes,
>> QgsVectorLayer API updates, QgsExpression API fixes, sip API 2 and
>> probably more).
>

Sounds like a lot of work you want to get done. Will the deadline 
extension be enough for you?

Regards,
Matthias

> Yes agreed I will propose another shift to the end of April (feature
> freeze remaining in effect).
>
> Regards
>
> Tim
>
>>
>> Cheers
>> Martin
>> _______________________________________________
>> Qgis-developer mailing list
>> Qgis-developer at lists.osgeo.org
>> http://lists.osgeo.org/mailman/listinfo/qgis-developer
>
>
>
> --
> Tim Sutton - QGIS Project Steering Committee Member (Release  Manager)
> ==============================================
> Please do not email me off-list with technical
> support questions. Using the lists will gain
> more exposure for your issues and the knowledge
> surrounding your issue will be shared with all.
>
> Visit http://linfiniti.com to find out about:
>  * QGIS programming and support services
>  * Mapserver and PostGIS based hosting plans
>  * FOSS Consulting Services
> Skype: timlinux
> Irc: timlinux on #qgis at freenode.net
> ==============================================