> 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. > > Stefan Brüns wrote: > 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. > > Pinak Ahuja wrote: > I've been thinking about this, you're probably right, we should keep > track of running monitors, had a similar implementation in mind. One > question, is there a Qt API to emit the standard propertiesChanged DBus > signal or do I have to manually construct the signal using > QDBusMessage::createSignal().
I don't think this is implemented in QtDBus or any of the KF5 addons. Have a look at the following two: https://randomguy3.wordpress.com/2010/09/07/the-magic-of-qtdbus-and-the-propertychanged-signal/ http://quickgit.kde.org/?p=amarok.git&a=blob&f=src%2Fdbus%2Fmpris2%2FDBusAbstractAdaptor.cpp The amarok implementation is slightly more complicated as it queues up the changed properties until entering the eventloop. As there is currently only one property, this is not needed. - 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 <<
