WIP: Port UDisks to using DBus ObjectManager
Needs ReviewPublic

Authored by broulik on Mar 11 2019, 11:18 AM.

Details

Reviewers
bruns
Group Reviewers
Frameworks
Summary

Right now it introspects DBus and fetches properties all over the place.
This patch changes UDisks Manager to be the governor of all device data,
calling GetManagedObjects (which conveniently returns all the properties)
on startup. It also connects to change signals and updates the cache
accordingly and tells the devices about it.

This reduces impact of UDisks backend on Dolphin startup from 4.3% to 0.3%
on my machine. On my laptop it spent 130ms on startup trying to figure out
if something might be a optical drive or not...

Since I don't have an optical drive, it is likely that handling for these
is broken in this patch, at least one FIXME is definitely breaking it :)

While it sheds quite some code and compacts property caching in a single place
I'm not too happy about having this static Manager pointer in DeviceBackend,
with both Manager telling the Device about changed properties and the Device
asking the Manager about its properties.. Also, the Block directly accessing
the Manager is somehwat awful.

Fix updating existing device when it gains new interfaces

So many hours wasted finding this...

Fix monitoring optical drives

We already listen to all change events anyway so rather than explicitly connecting
to a certain path like the old one, we just process everything and try to figure out
if it was meant for an optical drive whose disc became available.

For the lack of an optical drive, I tested it in VirtualBox where attaching and
removing CD images worked fine now.

Diff Detail

Repository
R245 Solid
Branch
broulik/objectmanager
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9468
Build 9486: arc lint + arc unit
broulik created this revision.Mar 11 2019, 11:18 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 11 2019, 11:18 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.Mar 11 2019, 11:18 AM
broulik retitled this revision from Port UDisks to using DBus ObjectManager to WIP: Port UDisks to using DBus ObjectManager.Mar 11 2019, 11:18 AM
bruns added a comment.Mar 11 2019, 1:15 PM

+10 on porting to ObjectManager

Will do a proper review later

apol added a subscriber: apol.May 22 2019, 6:43 PM

+1 do we know what's stopping this to move forward?

At least the commit message needs addressing.

src/solid/devices/backends/udisks2/udisksmanager.h
41

This comment looks wrong. If this was the case it would be QList<QVariantMap>

bruns added inline comments.May 23 2019, 11:28 AM
src/solid/devices/backends/udisks2/udisksdevicebackend.cpp
146

not necessary

src/solid/devices/backends/udisks2/udisksdevicebackend.h
74

no longer used

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.

src/solid/devices/backends/udisks2/udisksdevicebackend.cpp
111

Why is this always cleared? I think a if (!m_propertyCache.isEmpty) { return; } would be correct here.

You mean I kill that entire DeviceBackend wrapper and have Device do all the things with all the data stored in the DeviceManager?

bruns added a comment.May 27 2019, 3:49 PM

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<QString, UDiskDeviceBackend*> 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.