hein added inline comments.
INLINE COMMENTS
> main.qml:247
> + onUrlsDropped: {
> + // if all dropped URLs point to application desktop files, we'll
> add a launcher for each of them
> + var createLaunchers = true;
I know this is nitpicky, but could you make sure code comments are generally
proper sentences with correct capitalization and punctuation? Errors are
errors, and I don't want to have to flag them all the time since it costs
"review energy" to feel bad for being nitpicky.
> main.qml:250
> + for (var i = 0, length = urls.length; i < length; ++i) {
> + if (!backend.isApplication(urls[i])) {
> + createLaunchers = false;
Does JS have anything like Python any/all we could use to avoid loop
boilerplate?
> main.qml:268
> + // as you probably don't expect some of your files to open in
> the app and others to spawn launchers
> + tasksModel.requestOpenUrls(hoveredItem.modelIndex(), urlsList);
> + }
Experience around the messyness of DND in Qt Quick tells me you want to add a
check on hoveredItem :)
> backend.cpp:346
> +{
> + if (!url.isValid() || !url.isLocalFile()) {
> + return false;
QUrl::isValid() famously won't return false for URLs that are actually invalid
in strict mode but were constructed in tolerant mode, so trusting it for QUrls
you didn't construct it is a bad idea. This probably doesn't matter here
because I hope KRun checks, but it's worth mentioning in case you want to be
more paranoid.
> backend.cpp:350
> +
> + const QString localPath = url.toLocalFile();
> +
This could be a const ref ... even if it's implicitly shared ...
REPOSITORY
rPLASMADESKTOP Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D2392
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: broulik, #plasma, #plasma:_design
Cc: hein, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
