Hi On Fri, Apr 19, 2013 at 11:18 PM, Matthias Kuhn <[email protected]> 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 <[email protected]> wrote: >>> Hi Matthias >>> >>> On Fri, Apr 19, 2013 at 4:07 PM, Matthias Kuhn <[email protected]> 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 >>> [email protected] >>> 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 ============================================== _______________________________________________ Qgis-developer mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/qgis-developer
