Re-organize D-Bus interfaces

Review Request #124919 - Created Aug. 25, 2015 and submitted

Information
Pinak Ahuja
baloo
master
Reviewers
baloo
vhanda
  • Previously mainhub class was being used to forward D-Bus calls to relevant objects
  • 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.

Issues

  • 0
  • 2
  • 2
  • 4
Description From Last Updated
Vishesh Handa
Vishesh Handa
Pinak Ahuja
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted with commit fa053a62c52a0630c6ee6f7b46f160c34dc1306a by Pinak Ahuja to branch master.
Hrvoje Senjan

   
src/dbus/CMakeLists.txt (Diff revision 2)
 
 

This looks like a SiC change to me, no? e.g. plasma-desktop won't build with this commit

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

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

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

  3. They are not part of the public API. I refuse to place additional restrictions which do not allow Baloo to evolve.

    Please see other similar reviews for another framework:
    https://git.reviewboard.kde.org/r/122926/
    https://git.reviewboard.kde.org/r/122970/

    If you install an interface, it's public, doesn't matter do you consider it such or no ;-)
    (It's not a matter 'do we want' it, but that's incorrect. There are possible users outside projects.kde.org.)

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

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

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

  7. Solved by https://git.reviewboard.kde.org/r/125124/

Loading...