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
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
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,
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
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
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
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
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,
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
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
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
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
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,
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
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
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
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 <<
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 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
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
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
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,
22 matches
Mail list logo
