Details
Diff Detail
- Repository
- R320 KIO Extras
- Branch
- mtp_next
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 3405 Build 3423: arc lint + arc unit
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.
| mtp/kiod_module/kmtpd.json | ||
|---|---|---|
| 11 | 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? | |
This is excellent work, thanks a lot for doing this. I just have "a few" minor comments... ;)
| mtp/kio_mtp.cpp | ||
|---|---|---|
| 160–161 | Don't use Q_FUNC_INFO in code. '%{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. | |
| 191–193 | move m_kmtpDaemon.devices to a const local variable, to avoid a detach. Yes this is annoying. | |
| 444 | 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()) | |
| 455 | Let's hope this signal is always ALWAYS emitted, including in all error cases... | |
| mtp/kio_mtp.h | ||
| 98 | 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). | |
| mtp/kiod_module/kmtpd.cpp | ||
| 60 | qAsConst(m_devices) to avoid a detach | |
| 129 | this method could probably be const? | |
| 154 | please clean up the "||||" stuff | |
| 164 | same | |
| 166 | There's removeOne(device) | |
| mtp/kiod_module/mtpstorage.cpp | ||
| 207 | 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. | |
| 226 | 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. | |
| 257 | Minor: this would be more readable with a MTPStorage * local variable, i.e. splitting this over two lines. | |
| 474 | Please remove Q_FUNC_INFO everywhere. | |
| 500 | DBus timeouts are actually configurable, if you need to block until the copy is done, btw. | |
| mtp/kiod_module/mtpstorage.h | ||
| 117 | you know what I think of private class-static methods ;-) | |
| mtp/shared/kmtpstorageinterface.cpp | ||
| 77 | As others said, this file should really be generated by qdbusxml2cpp instead. | |
| mtp/kiod_module/kmtpd.json | ||
|---|---|---|
| 11 | 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? 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). | |
Thanks for the review. I will address your comments and hopefully have a patch ready in the next days. :)
- 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
| mtp/kiod_module/mtpstorage.cpp | ||
|---|---|---|
| 500 | 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. :) | |
- Use QString() directly instead of QStringLiteral("")
- Use explicit specifier
- Call either finished or error in slave methods
| mtp/kiod_module/mtpstorage.cpp | ||
|---|---|---|
| 177 | Of course, thanks! :) | |
Great stuff. Only found some typos and very minor things, feel free to fix and push.
| mtp/kiod_module/mtpdevice.cpp | ||
|---|---|---|
| 119 | Technically this is missing a list.reserve(m_storages.count()) but I guess these lists are pretty small in practice... | |
| mtp/kiod_module/mtpstorage.cpp | ||
| 31 | 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). | |
| mtp/shared/kmtpdeviceinterface.cpp | ||
| 35 | m_storages.reserve(storageNames.count()) | |
| mtp/shared/kmtpstorageinterface.cpp | ||
| 33 | s/listening/listing/ ? | |
| mtp/shared/org.kde.kmtp.daemon.xml | ||
| 40 | s/deamon/daemon/ | |
- Rebase
- Reserve memory for lists/vectors in advance.
- Prepend "static" to all class-level functions
- Fix typos
Great! Is there anything else left to do @elvisangelaccio and @mlaurent before I land this?