I am currently going through the patch and most of it seems very good. However, one point is not good: in updateAllKickoffIds() you added new queries. This patch should also be about improving the performance of Resource. Introducing another query has the opposite effect. Thus, I think we need to get rid of that one again...
Cheers, Sebastian BTW: please fix the indentation in your editor: KDE uses 4 spaces, no tabs. On 06/01/2010 09:57 AM, Vishesh Handa wrote: > I cleaned up the code. Introduced 2 new functions. And cleaned up the > comments in remove() and invalidateCache(). Both of them now work > properly. I didn't see the need of keep the determineAllUris() in > remove(), so I removed it. There are loads of calls in Resource to check > if the m_data is valid. Are those really necessary? The only case where > m_data won't valid we be when we assign it manually via the constructor. > I think we can remove those checks. > > Otherwise I think we're done with the patch. Could you please check for > any multi-threading issues? > > Thanks > - Vishesh Handa > > > On Tue, Jun 1, 2010 at 1:21 AM, Vishesh Handa <[email protected] > <mailto:[email protected]>> wrote: > > Sorry for the stream of emails. I'm done. Honest. > > I fixed a bug in removeProperty() and setProperty(). And added > another test. > > - Vishesh Handa > > > On Tue, Jun 1, 2010 at 12:30 AM, Vishesh Handa <[email protected] > <mailto:[email protected]>> wrote: > > Yea, so studying for an exam is kinda boring. I landed up fixing > the patch. > > A couple of things - > * QUrl::isValid() returns true for simple strings like "res1". > Weird! > * The only advantage of using a QSet instead of a QList is that > we avoid duplicates. > * I've modified some of the comments, but I think a couple are > still missing. > * Maybe the addition of other identifiers should be moved to a > new function. > > If/When we merge determineUri() and load(). The loading is the > m_kickOffUris could be done along with it. > > Otherwise, I think we're done. We just need to clean up the code > a little bit. > > - Vishesh Handa > > > On Mon, May 31, 2010 at 9:17 PM, Vishesh Handa > <[email protected] <mailto:[email protected]>> wrote: > > This is what I've done - > 1. Combined both the lists > 2. Altered determineUri to add all identifying properties to > m_kickOffUri. > 3. Misc fixes > > It's unfortunately not passing one of the tests! (And is > spewing loads of QDBus warnings) > > I'll look more into it later. Tomorrow probably. > > - Vishesh Handa > > > On Mon, May 31, 2010 at 5:58 PM, Vishesh Handa > <[email protected] <mailto:[email protected]>> wrote: > > > > On Mon, May 31, 2010 at 5:47 PM, Sebastian Trüg > <[email protected] <mailto:[email protected]>> wrote: > > On 05/31/2010 01:44 PM, Vishesh Handa wrote: > > Resources are identified by one of these 3 ways - > > 1. nao:identifier > > 2. nie:url (includes the whole filex:/ part) > > 3. nepomuk:/res/ > > > > m_kickOffId uses 1, 2 (minus the filex:/) & 3. And > m_kickOffUri uses 2 & > > 3. I say, we scrap the m_kickOffId, and store the > nao:identifier in the > > m_kickOffUri ( perhaps name it to something else? ) > > > > The determineUri code would become a lot simpler - > > > > if( m_kickOffUri.isValid() ) { > > // Either 2 or 3 > > } > > else { > > // nao:identifier > > } > > > > The would even clear one of the odd cases where > someone tries to > > initialize a Resource by providing its > nepomuk:/res/ uri as a QString > > instead a QUrl. > > So you want to store a string in a KUrl then? Well, > that could work indeed. > > > Yup. Or if you really want we could do the opposite and > store everything as strings. > > > > > > > 2. Convert the ResourceData m_kickOffUri > into a list AND make sure > > that > > > while determining one URI it adds all other > cases to the lists as > > well. > > > > > > Number 1 is more of a convenience, but *2* > is really important. You've > > > done half the job ( I thought we'll take > care of it in another patch ) > > > > > > Now, about the comments in > determineFinalResourceData(). The flaw with > > > our, not so little, proxy removal plan was > that if - > > > Resource r1("foo"); > > > r1.determineUri() > > > > > > Resource r2( foo's nie:url ); > > > r2.determineUri() // The proxy thing would > be activated ( could be > > > avoided via 2 ) > > > > > > Resource r3( foo's nie:url ); > > > r3.determineUri() // The proxy thing is > AGAIN activated as the nie:url > > > wasn't added to the list. > > > > > > With your patch you seem to have fixed the > problem. But I would have > > > preferred the more concrete solution via 2. > > > > Agreed. > > > > How about converting them into hash tables instead > of lists? That way we > > could easily check if addProperty() or > removeProperty() updates any of > > the identifying properties in m_kickOddUri. We > could then even > > potentially stop clearing up the cache after ever > metadata move > > operation. > > What would the hash contain? > > > Uhh. What I meant was that we store them in a set > instead of a list. It's a rather trivial thing. > > I guess I'll start modifying the patch. > > - Vishesh Handa > > > > Cheers, > Sebastian > > > > > > _______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
