D19677: WIP: Port UDisks to using DBus ObjectManager
bruns added a comment. The UDisksDeviceBackend class serves two functions: - shared data cache for UDisksDevice instances - static factory of instances The manager has everything it needs to function as a factory. The UDisksDeviceBackend would become a representation of the individual UDisks Objects, and would store the interfaces and associated properties. The manager does no store interfaces or properties directly, but only objects, i.e. `QMap m_cache; m_cache[udi] = new UDisksDeviceBackend(interfaces_and_properties); The manager would also act as a dispatcher, demarshaling the DBus messages, and then e.g. calling on propertiesChanged `m_cache[udi]->propertiesChanged(QStringList invalidated, QVariantMap changed)` As the backend is a shared instance I don't think it can be merged with the device. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D19677 To: broulik, #frameworks, bruns Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
broulik added a comment. You mean I kill that entire `DeviceBackend` wrapper and have `Device` do all the things with all the data stored in the `DeviceManager`? REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D19677 To: broulik, #frameworks, bruns Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
bruns added a comment.
In the old code, the backend was created lazily, as the associated DBus
connection was expensive, this is no longer the case. I am wondering if
creating the backend immediately/always would not solve a number of structural
problems here (though this is already *much* better than what we have
curently). We could store the properties in the backend (non-flattened), and
return the flattened map on demand. For normal property lookups, we would just
walk the interfaces and lookup the key/value.
INLINE COMMENTS
> udisksdevicebackend.cpp:111
> {
> -QDBusMessage call = QDBusMessage::createMethodCall(UD2_DBUS_SERVICE,
> m_udi, DBUS_INTERFACE_PROPS, "GetAll");
> -
> -Q_FOREACH (const QString , m_interfaces) {
> -call.setArguments(QVariantList() << iface);
> -QDBusPendingReply reply =
> QDBusConnection::systemBus().call(call);
> -
> -if (reply.isValid()) {
> -auto props = reply.value();
> -// Can not use QMap<>::unite(), as it allows multiple values per
> key
> -for (auto it = props.cbegin(); it != props.cend(); ++it) {
> -m_propertyCache.insert(it.key(), it.value());
> -}
> -} else {
> -qCWarning(UDISKS2) << "Error getting props:" <<
> reply.error().name() << reply.error().message();
> +m_propertyCache.clear();
> +
Why is this always cleared? I think a `if (!m_propertyCache.isEmpty) { return;
}` would be correct here.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D19677
To: broulik, #frameworks, bruns
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
bruns added inline comments.
INLINE COMMENTS
> udisksdevicebackend.cpp:146
> if (m_propertyCache.contains(key)) {
> return;
> }
not necessary
> udisksdevicebackend.h:74
>
> QDBusInterface *m_device;
>
no longer used
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D19677
To: broulik, #frameworks, bruns
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
apol added a comment.
+1 do we know what's stopping this to move forward?
At least the commit message needs addressing.
INLINE COMMENTS
> udisksmanager.h:41
>
> +// interface -> [ {property: key}, {property: key}, ... ]
> +using PropertyMap = QMap;
This comment looks wrong. If this was the case it would be QList
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D19677
To: broulik, #frameworks, bruns
Cc: apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
bruns added a comment. +10 on porting to ObjectManager Will do a proper review later REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D19677 To: broulik, #frameworks, bruns Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19677: WIP: Port UDisks to using DBus ObjectManager
broulik retitled this revision from "Port UDisks to using DBus ObjectManager" to "WIP: Port UDisks to using DBus ObjectManager". REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D19677 To: broulik, #frameworks, bruns Cc: kde-frameworks-devel, michaelh, ngraham, bruns
