D19588: [Notifications] Improve finished notification

[David Faure]

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

INLINE COMMENTS

> anthonyfieroni wrote in Jobs.qml:117-120
>   displayDestUrl = destUrl.replace(/^(file:\/{2})/, "")

All of this is wrong. Removing file:/// leaves something that is neither a correct path nor a correct URL.

You want destUrl.toString(QUrl::PreferLocalFile).

This code being javascript is no excuse -- move all this to C++ code :-)

> Jobs.qml:128
> +                    if (url[0] === "/") {
> +                        url = "file://" + url;
> +                    }

Nooooooo.
This is broken. Testcase: try a filename with a '#' in it.

If the input is indeed a "path or URL" (urgh, that's never good practice, other than for showing to the user), then the way to turn this into a URL is QUrl::fromUserInput().

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, #vdg, dfaure
Cc: anthonyfieroni, rooty, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190307/811221df/attachment.html>