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, &KMTPStorageInterface::copyFinished, &loop, > &QEventLoop::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 &udi) > +{ 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<MTPStorage *>(static_cast<const MTPStorage > *>(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 << QVariant::fromValue(path); > + return callWithArgumentList(QDBus::Block, > QStringLiteral("getFileToHandler"), argumentList); > +} As others said, this file should really be generated by qdbusxml2cpp instead. 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