From kfm-devel Wed Feb 05 21:05:58 2014 From: Frank Reininghaus Date: Wed, 05 Feb 2014 21:05:58 +0000 To: kfm-devel Subject: Re: Dolphin and Baloo Message-Id: X-MARC-Message: https://marc.info/?l=kfm-devel&m=139163437011133 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 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 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