Differential D4305

Work round the DBus type casting bug
ClosedPublic
Actions

Authored by davidedmundson on Jan 27 2017, 9:18 AM.

Details

Reviewers
broulik
Commits
R245:6f1c6dc84a3d: Work round DBus property fetching bug
Summary

property() works in a slightly different way to just calling Get().
It allocates the variant of the relevant type in advance in QObject code, and then calls
the DBus code to populate it.

This fails for QByteArrayList, before it reaches DBus code.
I don't know why, and I should probably investigate..but we need an ASAP fix in Solid anyway.

Calling Get directly aleviates this problem.
m_device is a QDBusInterface making use of a magic DBus feature where it
aggregates all interfaces on that object path. This code therefor has to do the same as the GetAll().

From a DBus traffic perspective this code is identical, we just avoid going through Qt properties.

This shouldn't introduce any new bugs, as we already use GetAll directly, if anything this brings it more
in line.

Test Plan

Commented out the checkCache so that it always loads data.
Instead of failing, it now works.

Diff Detail

Repository
R245 Solid
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson updated this revision to Diff 10610.Jan 27 2017, 9:18 AM
davidedmundson retitled this revision from to Work round the DBus type casting bug.
davidedmundson updated this object.
davidedmundson edited the test plan for this revision. (Show Details)
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 27 2017, 9:18 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
davidedmundson added a reviewer: broulik.Jan 27 2017, 9:19 AM
davidedmundson removed a subscriber: Frameworks.
broulik edited edge metadata.Jan 27 2017, 12:34 PM

BUG: 345871

broulik added inline comments.Jan 30 2017, 9:29 AM
src/solid/devices/backends/udisks2/udisksdevicebackend.cpp
181

Whitespace

184–185

You still need to insert an invalid variant in the property cache if you didn't succeed here, the code uses that for optimization. With the new code Dolphin startup does 10x more DBus stuff than neccessary, noticeably slowing it down.

bool propertyFound = false;

foreach {
    ...
    if (reply.isValid()) {
        m_propertyCache.insert(...)
        propertyFound = true;
    }
}

if (!propertyFound) {
    m_propertyCache.insert(key, QVariant());
}
185

I don't fully understand why you need to iterate over something because the old code just does m_device->property(…) but perhaps QDBusInterface does that internally.

252

Whitespace

davidedmundson updated this revision to Diff 10702.Jan 30 2017, 10:23 AM
davidedmundson edited edge metadata.

Replaced for loop with setting interface to null, it's then up to the
client to loop through ifaces, which is quicker than multiple calls.

broulik accepted this revision.Jan 30 2017, 10:48 AM
broulik edited edge metadata.
This revision is now accepted and ready to land.Jan 30 2017, 10:48 AM
Closed by commit R245:6f1c6dc84a3d: Work round DBus property fetching bug (authored by davidedmundson). · Explain WhyJan 30 2017, 12:20 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 10712

View Options

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

Loading...