[Differential] [Changed Subscribers] D2392: [Task Manager] Allow dropping files onto tasks

[hein (Eike Hein)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160812/2252000f/attachment.html>