On Fri, Apr 12, 2013 at 10:33 AM, Jürgen E. <j...@norbit.de> wrote: >> Recently I have been wondering whether it would not be better to >> actually stop using flags and replace them by usual getter/setter >> functions. My main concern is the setSubsetOfAttributes() function: it >> also sets the flag, so the users may end up with a weird behavior, >> e.g.: >> QgsFeatureRequest().setSubsetOfAttributes( ["name","type"], fields >> ).setFlags( NoGeometry ) >> That would fetch all attributes... because the setFlags overrides the >> previously set SubsetOfAttributes flag. > > Yes, I've also thought about that, but somehow missed to speak up - but IIRC > (;)) I migrated everything accordingly. > > Given that the flags need qualification it's even shorter to use the > getter/setters API-wise.
That's right. Thinking about it again, we should really fix the API before 2.0. It seems you have ported that correctly (btw. many thanks for that!), but ordinary users that do not read c++ code will sooner or later struggle with it. I'm adding that to my todo list :-) >> Also, the way how things are done right now, providers need to >> implement fetching of attributes in this way (pseudo-code): >> if using subset of attributes: >> for each attribute from subset: >> fetch attribute >> else: >> for each attribute from provider: >> fetch attribute > >> This is quite annoying, we should probably have a simple array where >> each value would indicate whether to fetch attribute with that index >> or not (by default all values would be true). The fetching would be >> simplified to: >> for each attribute from provider: >> if attribute should be fetched: >> fetch attribute > >> What do you think? :-) > > Don't they just all do: > > subsetOfAttributes ? mRequest.subsetOfAttributes() : P->attributeIndexes() Not really. Usually there's the kind of code I have mentioned above. In theory we could change it to use the attributeIndexes() call. > Can't we move that to a requestedAttributes() in QgsAbstractFeatureIterator > and > just iterate over the result? Yes we can :-) > BTW what about parallel iterators? I suppose there were problems and thats > why > you added the "active iterators" - do you recall which providers were affected > and how? I have modified providers to have pointers to active iterators because of two reasons: 1. if the user asks for a second iterator and the provider does not support multiple iterators, the first one has to be closed - they would not behave correctly if used together 2. if the layer is deleted for whatever reason, we must ensure that any iterators that may still exist will be closed - they would cause crashes otherwise As of now, all providers support only one iterator per provider instance. For parallel iterators (N iterators, one thread) we could have a provider capability indicating whether they are supported or not. Providers supporting parallel iterators would not close the previously active iterators - instead they would only keep them in the list of active iterators. When we add multi-threaded rendering, things will get a bit more complex. Providers allowing parallel iterators should be fine - they will just need some locking of the list of active iterators. There will be a slight problem with providers allowing just one iterator: in a different thread we cannot simply close the currently active iterator (from a different thread) as that would be a run condition. I guess we will have to define the behavior in the following way - when the provider is asked for a new iterator, it will: a. wait for the active iterator to get closed - if it comes from a different thread b. close the old iterator - in case it comes from the same thread This should ensure that the rendering (or processing) thread will wait until GUI thread finishes what it wants (and vice versa), while still keeping the current behavior when dealing with only one thread. We will just need to explicitly assign iterators with the threads that asks for them. Btw. the case b is really just a sanity check for invalid code - it should not happen in production environment - if it does, there is something wrong with the application logic. Maybe during the porting of providers to support multi-threaded environment we will find out that it will be easier to simply update all providers to allow multiple iterators instead of adding the support for the case of N threads and one iterator - that is yet something that needs investigation. Regards Martin _______________________________________________ Qgis-developer mailing list Qgis-developer@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/qgis-developer