[Nepomuk] Nepomuk Core - Questions & Patches

[Sebastian Trüg]

On 05/09/2010 02:37 PM, Vishesh Handa wrote:
> *4.* In *ResourceData::resetAll* - shouldn't *m_cacheDirty = false *be
> equal to true? Considering resetAll is called when the resource is being
> Removed, it shouldn't make a any difference. Still, just to be on the
> safer side.

This would not make much sense since the Resource becomes invalid and
has no URI to check for any data to load.

> *5. Error Checking: *There is considerable error checking in
> Nepomuk::ResouceFilterModel where /Soprano::Error/s are returned, but
> after that it all stops.

This is a bad situation indeed. However, I am not sure how to fix that
at the moment. The only solution I see would be to emit signals from
ResourceManager reporting the problems - although that does not at all
fit with the rest of the sync api.

> Particularly in ResourceData and Resource. We
> could make the Resource class return Error codes or simply boolean
> values in functions like SetProperty.

That is not possible due to binary compatibility.

> That would mean additional error checking from the users side, and would
> result in ugly code. This is one of cases where I would love to use
> exceptions.

I agree. Here exceptions would be nice.

> *6. Destructors: *Shouldn't ResourceManagerPrivate have a destructor
> where it clears out the remaining ResouceData in m_initializedData and
> other? I know ResourceData::resetAll removes them, but it feels wrong
> not to check them just in case.

Maybe we should rather put a Q_ASSERT(m_initializedData.isEmpty()) in there.

> *7.* *ResourceManagerPrivate::dataCacheFull()*: Why 1000? I think this
> value should be customizable.

That is a random value I chose. I would be fine with a customization.
But I doubt it would ever be used.

> *8.* *ResourceManager::allResourcesWithProperty*: They seem to be using
> a QList internally and using the QList:contains() functions quite
> frequently. Considering QList isn't sorted, i think it would be quite
> costly -> O(n). How about using a QSet? The only problem is the added
> QSet::toList() in the end, which shouldn't be too slow. (Max O(n)). I've
> provided a patch.

please apply. :)

Cheers,
Sebastian