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

[Tim Sutton]

Hi

On Fri, Apr 19, 2013 at 11:18 PM, Matthias Kuhn <matthias.kuhn at gmx.ch> wrote:
> 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?

+1 for your changes

Regards

Tim

>
> 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
>> ==============================================
>
>



--
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
==============================================