D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[René J.V. Bertin]

rjvbb updated this revision to Diff 18366.
rjvbb added a comment.


Don't add Block output where there shouldn't be.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7401?vs=18364&id=18366

REVISION DETAIL
https://phabricator.kde.org/D7401

AFFECTED FILES
src/solid/devices/CMakeLists.txt
src/solid/devices/backends/iokit/CMakeLists.txt
src/solid/devices/backends/iokit/cfhelper.cpp
src/solid/devices/backends/iokit/iokitbattery.cpp
src/solid/devices/backends/iokit/iokitbattery.h
src/solid/devices/backends/iokit/iokitblock.cpp
src/solid/devices/backends/iokit/iokitblock.h
src/solid/devices/backends/iokit/iokitdevice.cpp
src/solid/devices/backends/iokit/iokitdevice.h
src/solid/devices/backends/iokit/iokitdeviceinterface.cpp
src/solid/devices/backends/iokit/iokitdeviceinterface.h
src/solid/devices/backends/iokit/iokitgenericinterface.cpp
src/solid/devices/backends/iokit/iokitmanager.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.h
src/solid/devices/backends/iokit/iokitopticaldrive.cpp
src/solid/devices/backends/iokit/iokitopticaldrive.h
src/solid/devices/backends/iokit/iokitprocessor.cpp
src/solid/devices/backends/iokit/iokitprocessor.h
src/solid/devices/backends/iokit/iokitstorage.cpp
src/solid/devices/backends/iokit/iokitstorage.h
src/solid/devices/backends/iokit/iokitstorageaccess.cpp
src/solid/devices/backends/iokit/iokitstorageaccess.h
src/solid/devices/backends/iokit/iokitvolume.cpp
src/solid/devices/backends/iokit/iokitvolume.h

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[René J.V. Bertin]

rjvbb set the repository for this revision to R245 Solid.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[Gilles Caulier]

cgilles added a comment.


Rene, this is a great improvement for MacOS support. Congratualtions.

When code will be add to KDE::Solid framework ?

Best

Gilles Caulier

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[René J.V. Bertin]

I can commit it anytime, as soon as I get a green light (within a reasonable amount of time). Have you tested it?

I expect that the only possible risk with these modifications is that they might break the build itself on newer OS versions than I have myself. Other than that I don't think there's any code currently that relies on the existing Mac support in Solid...

[Gilles Caulier]

cgilles added a comment.


I don't yet tested, i'm currently busy to prepare digiKam 5.7.0, to review last code from GSoC 2017 students, and to prepare Randa event tasks.

Do you come to Randa ? If yes, we can take a look together while the event...

Gilles

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[René J.V. Bertin]

rjvbb edited the test plan for this revision.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[Gilles Caulier]

cgilles added a comment.


Renee,

I tested your patch against Solid under MacOS Sierra 10.12.6, and now digiKam can see USB devices. Gre

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: cgilles, kde-mac

[René J.V. Bertin]

Perfect, good to know.

So, spoiler alert: I'll be pushing this (rebased and possibly polished where appropriate) somewhere this week unless anyone objects.

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

cfhelper.cpp:51-56
+static QVariant q_toVariant(const CFTypeRef &obj, bool verbose=false)
{
const CFTypeID typeId = CFGetTypeID(obj);
+// if (verbose) {
+// qWarning() << "CFTypeID for obj" << obj << "=" << typeId << q_toString(CFCopyTypeIDDescription(typeId));
+// }

Remove verbose

iokitdevice.cpp:148
+{
+ QString qModel = QString();
+ char *model = NULL;

QString has a default constructor.

iokitdevice.cpp:149
+ QString qModel = QString();
+ char *model = NULL;
+ size_t size = 0;

nullptr even on C functions

iokitdevice.cpp:293-294
+ return Processor::vendor();
+ break;

break after return is useless

iokitdevice.cpp:375-377
+ return "computer-laptop";
+ } else {
+ return "computer";

QStringLiteral

iokitmanager.cpp:95-98
+ return "IOMedia";
+ return "IOCDMedia";

QStrinLiteral

iokitopticaldrive.h:33
+{
+class IOKitOpticalDrive : public IOKitStorage, virtual public Solid::Ifaces::OpticalDrive
+{

I see that all classes have a virtual inheritance but i don't see they are exported.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

Post by Anthony Fieroni

+class IOKitOpticalDrive : public IOKitStorage, virtual public Solid::Ifaces::OpticalDrive
+{

I see that all classes have a virtual inheritance but i don't see they are exported.

Indeed. I'm pretty sure that got there because it was already in code I cloned (for the simple reason this isn't a construct I'm familiar with =) ).

That said, there's inheritance inside the IOKit backend so this kind of inheritance could make sense even without exporting publicly.

[René J.V. Bertin]

rjvbb marked 7 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

anthonyfieroni wrote in iokitdevice.cpp:293-294
break after return is useless

I know, I do this as a matter of principle (and I'll leave it in since I'll undoubtedly be the principal maintainer of this code for the foreseeable future).

anthonyfieroni wrote in iokitmanager.cpp:95-98
QStrinLiteral

No, not here. Check the return type and what the returned strings are used for.

anthonyfieroni wrote in iokitopticaldrive.h:33
I see that all classes have a virtual inheritance but i don't see they are exported.

Did you see that this is also the case in the other backends, at least the ones I used for reference (hal and udisk)?

As I said, I'm not familiar enough with the construct to know what difference this would make at runtime.
Shouldn't changing this be the focus of a different patch and review, tackling all backends at once?

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks
Cc: anthonyfieroni, cgilles, kde-mac

[Kevin Funk]

This post might be inappropriate. Click to display it.

[Kevin Funk]

kfunk added inline comments.

INLINE COMMENTS

iokitopticaldrive.cpp:27
+
+#ifdef EJECT_USING_DISKARBITRATION

Where's that defined via the build system?

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb marked 27 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in iokitblock.h:43
Here and below: Missing `Q_DECL_OVERRIDE`

I hope you meant everywhere, including in old code...

kfunk wrote in iokitdevice.cpp:156
`new`/`delete` mismatch. Use `delete[]`

(This keeps biting me. Even C doesn't have separate de/allocators for pointers to scalars and pointers to arrays ...)

kfunk wrote in iokitdevice.cpp:171
That's *very* odd style. Why does a private class delete its public counterpart? I've never seen this.

Heh, that's because there is (was) a confusion in parenthood here. The member var didn't point to the private class parent, but holds a reference to the parent of the current IOKit device. Currently it's used and thus allocated only when getting the device icon.

kfunk wrote in iokitdevice.cpp:197
Don't leave commented code around. Either enable this code paths properly via categorized logging or remove it altogether.

I'd love to, but Solid doesn't have any modern logging set up. Rather than introducing that through the back(end) door, wouldn't it be better if this were done for all of Solid? (Preferably by someone having a good overview of the entire framework...)

https://bugs.kde.org/show_bug.cgi?id=386107

kfunk wrote in iokitopticaldrive.cpp:27
Where's that defined via the build system?

Nowhere currently. It's somewhat experimental code which uses the DiskArbitration SDK for ejection, instead of invoking an external (system) command.

It works (and it would thus be a pity to throw everything away) but as documented in the comment, there are a few issues that I haven't been able to iron out.

Making this a build option would certainly increase its visibility and thus (hopefully) incite some other Apple users to test it. Should I put one under the WITH_NEW_* options in the toplevel CMake file?

kfunk wrote in iokitstorage.cpp:36
`nullptr`

Did you check that these are indeed pointers? ;)

kfunk wrote in iokitstorage.cpp:75
Please just remove the constructors taking a `const IOKitDevice *device` and adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for pointer types to be `const` in such contexts. It just adds noise.

I don't get it, sorry, can you explain in more words how you'd want to see this changed? If I remove this extra ctor, I can no longer call `IOKitStorage(this).vendor()` in `IOKitDevice::vendor()` without extra code that's also going to add noise.

I get a warning when I remove the const attribute from `IOKitDevice::vendor()`, which suggests that I'd no longer be reimplementing a virtual method but adding a method instead.

All these "extra" ctors hand off the pointer to a "const this" to `DeviceInterface` which finally makes a deep copy. I find that cleaner than creating a deep copy of `this` everywhere needed and ensuring it gets deallocated (even via QPointers).
Unusual doesn't mean wrong (we're on Mac here, after all ^^)

kfunk wrote in iokitstorageaccess.cpp:91
Early-return would reduce the indentation level here [1].
if (errorCase)
// return early
return {};
}
// normal case
// ...
[1] https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

Compromise. Too many return points make functions hard to maintain (Apple code tends to be full of `goto bail;` instructions because of that; we're far here from the nesting you'd get without those in code using the QT SDK.)

kfunk wrote in iokitvolume.cpp:177
That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product, `description`. I'm sure that can be done better.

Somewhat.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb updated this revision to Diff 21187.
rjvbb marked 7 inline comments as done.
rjvbb added a comment.


Updated as requested/discussed.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7401?vs=18366&id=21187

REVISION DETAIL
https://phabricator.kde.org/D7401

AFFECTED FILES
src/solid/devices/CMakeLists.txt
src/solid/devices/backends/iokit/CMakeLists.txt
src/solid/devices/backends/iokit/cfhelper.cpp
src/solid/devices/backends/iokit/iokitbattery.cpp
src/solid/devices/backends/iokit/iokitbattery.h
src/solid/devices/backends/iokit/iokitblock.cpp
src/solid/devices/backends/iokit/iokitblock.h
src/solid/devices/backends/iokit/iokitdevice.cpp
src/solid/devices/backends/iokit/iokitdevice.h
src/solid/devices/backends/iokit/iokitdeviceinterface.cpp
src/solid/devices/backends/iokit/iokitdeviceinterface.h
src/solid/devices/backends/iokit/iokitgenericinterface.cpp
src/solid/devices/backends/iokit/iokitmanager.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.h
src/solid/devices/backends/iokit/iokitopticaldrive.cpp
src/solid/devices/backends/iokit/iokitopticaldrive.h
src/solid/devices/backends/iokit/iokitprocessor.cpp
src/solid/devices/backends/iokit/iokitprocessor.h
src/solid/devices/backends/iokit/iokitstorage.cpp
src/solid/devices/backends/iokit/iokitstorage.h
src/solid/devices/backends/iokit/iokitstorageaccess.cpp
src/solid/devices/backends/iokit/iokitstorageaccess.h
src/solid/devices/backends/iokit/iokitvolume.cpp
src/solid/devices/backends/iokit/iokitvolume.h

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb set the repository for this revision to R245 Solid.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

[Kevin Funk]

This post might be inappropriate. Click to display it.

[Gilles Caulier]

cgilles added a comment.


What's about this very important entry to improve Solid support under MacOS ? It will be integrated officially soon ?

Gilles Caulier

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb added a comment.


I'd hope so. I thought I was waiting for additional information but it seems I may have missed a notification that there was new feedback. Apologies if that's the case.

I'll need to find some time to go over this again.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb marked 10 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

kfunk wrote in iokitdevice.cpp:463
Returning inside the case statements would make this code clearer as well.

I don't share that opinion and think that multiple exit points do not make the code more efficient either (judging from stepping through the code in a debugger).

But whatever...

kfunk wrote in iokitvolume.cpp:70
Dito, inconsistent member naming.
And given that I've noticed that three times now, this again leads to the conclusion this is very repetitive code amongst . `{IOKitStorageAccess,IOKitVolume,IOKitStorage}::Private`.
Maybe there should be helper class for accessing a `CFDictionary` instead which all these classes use?
I'm not trying to piss you off René, but this is slightly sloppy coding which easy for the initial writer to do, but will bite us any time in the future when someone unfamiliar with the code needs to do fixes to this code and potentially fixes only one copy of these snippets. Please think about your architecture more carefully.

Not really certain why I didn't decide to this myself, I'm pretty certain it did cross my mind.

kfunk wrote in iokitvolume.h:45
No `virtual` needed if there's already a `Q_DECL_OVERRIDE` or `override`.

I guess that's safe here because IOKitOpticalDisc doesn't override any of these methods?

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb updated this revision to Diff 25846.
rjvbb marked 3 inline comments as done.
rjvbb added a comment.


updated as requested/discussed.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7401?vs=21187&id=25846

REVISION DETAIL
https://phabricator.kde.org/D7401

AFFECTED FILES
src/solid/devices/CMakeLists.txt
src/solid/devices/backends/iokit/CMakeLists.txt
src/solid/devices/backends/iokit/cfhelper.cpp
src/solid/devices/backends/iokit/dadictionary.cpp
src/solid/devices/backends/iokit/dadictionary_p.h
src/solid/devices/backends/iokit/iokitbattery.cpp
src/solid/devices/backends/iokit/iokitbattery.h
src/solid/devices/backends/iokit/iokitblock.cpp
src/solid/devices/backends/iokit/iokitblock.h
src/solid/devices/backends/iokit/iokitdevice.cpp
src/solid/devices/backends/iokit/iokitdevice.h
src/solid/devices/backends/iokit/iokitdeviceinterface.cpp
src/solid/devices/backends/iokit/iokitdeviceinterface.h
src/solid/devices/backends/iokit/iokitgenericinterface.cpp
src/solid/devices/backends/iokit/iokitmanager.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.h
src/solid/devices/backends/iokit/iokitopticaldrive.cpp
src/solid/devices/backends/iokit/iokitopticaldrive.h
src/solid/devices/backends/iokit/iokitprocessor.cpp
src/solid/devices/backends/iokit/iokitprocessor.h
src/solid/devices/backends/iokit/iokitstorage.cpp
src/solid/devices/backends/iokit/iokitstorage.h
src/solid/devices/backends/iokit/iokitstorageaccess.cpp
src/solid/devices/backends/iokit/iokitstorageaccess.h
src/solid/devices/backends/iokit/iokitvolume.cpp
src/solid/devices/backends/iokit/iokitvolume.h

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac

[René J.V. Bertin]

rjvbb set the repository for this revision to R245 Solid.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac

[Gilles Caulier]

cgilles added a comment.


PING again to KF5 core developpers ...

We need to advance with this very important patch to support properly Apple device with Solid. This will permit to use Apple hardware as well, as under Linux.

Please review, fix, and validate this non negligible job, done by René.

Thanks in advance.

Gilles Caulier

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac, michaelh

[René J.V. Bertin]

rjvbb added a comment.


Thanks Gilles.

Please remember that perfect is the enemy of good and that further improvements can always be applied down the road.

This patch touches only Mac and concerns very specific functionality that is used by only very few applications (besides digiKam I can only think of Dolphin and the KDE file dialogs). Given that I am inclined to committing it say next Friday if I don't get revision requests I cannot address by then.

REPOSITORY
R245 Solid

REVISION DETAIL
https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac, michaelh

[René J.V. Bertin]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:cbe5085a646e: Mac/IOKit backend: support for drives, discs and volumes (authored by rjvbb).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D7401?vs=25846&id=27395#toc

REPOSITORY
R245 Solid

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7401?vs=25846&id=27395

REVISION DETAIL
https://phabricator.kde.org/D7401

AFFECTED FILES
src/solid/devices/CMakeLists.txt
src/solid/devices/backends/iokit/CMakeLists.txt
src/solid/devices/backends/iokit/cfhelper.cpp
src/solid/devices/backends/iokit/dadictionary.cpp
src/solid/devices/backends/iokit/dadictionary_p.h
src/solid/devices/backends/iokit/iokitbattery.cpp
src/solid/devices/backends/iokit/iokitbattery.h
src/solid/devices/backends/iokit/iokitblock.cpp
src/solid/devices/backends/iokit/iokitblock.h
src/solid/devices/backends/iokit/iokitdevice.cpp
src/solid/devices/backends/iokit/iokitdevice.h
src/solid/devices/backends/iokit/iokitdeviceinterface.cpp
src/solid/devices/backends/iokit/iokitdeviceinterface.h
src/solid/devices/backends/iokit/iokitgenericinterface.cpp
src/solid/devices/backends/iokit/iokitmanager.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.cpp
src/solid/devices/backends/iokit/iokitopticaldisc.h
src/solid/devices/backends/iokit/iokitopticaldrive.cpp
src/solid/devices/backends/iokit/iokitopticaldrive.h
src/solid/devices/backends/iokit/iokitprocessor.cpp
src/solid/devices/backends/iokit/iokitprocessor.h
src/solid/devices/backends/iokit/iokitstorage.cpp
src/solid/devices/backends/iokit/iokitstorage.h
src/solid/devices/backends/iokit/iokitstorageaccess.cpp
src/solid/devices/backends/iokit/iokitstorageaccess.h
src/solid/devices/backends/iokit/iokitvolume.cpp
src/solid/devices/backends/iokit/iokitvolume.h

To: rjvbb, #frameworks, kfunk
Cc: ngraham, kfunk, anthonyfieroni, cgilles, kde-mac, michaelh