[Nepomuk] Nepomuk Core - Questions & Patches
Sebastian Trüg
trueg at kde.org
Tue Jun 1 10:43:07 CEST 2010
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 <handa.vish at gmail.com
> <mailto:handa.vish at gmail.com>> 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 <handa.vish at gmail.com
> <mailto:handa.vish at gmail.com>> 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
> <handa.vish at gmail.com <mailto:handa.vish at gmail.com>> 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
> <handa.vish at gmail.com <mailto:handa.vish at gmail.com>> wrote:
>
>
>
> On Mon, May 31, 2010 at 5:47 PM, Sebastian Trüg
> <trueg at kde.org <mailto:trueg at kde.org>> 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
>
>
>
>
>
>
More information about the Nepomuk
mailing list