> On June 24, 2015, 10:54 p.m., Stefan Brüns wrote: > > Some comments about the DBus interface you are exporting: > > I think you should make the current file name a property. Signal changes > > through the standard PropertiesChanged DBus signal. This way you can also > > add more properties later, but only use one common signal for property > > changes. It also allows a monitor client on startup to request the current > > file, without waiting for the next file. > > > > Instead of using an explicit flag for enabling/disabling the signaling, you > > should keep a list of registered monitors (you can get the name of the > > registering monitor with QDBusMessage::service()). Add a > > QDbusServiceWatcher to get notified if one of the monitors dies. > > Pinak Ahuja wrote: > Using property sounds like a good idea, it would certainly help in case > the extractor is stuck and we open the monitor to check which file it was > working with. Though I'm not sure if we need the complexity of keeping track > of running monitors as I can't think of any use case in which we have more > than one monitor running, though if such a need arises I can add that later > on.
My concern here is that a DBus Interface is public API, so it should be stable. I think you can leave the actual implementation for later, but make sure the interface is able to deal with this. For the beginning, just replace the enableExport()/disableExport() slots with (un)registerMonitor(const QDbusMessage &message). The function body can stay the same. If need arises, the boolean m_exportUrl can be replaced with a list of monitors. "if (m_monitorList.size() > 0) Q_EMIT startedWithUrl(url);". Tracking the monitor has the benefit you can be notified it the monitor dies. Multiple monitors may be useful for e.g. one monitor in the system tray and another one in a system settings module. - Stefan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124162/#review81736 ----------------------------------------------------------- On June 24, 2015, 11:52 a.m., Pinak Ahuja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124162/ > ----------------------------------------------------------- > > (Updated June 24, 2015, 11:52 a.m.) > > > Review request for Baloo and Vishesh Handa. > > > Repository: baloo > > > Description > ------- > > Extractor now has a proper D-Bus interface with methods to enable/disable url > export. The monitor calles enable export on startup or when extractor starts > and disables export when it is closed. > > > Diffs > ----- > > src/file/extractor/app.h f8b8834 > src/file/extractor/app.cpp e288285 > src/file/extractor/main.cpp 58ddbeb > src/file/extractor/org.kde.baloo.extractor.xml PRE-CREATION > src/tools/baloo-monitor/CMakeLists.txt b80d129 > src/tools/baloo-monitor/main.cpp 9f108ea > src/tools/baloo-monitor/monitor.h b72a8da > src/tools/baloo-monitor/monitor.cpp 9957424 > > Diff: https://git.reviewboard.kde.org/r/124162/diff/ > > > Testing > ------- > > Tested using dbus-monitor, extractor only exports urls when monitor is > running. > > > Thanks, > > Pinak Ahuja > >
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
