Dolphin and Baloo

[Frank Reininghaus]

Hi,

2014-02-05 Vishesh Handa:
> On Tuesday 04 February 2014 15:45:53 Frank Reininghaus wrote:
>> Hi,
>>
>> 2014-02-04 Vishesh Handa:
>> > I just fixed all the issues you mentioned.
>>
>> cool, thanks! I think we're getting there. I noticed just two more things:
>>
>> 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)).

>> 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)).

>> (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.

> but I'm not too sure why that works.

Well, why shouldn't it work? Essentially, the effect of this change is
that only Baloo roles will be updated if a "fileMetaDataChanged"
signal is received. The previous code also updated all other roles
(e.g., file type, directory size) in that case, but that is not
necessary at all. In the case that not only the tags, but also the
file contents change, we get a signal from KDirWatch that triggers a
call of rolesData anyway.

BTW, thanks for

http://quickgit.kde.org/?p=kde-baseapps.git&a=commit&h=cad3f617821541ca29bf6681fe9b2ff3d02b4101

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 :-)

Cheers,
Frank