Review Request 119382: Port kio-mtp to KF5
Alexander Richardson
arichardson.kde at gmail.com
Fri Jul 25 16:17:40 UTC 2014
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119382/#review63168
-----------------------------------------------------------
CMakeLists.txt
<https://git.reviewboard.kde.org/r/119382/#comment43929>
QT_MIN_VERSION seems to be unintialized (empty) here
CMakeLists.txt
<https://git.reviewboard.kde.org/r/119382/#comment43930>
maybe use set(CMAKE_INCLUDE_CURRENT_DIR ON) instead?
devicecache.cpp
<https://git.reviewboard.kde.org/r/119382/#comment43932>
kError -> qCDebug? Shouldn't this be qCWarning or qCCritical? It seems to me that you can't use the device at all if that happens, and therefore it should be more than a debug message.
Same thing in next case statement.
devicecache.cpp
<https://git.reviewboard.kde.org/r/119382/#comment43933>
Same thing again (kError -> qCDebug)
kio_mtp.h
<https://git.reviewboard.kde.org/r/119382/#comment43934>
Use Q_DECL_OVERRIDE after the function instead of (or in addition to) virtual here and in the following lines
kio_mtp.cpp
<https://git.reviewboard.kde.org/r/119382/#comment43936>
I don't think you need the extra 1 line function here, just write the `url.adjusted(QUrl::StripTrailingSlash).path()` inline. Or if you keep the extra function and rename it to something else. With the current name I would expect it to return the full url without trailing slash, not just the path.
As far as I can see it is only used once, so not much reason to keep it.
- Alexander Richardson
On Juli 21, 2014, 1:54 nachm., Jan Grulich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119382/
> -----------------------------------------------------------
>
> (Updated Juli 21, 2014, 1:54 nachm.)
>
>
> Review request for KDE Frameworks and Philipp Schmidt.
>
>
> Repository: kio-mtp
>
>
> Description
> -------
>
> I ported kio-mtp to KF5 including:
> kDebug -> qCDebug
> KUrl -> QUrl
>
> And it doesn't need Kdelibs4Support.
>
> Also one thing which should be probably considered is including kio-mtp in kio-extras.
>
>
> Diffs
> -----
>
> kio_mtp_helpers.cpp 0f9ddbc
> CMakeLists.txt cb27440
> devicecache.h 1fa3932
> devicecache.cpp 54ca56f
> filecache.h 590e799
> filecache.cpp dfd4fe6
> kio_mtp.h b05f09b
> kio_mtp.cpp 6e587fa
> kio_mtp_helpers.h b66adbd
>
> Diff: https://git.reviewboard.kde.org/r/119382/diff/
>
>
> Testing
> -------
>
> My phone is visible in Dolphin and I was able to finally copy my photos from phone to my laptop.
>
>
> Thanks,
>
> Jan Grulich
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140725/ccf21bfa/attachment.html>
More information about the Kde-frameworks-devel
mailing list