[Notifications] Improve finished notification
AbandonedPublic

Authored by broulik on Mar 7 2019, 10:45 AM.

Details

Reviewers
dfaure
Group Reviewers
Plasma
VDG
Summary

Right now when a job finishes, we get a notification that just contains whatever file was copied last as summary, which isn't very useful. The "Open" button also just opens that file.
This patch adds some real-world heuristics for file transfer jobs extracting the total number of files processed.
When exactly one file is processed, the label is treated as destination file. If more than one file was processed, and the job's destination URL is known (ie. the parent folder all the files were supposed to be copied into), that one is used instead.
Moreover, a thumbnail is added for the file, so one can easily manipulate the file, open the containing folder of the file copied, or just access the destination parent folder.

Test Plan

Depends on D19583 D19585 D19586

Only jobs that set a totalAmount of Files will get this, which is currently only the file copy jobs in KIO. I did the same in p-b-i and get beautiful download finished notifications now.
However, I don't really know the implications of just setting a total amount of Files, I surely don't want to see a "0 of 1 files" or "1 of 1 files" in every file download... Plasma doesn't do this, but I don't know about the widget file copy dialog.

Copied a jpeg file to somewhere else


Context menu has all the goodies

Copied a folder. When copying a bunch of files the parent folder is quite useful. However, when copying a folder it shows the parent of that folder instead of the folder that I copied, which isn't super ideal but a lot better than status quo.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Mar 7 2019, 10:45 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 7 2019, 10:45 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rooty added a subscriber: rooty.EditedMar 7 2019, 11:07 AM

A few concerns:

Is this the JPG itself or a thumbnail?

The new icon doesn't seem to do much here - why not make "3689 files" a hyperlink with a context menu? Or replace the Dolphin icon with the "kiocl" icon (and its hamburger menu)?

Here too.

broulik added a comment.EditedMar 7 2019, 11:09 AM

Is this the JPG itself or a thumbnail?

A thumbnail

The new icon doesn't seem to do much here - why not make "3689 files" a hyperlink with a context menu? Or replace the Dolphin icon with the "kiocl" icon (and its hamburger menu)?

This is how the thumbnail stuff in notifications has been working ever since Plasma 5.9... it's also kinda modular so I cannot just randomly move it around. I mean, what I could in theory is when there's just a single icon without a thumbnail it shows some details about the file on the right.

rooty added a comment.Mar 7 2019, 11:22 AM

A thumbnail

Hmm... A privacy issue, but I suppose it's an acceptable compromise.
Is it possible to turn this thumbnail preview thing off?

This is how the thumbnail stuff in notifications has been working ever since Plasma 5.9... it's also kinda modular so I cannot just randomly move it around. I mean, what I could in theory is when there's just a single icon without a thumbnail it shows some details about the file on the right.

So we can't make the icon to the left of the summary right-clickable or add a hamburger menu to it?

anthonyfieroni added inline comments.
applets/notifications/package/contents/ui/Jobs.qml
117–120
displayDestUrl = destUrl.replace(/^(file:\/{2})/, "")
dfaure requested changes to this revision.Mar 7 2019, 1:10 PM
dfaure added inline comments.
applets/notifications/package/contents/ui/Jobs.qml
117–120

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 :-)

128

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().

This revision now requires changes to proceed.Mar 7 2019, 1:10 PM
broulik abandoned this revision.Apr 1 2019, 9:55 AM