Dolphin and Baloo

[Frank Reininghaus]

Hi,

2014-02-05 Vishesh Handa:
[...]
>> >> (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.

Yes, I see that it can be slightly misleading. However, if it replaced
all the data, then it would also delete any values which had been set
outside of KFileItemModelRolesUpdater's control.

>> > 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?

The final result would be the same, right? So the simplest thing would
be to re-add the foreach loop just below the definition of "data".

Cheers,
Frank