D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler updated this revision to Diff 43074.
akrutzler marked 4 inline comments as done.
akrutzler added a comment.


  - Rebase
  - Reserve memory for lists/vectors in advance.
  - Prepend "static" to all class-level functions
  - Fix typos

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15277?vs=42685=43074

BRANCH
  mtp_next (branched from master)

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

AFFECTED FILES
  mtp/CMakeLists.txt
  mtp/devicecache.cpp
  mtp/devicecache.h
  mtp/filecache.cpp
  mtp/filecache.h
  mtp/kio_mtp.cpp
  mtp/kio_mtp.h
  mtp/kio_mtp_helpers.cpp
  mtp/kio_mtp_helpers.h
  mtp/kiod_module/CMakeLists.txt
  mtp/kiod_module/kmtpd.cpp
  mtp/kiod_module/kmtpd.h
  mtp/kiod_module/kmtpd.json
  mtp/kiod_module/mtpdevice.cpp
  mtp/kiod_module/mtpdevice.h
  mtp/kiod_module/mtpstorage.cpp
  mtp/kiod_module/mtpstorage.h
  mtp/shared/CMakeLists.txt
  mtp/shared/kmtpdeviceinterface.cpp
  mtp/shared/kmtpdeviceinterface.h
  mtp/shared/kmtpdinterface.cpp
  mtp/shared/kmtpdinterface.h
  mtp/shared/kmtpfile.cpp
  mtp/shared/kmtpfile.h
  mtp/shared/kmtpstorageinterface.cpp
  mtp/shared/kmtpstorageinterface.h
  mtp/shared/org.kde.kmtp.daemon.xml
  mtp/shared/org.kde.kmtp.device.xml
  mtp/shared/org.kde.kmtp.storage.xml

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure, mlaurent
Cc: mlaurent, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Great stuff. Only found some typos and very minor things, feel free to fix 
and push.

INLINE COMMENTS

> mtpdevice.cpp:119
> +{
> +QList list;
> +for (const MTPStorage *storage : m_storages) {

Technically this is missing a

  list.reserve(m_storages.count())

but I guess these lists are pretty small in practice...

> mtpstorage.cpp:31
> + */
> +uint16_t onDataPut(void *, void *priv, uint32_t sendlen, unsigned char 
> *data, uint32_t *putlen)
> +{

Prepend "static" to all these class-level functions (so they don't get 
exported, and so that the compiler knows they are only usable in this file, 
which can lead to more optimizations like inlining).

> kmtpdeviceinterface.cpp:35
> +const auto storageNames = m_dbusInterface->listStorages().value();
> +for (const QDBusObjectPath  : storageNames) {
> +m_storages.append(new KMTPStorageInterface(storageName.path(), 
> this));

m_storages.reserve(storageNames.count())

> kmtpstorageinterface.cpp:33
> +  this);
> +m_dbusInterface->setTimeout(5 * 60 * 1000); // TODO: listening folders 
> with a huge amount of files may take a while
> +

s/listening/listing/ ?

> org.kde.kmtp.daemon.xml:40
> +
> +

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

> mlaurent wrote in mtpstorage.cpp:176
> QString() directly no ?

Of course, thanks! :)

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure, mlaurent
Cc: mlaurent, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler updated this revision to Diff 42685.
akrutzler marked 5 inline comments as done.
akrutzler added a comment.


  - Use QString() directly instead of QStringLiteral("")
  - Use explicit specifier
  - Call either finished or error in slave methods

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15277?vs=42450=42685

BRANCH
  mtp_next

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

AFFECTED FILES
  mtp/CMakeLists.txt
  mtp/devicecache.cpp
  mtp/devicecache.h
  mtp/filecache.cpp
  mtp/filecache.h
  mtp/kio_mtp.cpp
  mtp/kio_mtp.h
  mtp/kio_mtp_helpers.cpp
  mtp/kio_mtp_helpers.h
  mtp/kiod_module/CMakeLists.txt
  mtp/kiod_module/kmtpd.cpp
  mtp/kiod_module/kmtpd.h
  mtp/kiod_module/kmtpd.json
  mtp/kiod_module/mtpdevice.cpp
  mtp/kiod_module/mtpdevice.h
  mtp/kiod_module/mtpstorage.cpp
  mtp/kiod_module/mtpstorage.h
  mtp/shared/CMakeLists.txt
  mtp/shared/kmtpdeviceinterface.cpp
  mtp/shared/kmtpdeviceinterface.h
  mtp/shared/kmtpdinterface.cpp
  mtp/shared/kmtpdinterface.h
  mtp/shared/kmtpfile.cpp
  mtp/shared/kmtpfile.h
  mtp/shared/kmtpstorageinterface.cpp
  mtp/shared/kmtpstorageinterface.h
  mtp/shared/org.kde.kmtp.daemon.xml
  mtp/shared/org.kde.kmtp.device.xml
  mtp/shared/org.kde.kmtp.storage.xml

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure, mlaurent
Cc: mlaurent, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Laurent Montel]

mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> mtpstorage.cpp:176
> +case LIBMTP_FILETYPE_UNDEF_AUDIO:
> +return QStringLiteral("");
> +case LIBMTP_FILETYPE_WMV:

QString() directly no ?

> mtpstorage.cpp:188
> +case LIBMTP_FILETYPE_UNDEF_VIDEO:
> +return QStringLiteral("");
> +case LIBMTP_FILETYPE_JPEG:

Same

> mtpstorage.cpp:240
> +case LIBMTP_FILETYPE_MHT:
> +return QStringLiteral("");
> +case LIBMTP_FILETYPE_JP2:

same

> kmtpdinterface.h:42
> +public:
> +KMTPDInterface(QObject *parent = nullptr);
> +

explicit

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure, mlaurent
Cc: mlaurent, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

> dfaure wrote in mtpstorage.cpp:499
> DBus timeouts are actually configurable, if you need to block until the copy 
> is done, btw.

In my previous approach I increased the timeout to 10 minutes. This worked fine 
until I downloaded a file of about 10GB or so from my phone -> timeout. It's 
hard to find a proper timeout which is why I chose the async way. :)

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler updated this revision to Diff 42450.
akrutzler marked 13 inline comments as done.
akrutzler added a comment.


  - Rebase
  - Remove unrelated changes
  - Remove Q_FUNC_INFO
  - Remove private static methods from header file
  - Use currentDateTimeUtc instead of currentDateTime
  - Generate D-Bus interface with qdbusxml2cpp

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15277?vs=41002=42450

BRANCH
  mtp_next

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

AFFECTED FILES
  mtp/CMakeLists.txt
  mtp/devicecache.cpp
  mtp/devicecache.h
  mtp/filecache.cpp
  mtp/filecache.h
  mtp/kio_mtp.cpp
  mtp/kio_mtp.h
  mtp/kio_mtp_helpers.cpp
  mtp/kio_mtp_helpers.h
  mtp/kiod_module/CMakeLists.txt
  mtp/kiod_module/kmtpd.cpp
  mtp/kiod_module/kmtpd.h
  mtp/kiod_module/kmtpd.json
  mtp/kiod_module/mtpdevice.cpp
  mtp/kiod_module/mtpdevice.h
  mtp/kiod_module/mtpstorage.cpp
  mtp/kiod_module/mtpstorage.h
  mtp/shared/CMakeLists.txt
  mtp/shared/kmtpdeviceinterface.cpp
  mtp/shared/kmtpdeviceinterface.h
  mtp/shared/kmtpdinterface.cpp
  mtp/shared/kmtpdinterface.h
  mtp/shared/kmtpfile.cpp
  mtp/shared/kmtpfile.h
  mtp/shared/kmtpstorageinterface.cpp
  mtp/shared/kmtpstorageinterface.h
  mtp/shared/org.kde.kmtp.daemon.xml
  mtp/shared/org.kde.kmtp.device.xml
  mtp/shared/org.kde.kmtp.storage.xml

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Andreas Krutzler]

akrutzler added a comment.


  Thanks for the review. I will address your comments and hopefully have a 
patch ready in the next days. :)

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kmtpd.json:10
> Currently the module is spawned by the ioslave on-demand. But imho it should 
> be possible to browse the device using only dbus, without having to open 
> dolphin first.
> 
> Can we try to autostart the module?

I don't understand what you mean here. How is a user supposed to "use only 
dbus"? ;-) Is this really about increasing overhead for every Plasma user (even 
those who never use mtp) for the benefit of an hypothetical command-line user?
I'm pretty sure qdbus can trigger on-demand loading too, even, since that's 
done by the dbus server, when using a dbus service file.

In case this is what you meant: please don't load the module at plasma startup. 
On demand is perfect. And it can be on demand of other apps than kioslaves, in 
case any app wants to do some MTP stuff (e.g. Amarok).

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  This is excellent work, thanks a lot for doing this. I just have "a few" 
minor comments... ;)

INLINE COMMENTS

> kio_mtp.cpp:107
>  {
> -qCDebug(LOG_KIO_MTP) << url.path();
> +qCDebug(LOG_KIO_MTP) << Q_FUNC_INFO << url.path();
>  

Don't use Q_FUNC_INFO in code.
Instead, set up your environment with %{function} in QT_MESSAGE_PATTERN.
For instance, mine says

'%{time h:mm:ss.zzz} %{appname}(%{pid}) %{if-category}%{category}: 
%{endif}%{if-debug}%{function}%{endif}%{if-warning}%{backtrace 
depth=3}%{endif}%{if-critical}%{backtrace 
depth=3}%{endif}%{if-fatal}%{backtrace depth=3}%{endif} %{message}'

See https://woboq.com/blog/nice-debug-output-with-qt.html for more info.

> kio_mtp.cpp:139
>  
> -getEntry(entry, device);
> -
> -listEntry(entry);
> -entry.clear();
> +for (const KMTPDeviceInterface *device : m_kmtpDaemon.devices()) {
> +listEntry(getEntry(device));

move m_kmtpDaemon.devices to a const local variable, to avoid a detach. Yes 
this is annoying.

> kio_mtp.cpp:320
> +const KMTPDeviceInterface *mtpDevice = 
> m_kmtpDaemon.deviceFromName(pathItems.first());
> +if (mtpDevice) {
> +KMTPStorageInterface *storage = 
> mtpDevice->storageFromDescription(pathItems.at(1));

and if mtpDevice is nullptr? This code doesn't seem to emit either error or 
finished, which is a bug.

Or if it can't be null, use Q_ASSERT instead of if() ;)

(Same in most other SlaveBase methods, please check that they all end up with 
either error() or finished())

> kio_mtp.cpp:338
> +});
> +connect(storage, ::copyFinished, , 
> ::exit);
> +const int result = loop.exec();

Let's hope this signal is always ALWAYS emitted, including in all error cases...

> kio_mtp.h:79
> +
> +static UDSEntry getEntry(const KMTPDeviceInterface *device);
> +static UDSEntry getEntry(const KMTPStorageInterface *storage);

class-static private functions pollute the header file for no benefit, they 
might as well become file-static functions (i.e. remove the declarations from 
here, and remove the classname from the implementation, and ensure they're at 
the top of the file).

> kmtpd.cpp:59
> +// Release devices
> +for (const MTPDevice *device : m_devices) {
> +deviceRemoved(device->udi());

qAsConst(m_devices) to avoid a detach

> kmtpd.cpp:128
> +
> +MTPDevice *KMTPd::deviceFromUdi(const QString )
> +{

this method could probably be const?

> kmtpd.cpp:153
> +if 
> (device.isDeviceInterface(Solid::DeviceInterface::PortableMediaPlayer)) {
> +qCDebug(LOG_KIOD_KMTPD) << "SOLID: New Device with udi=" << udi << 
> "";
> +

please clean up the "" stuff

> kmtpd.cpp:163
> +if (device) {
> +qCDebug(LOG_KIOD_KMTPD) << "SOLID: Device with udi=" << udi << " 
> removed. ";
> +

same

> kmtpd.cpp:165
> +
> +m_devices.removeAt(m_devices.indexOf(device));
> +delete device;

There's removeOne(device)

> mtpstorage.cpp:206
> +{
> +QDateTime dateTime = QDateTime::currentDateTime();
> +dateTime = dateTime.addSecs(timeToLive);

Any chance currentDateTimeUtc could be used everywhere? It's much faster than 
currentDateTime because it doesn't need to do timezone conversions (which use 
the awful process-global tzset()).

Just a thought, maybe you do need local times.

> mtpstorage.cpp:225
> +path.append(QLatin1Char('/'));
> +path.append(pathItems.at(i));
> +}

This method could be made much faster by locating the Nth slash and then using 
left(), to avoid the many reallocations due to multiple appends. Not sure it 
matters though.

> mtpstorage.cpp:256
> +
> +emit const_cast(static_cast *>(priv))->copyProgress(sent, total);
> +return LIBMTP_HANDLER_RETURN_OK;

Minor: this would be more readable with a MTPStorage * local variable, i.e. 
splitting this over two lines.

> mtpstorage.cpp:473
> +{
> +qCDebug(LOG_KIOD_KMTPD) << Q_FUNC_INFO << path;
> +

Please remove Q_FUNC_INFO everywhere.

> mtpstorage.cpp:499
> +
> +// big files take some time to copy, and this may lead into D-Bus 
> timeouts.
> +// therefore the actual copying is not done within the D-Bus method 
> itself but right after we return to the event loop

DBus timeouts are actually configurable, if you need to block until the copy is 
done, btw.

> mtpstorage.h:116
> + */
> +static uint16_t onDataPut(void *, void *priv, uint32_t sendlen, unsigned 
> char *data, uint32_t *putlen);
> +static int onDataProgress(const uint64_t sent, const uint64_t total, 
> const void * const priv);

you know what I think of private class-static methods ;-)

> kmtpstorageinterface.cpp:76
> +argumentList << 

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kmtpd.json:10
> +"X-KDE-DBus-ServiceName": "kmtpd",
> +"X-KDE-Kded-autoload": false,
> +"X-KDE-Kded-load-on-demand": true

Currently the module is spawned by the ioslave on-demand. But imho it should be 
possible to browse the device using only dbus, without having to open dolphin 
first.

Can we try to autostart the module?

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Elvis Angelaccio]

elvisangelaccio added a reviewer: dfaure.

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[Elvis Angelaccio]

elvisangelaccio added a comment.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.


  From a quick look, the architecture is sound and respects what was discussed 
in T9390 . And indeed it works pretty well, 
awesome job @akrutzler!
  
  I'd even say this could already be shipped because it's better than what we 
currently ship. But I do have a few remarks:
  
  - It should be possible to generate the xml files using `qdbuscpp2xml`. And 
we should actually do that and copy the generated files in the `shared` folder 
(to be sure there aren't e.g. typos).
  - This diff contains some unrelated changes, which should be moved to 
different commits. For example, porting to the JSON protocol file, `#include` 
changes, etc.

REPOSITORY
  R320 KIO Extras

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

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp