<table><tr><td style="">drosca requested changes to this revision.<br />drosca added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D21584">View Revision</a></tr></table><br /><div><div><p>After this, it should be good to go.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21584#inline-121540">View Inline</a><span style="color: #4b4d51; font-weight: bold;">drosca</span> wrote in <span style="color: #4b4d51; font-weight: bold;">gattapplication.h:47</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Maybe it would be best to add constructor that uses default object path. Also please add better documentation as to what object path prefix is.</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">explicit GattApplication(QObject *parent = nullptr);</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Here I meant adding new constructor, because as you did it now, if you want to set parent from constructor you would need to copy the default value for the objectPathPrefix.</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">GattApplication(QObject *parent = nullptr);
GattApplication(const QString &objectPathPrefix, QObject *parent = nullptr);</pre></div></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21584#inline-121399">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mweichselbaumer</span> wrote in <span style="color: #4b4d51; font-weight: bold;">objectmanager.h:47</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">QDbusAdaptors can only realize one single DBUS interface.<br />
This is the base class the org.freedesktop.DBus.ObjectManager adaptor is handling.<br />
Sure, this could (and should) be reused if other DBUS/BlueZ objects shall realize this interface.<br />
GattApplication is meant to realize the org.freedesktop.DBus.ObjectManager interface (see gatt-api.txt).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't really like this class at all. This is implementation detail, and is of no use for the users of the library, so it shouldn't be exported. On top of it, there is actually no code, it is just interface class.</p>
<p style="padding: 0; margin: 8px;">Another issue here, although minor, is that the bluezqt_dbustypes.h is not installed, so you can't use it. Yes, you could install it, but we don't need it.</p>
<p style="padding: 0; margin: 8px;">As I can see, the only reason for this class is so you can pass it to ObjectManagerAdaptor, but there are other ways to achieve the same thing without adding <tt style="background: #ebebeb; font-size: 13px;">ObjectManager</tt> and deriving from it in GattApplication. If we really need it in generic way, we can do it later.</p>
<p style="padding: 0; margin: 8px;">Something like this would work:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">ObjectManagerAdaptor(QObject *parent)
{
GattApplication *app = qobject_cast<GattApplication*>(parent);
}
DBusManagerStruct GetManagedObjects()
{
return app->d->getManagedObjects();
}</pre></div>
<p style="padding: 0; margin: 8px;">Or for now just make it work only with GattApplication, as this is the only one we have now.</p>
<p style="padding: 0; margin: 8px;">Yes, it will not work in the autotest, since you won't have the hook anymore, but you can just remove that test.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R269 BluezQt</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21584">https://phabricator.kde.org/D21584</a></div></div><br /><div><strong>To: </strong>mweichselbaumer, drosca<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns<br /></div>