Review Request #124919 - Created Aug. 25, 2015 and submitted Sept. 9, 2015, 9:49 p.m.
Now each object that needs D-Bus communication is registered as a separate D-Bus object and communication takes place directly.
FileContentIndexer has been made a long lived class now to register a D-Bus object for it.
baloo-monitor and balooctl seem to be working as before.
Maybe just 'fileIndexer' or some similar small word? This is quite the mouthfull. Vishesh Handa Aug. 25, 2015, 5:52 p.m.
This might not be thread safe. It calls batchTimings which accesses m_batchTimeBuffer. This variable is also being modified in the ... Vishesh Handa Aug. 30, 2015, 4:14 p.m.
Maybe we can just use '/'? This is getting a bit wordy, and is exposing the internal class name/structure. Vishesh Handa Aug. 25, 2015, 5:54 p.m.
This looks like a SiC change to me, no? e.g. plasma-desktop won't build with this commit Hrvoje Senjan Aug. 31, 2015, 5:22 p.m.
Aug. 25, 2015, 4:54 p.m.
Looks good. The only minor thing I realized is that I'm not sure how QtDbus behaves with interfaces which appear and disappear (contentIndexer).
Pinak Ahuja Aug. 25, 2015, 4:59 p.m.
ContentIndexer doesn't appear and disappear now, it's a long lived class, with this patch. Handling the monitor registration with an appearing and disappearing class would be a pain.
Maybe just 'fileIndexer' or some similar small word? This is quite the mouthfull.
This might not be thread safe.
It calls batchTimings which accesses m_batchTimeBuffer. This variable is also being modified in the run method which runs in another thread.
I'll need to check this out.
Pinak Ahuja Aug. 25, 2015, 5:54 p.m.
This is not thread safe, the solution would be to move all the time estimation code including the circular buffer to the main thread. Lets do this in a separate patch considering this behaviour was present before this patch.
Maybe we can just use '/'? This is getting a bit wordy, and is exposing the internal class name/structure.
I'll rename the objects to something smaller.
Aug. 25, 2015, 5:52 p.m.
Aug. 30, 2015, 1:58 p.m.
Aug. 30, 2015, 4:32 p.m.
Submitted with commit fa053a62c52a0630c6ee6f7b46f160c34dc1306a by Pinak Ahuja to branch master.
Aug. 31, 2015, 8:45 a.m.
Harald Sitter Aug. 31, 2015, 9:14 a.m.
Yeah unfortunately removing methods from the dbus interfaces and renaming the interfaces files (file.index -> fileindexer) is as SiC as it gets. In particular considering the interfaces are getting installed they are forming part of the public API.
Pinak Ahuja Aug. 31, 2015, 5:18 p.m.
I didn't realize messing around with the D-Bus interfaces would be a big deal. Forgive me for my ignorance, but what is a SiC change? From the discussion above, SiC change seems to be a change in the public API? What is the proper way to handle it.
Also I looked at plasma-desktop, it does generate a D-Bus interface for baloo in baloo kcm but doesn't seem to be using it? Should I just remove it from there?
Vishesh Handa Aug. 31, 2015, 5:21 p.m.
They are not part of the public API. I refuse to place additional restrictions which do not allow Baloo to evolve.
I grepped the plasma code, and it seems the KCM does depend on the interface in the dbus in the CMakeLists but does not actually use it anywhere. I've fixed it plasma-desktop 5.4 branch.
For this release, if you guys want we can still ship the old dbus interface, but it won't work.
Hrvoje Senjan Aug. 31, 2015, 5:54 p.m.
Vishesh Handa Aug. 31, 2015, 8:52 p.m.
It matters to me. Baloo guarentees ABI stability for the libraries and API stability for the C++ interfaces. That is all. If you want I can write this down in the README.
The dbus interface is going to break, it makes more sense to break it by removing the file instead of silently failing. However, considering that plasma-desktop does have a dependency on it, I can ship an empty file to satisfy the requirement. Perhaps for the next 6 months or so.
Harald Sitter Sept. 1, 2015, 8:16 a.m.
Talk to dfaure if he considers it ok to remove all interfaces and then remove all interfaces and replace the one pseudo-used in plasma with an empty file. Since they are not public API they ought not be installed so we'll eliminate this source of SIC headache for the future, and since plasma doesn't actively use the file it references, a stub place holder ought to prevent that from exploding.
David Faure Sept. 6, 2015, 9:59 p.m.
Yes, please do whatever is needed for the last plasma release to keep building with KF 5.14, even if that means installing an empty file.
For the hypothetical outside users, we'll just have to say "you were not supposed to use this", just like someone using a Qt private header isn't supposed to.
It was maybe less clear for the case of the baloo dbus interface, but anyway, it's clear now, and we indeed don't want to remain stuck due to that hypothetical case.
David Faure Sept. 9, 2015, 9:49 p.m.