From kfm-devel  Wed Feb 05 17:10:20 2014
From: Vishesh Handa <me () vhanda ! in>
Date: Wed, 05 Feb 2014 17:10:20 +0000
To: kfm-devel
Subject: Re: Dolphin and Baloo
Message-Id: <4734620.Ct1SQFgZuf () vlap>
X-MARC-Message: https://marc.info/?l=kfm-devel&m=139162016003186

On Wednesday 05 February 2014 17:27:52 Frank Reininghaus wrote:
> Hi,
> 
> >> 1. There is a HAVE_NEPOMUK left in src/search/dolphinsearchbox.cpp. I
> >> guess the #ifdef'ed code should just be removed?
> > 
> > Not entirely sure. Will look more into it.
> 
> The #ifdef'ed code looks like it replaces the label "Find" by
> something else, based on
> Nepomuk2::Query::Query::titleFromQueryUrl(m_readOnlyQuery)
> 
> I'm currently unable to test this, and I don't remember if there was
> ever anything fancy being shown in the label, but still, it looks safe
> to remove that (at least if Baloo doesn't provide any replacement for
> titleFromQueryUrl(QString)).
> 

I've added such a function in Baloo and updated the code.

> >> 2. About this code in KFileItemModelRolesUpdater::rolesData():
> >> 
> >> #ifdef HAVE_BALOO
> >> 
> >>     if (m_balooFileMonitor)
> >>     
> >>         m_balooFileMonitor->addFile(item.localPath());
> >>     
> >>     // We never fill the Baloo roles over here since that would require
> >>     // us to block by spawning a local event loop. Instead we call a slot
> >>     // to fill the values later
> >>     QMetaObject::invokeMethod(this, "applyChangedBalooRoles",
> >>     
> >>                               Qt::QueuedConnection, Q_ARG(QString,
> >> 
> >> item.localPath()));
> >> #endif
> >> 
> >> (a) This will always invoke applyChangedBalooRoles and thus start a
> >> Baloo::FileFetchJob, even if no Baloo roles are shown at all. We could
> >> just include that statement in the "if" block to prevent that, right?
> > 
> > Fixed.
> 
> Cool. It could be done easier though: instead of
> 
> 
>     if (m_balooFileMonitor)
>         m_balooFileMonitor->addFile(item.localPath());
> 
>     const KBalooRolesProvider& rolesProvider =
> KBalooRolesProvider::instance();
> 
>     bool hasBalooRole = false;
>     foreach (const QByteArray& role, m_roles) {
>         if (rolesProvider.roles().contains(role)) {
>             hasBalooRole = true;
>             break;
>         }
>     }
> 
>     // We never fill the Baloo roles over here since that would require
>     // us to block by spawning a local event loop. Instead we call a slot
>     // to fill the values later
>     if (hasBalooRole) {
>         applyChangedBalooRoles(item.localPath());
>     }
> 
> we could just do
> 
>     if (m_balooFileMonitor) {
>         m_balooFileMonitor->addFile(item.localPath());
>         applyChangedBalooRoles(item.localPath());
>     }
> 
> because m_balooFileMonitor is != 0 if and only if there are "Baloo
> roles" (see KFileItemModelRolesUpdater::setRoles(const
> QSet<QByteArray>& roles)).
> 

Again. You're quite right. I feel like I should stop making so many commits 
and open up a review request :)

Fixed.

> >> (b) Is there a reason to use a QMetaObject invocation for calling
> >> applyChangedBalooRoles()? Since that function just starts the job, it
> >> should finish quite fast, right?
> > 
> > Right. Fixed it.
> > 
> >> (c) Unless I'm overlooking something, it looks to me like
> >> rolesData(KFileItem) invokes applyChangedBalooRoles(QString), which
> >> will then finally trigger applyChangedBalooRolesJobFinished(KJob*).
> >> The latter function then calls rolesData(KFileItem) again. This looks
> >> like an infinite recursion to me. It won't cause a crash or block the
> >> GUI because two of the recursive calls are asynchronous, but it will
> >> make Dolphin use 100% of one CPU core permanently. Or am I missing
> >> something here?
> > 
> > You're absolutely right.
> > 
> > I've changed it based on what you recommended
> 
> At first sight, this change (from ad74d7e575):
> 
> -    QHash<QByteArray, QVariant> data = rolesData(item);
> 
>      const KBalooRolesProvider& rolesProvider =
> KBalooRolesProvider::instance(); -    foreach (const QByteArray& role,
> rolesProvider.roles()) {
> -        // Overwrite all the role values with an empty QVariant,
> because the roles
> -        // provider doesn't overwrite it when the property value list is
> empty. -        // See bug 322348
> -        data.insert(role, QVariant());
> -    }
> +    QHash<QByteArray, QVariant> data;
> 
> looks like it might bring back bug 322348 though. I think that the
> foreach loop should be kept. It's just that "data" should be empty
> before that loop starts, i.e., just the initialization with
> rolesData(item) should be removed. Sorry if I was a bit unclear about
> this issue earlier.
> 

Are you sure it would bring back the bug? I think ...

I checked the code, and the foreach would be required. The name 'setData' is 
slightly misleading since it actually replaces only the roles which were 
provided and not all the data.

> > but I'm not too sure why that works.
> 
> Well, why shouldn't it work?

I assumed that setData would reset all the data :)

How would you prefer me to fix the code? Revert the original patch and fix it 
properly or just re-introduce the foreach loop?

> 
> BTW, thanks for
> 
> http://quickgit.kde.org/?p=kde-baseapps.git&a=commit&h=cad3f617821541ca29bf6
> 681fe9b2ff3d02b4101
> 
> I'm quite happy to see the bug-prone duplication of the code that
> answers the "is this path indexed or not" question go away :-)
> 

:D

> Cheers,
> Frank

-- 
Vishesh Handa