D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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=27395#toc REPOSITORY R245 Solid CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7401?vs=25846=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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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=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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
kfunk requested changes to this revision.
kfunk added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> iokitdevice.cpp:307
> +default:
> +vendor = QString();
> +break;
Why not `return` here as well? For consistency.
You can end the function with
Q_UNREACHABLE();
return {};
Very common pattern.
> iokitdevice.cpp:463
> {
> -return (type == Solid::DeviceInterface::GenericInterface
> -|| type == d->type);
> +bool ret = false;
> +switch (type) {
Returning inside the case statements would make this code clearer as well.
> iokitdeviceinterface.h:51
> IOKitDevice *m_device;
> +IOKitDevice *copy;
> };
`m_` prefix missing. Would call it `m_deviceCopy`.
> rjvbb wrote in iokitstorage.cpp:36
> Did you check that these are indeed pointers? ;)
Yes. And they are, no?
> rjvbb wrote in iokitstorage.cpp:75
> 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 ^^)
Okay, well, leave it like that.
I'm running out of time to properly explain how I'd envision this code to be
like in a perfect world.
> iokitstorage.cpp:73
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;
Inconsistent member naming. Some with `m_` prefix, some without. Choose one
style. Private classes' members usually live without the `m_` prefix, but we
don't mind them being around (in KDE land)
> iokitstorage.h:43
> +
> +const QString vendor();
> +const QString product();
Remove `const` from return type, but make the method `const`.
> iokitstorageaccess.cpp:56
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;
Dito, inconsistent member naming.
> iokitvolume.cpp:70
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;
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.
> iokitvolume.h:45
> +
> +virtual bool isIgnored() const Q_DECL_OVERRIDE;
> +virtual Solid::StorageVolume::UsageType usage() const Q_DECL_OVERRIDE;
No `virtual` needed if there's already a `Q_DECL_OVERRIDE` or `override`.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D7401
To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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=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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
kfunk added inline comments. INLINE COMMENTS > iokitopticaldrive.cpp:27 > + > +#ifdef EJECT_USING_DISKARBITRATION > +// for QCFType: 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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.
This definitely needs another revision. Please fix the outstanding issues.
INLINE COMMENTS
> iokitblock.h:43
> +
> +virtual int deviceMajor() const;
> +virtual int deviceMinor() const;
Here and below: Missing `Q_DECL_OVERRIDE`
> iokitdevice.cpp:151
> +size_t size = 0;
> +if (sysctlbyname("hw.model", NULL, , NULL, 0) == 0 && size > 0) {
> +model = new char [size];
Use `nullptr` everywhere
> iokitdevice.cpp:154
> +if (sysctlbyname("hw.model", model, , NULL, 0) == 0) {
> +qModel= QLatin1String(model);
> +}
Style: Use `qModel = ...`
> iokitdevice.cpp:156
> +}
> +delete model;
> +}
`new`/`delete` mismatch. Use `delete[]`
> iokitdevice.cpp:167
> +{
> +type << Solid::DeviceInterface::Unknown;
> +}
Initialize at class member decl
> iokitdevice.cpp:171
> +{
> +if (parent) {
> +delete parent;
That's *very* odd style. Why does a private class delete its public
counterpart? I've never seen this.
> iokitdevice.cpp:197
> +type = typesFromEntry(entry, properties, mainType);
> +// if (udi.contains(QStringLiteral("IOBD")) ||
> udi.contains(QStringLiteral("BD PX"))) {
> +// qWarning() << "Solid: BlueRay entry" << entry << "mainType=" <<
> mainType << "typeList:" << type
Don't leave commented code around. Either enable this code paths properly via
categorized logging or remove it altogether.
> iokitopticaldrive.cpp:211
> +}
> +delete ioDVDServices;
> +d = new Private(device, devCharMap);
Create on `ioDVDServices` on the stack to begin with?
> iokitopticaldrive.cpp:333
> +{
> +QList speeds;
> +return speeds;
`return {}`
> iokitopticaldrive.cpp:354
> +return true;
> +} else {
> +emit ejectDone(Solid::ErrorType::OperationFailed, QVariant(),
> m_device->udi());
Coding style: Would turn around this two branches (and remove the else part) to
make the intended meaning more clear:
Pseudo code:
if (errorCase) {
return ...;
}
// normal case, code flow continues
return ...;
That's usually the common style
> iokitprocessor.cpp:84
> +
> +const QString Processor::vendor()
> +{
Remove `const`
> iokitprocessor.cpp:87
> +QString qVendor = QString();
> +char *vendor = NULL;
> +size_t size = 0;
Way too much error prone `new`/`delete` logic on naked char arrays. Every
invocation here is wrong (causing undefined behavior) due to the
`new[]`/delete` mismatch mentioned above.
Factor that out reading into a `QString` into a separate function, cf.
https://github.com/trueos/lumina/blob/master/src-qt5/core/libLumina/LuminaOS-DragonFly.cpp#L29
and use it everywhere instead.
> iokitprocessor.cpp:99
>
> +const QString Processor::product()
> +{
Remove `const`
> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;
Here and one line below: Could be initialized at decl again.
> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;
`nullptr`
> iokitstorage.cpp:51
> +CFRelease(daDict);
> +daDict = 0;
> +}
`nullptr`, etc. pp.
> iokitstorage.cpp:75
> +
> +IOKitStorage::IOKitStorage(const IOKitDevice *device)
> +: Block(device)
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.
> iokitstorageaccess.cpp:41
> +} else {
> +daRef = 0;
> +}
Here and below: `nullptr`
> iokitstorageaccess.cpp:91
> +{
> +// mount points can change (but can they between invocations of
> filePath()?)
> +if (d->daRef) {
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
> iokitstorageaccess.cpp:132
> +return false;
> +} else {
> +// TODO?
Coding style: Eliminate `else` branch, just `return in function-level scope.
> iokitstorageaccess.cpp:142
> +return false;
> +} else {
> +// TODO?
Dito
> iokitvolume.cpp:42
> +} else {
> +daRef = 0;
> +}
Here and below: `nullptr`
> iokitvolume.cpp:121
> +if (dict) {
> +// qWarning() << Q_FUNC_INFO;
> +// CFShow(dict);
Remove commented code.
> iokitvolume.cpp:177
> +if (d->daRef) {
> +CFDictionaryRef dict = DADiskCopyDescription(d->daRef);
> +if (dict) {
That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product,
`description`. I'm sure that can be done better.
> iokitvolume.h:45
> +
> +virtual bool isIgnored() const;
> +virtual
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
rjvbb added a comment. 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. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D7401 To: rjvbb, #frameworks Cc: anthonyfieroni, cgilles, kde-mac
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
anthonyfieroni added inline comments.
INLINE COMMENTS
> cfhelper.cpp:51-56
> +static QVariant q_toVariant(const CFTypeRef , 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
> +case Solid::DeviceInterface::Processor:
> +return Processor::vendor();
> +break;
> +case Solid::DeviceInterface::Battery:
break after return is useless
> iokitdevice.cpp:375-377
> +return "computer-laptop";
> +} else {
> +return "computer";
QStringLiteral
> iokitmanager.cpp:95-98
> +return "IOMedia";
> +case Solid::DeviceInterface::OpticalDrive:
> +case Solid::DeviceInterface::OpticalDisc:
> +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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
rjvbb added a comment. Perfect, good to know. So, spoiler alert: I'll be pushing this (rebased and possibly polished where appropriate) somewhere this week unless anyone objects. REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D7401 To: rjvbb, #frameworks Cc: cgilles, kde-mac
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
rjvbb added a comment. 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... REPOSITORY R245 Solid REVISION DETAIL https://phabricator.kde.org/D7401 To: rjvbb, #frameworks Cc: cgilles, kde-mac
D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)
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
