[Notifications] Only keep job finished notification in case of an error
ClosedPublic

Authored by broulik on Jul 19 2016, 2:27 PM.

Details

Summary

As requested by Usability we'll assume a job finished successfully as the most likely case and only keep the notification persistent in case of an error. This makes dealing with files significantly less annoying.

Test Plan

Depends on D2222 (hooray, repdigit \o/)

selected file, Ctrl+C, Ctrl+V in the same folder, waited a bit for the job to show up, now:

  • Canceled transfer,, got a persistent "Copying: Failed" notification
  • Chose different file name, got a regular "Copying: Finished" notification, nothing in my history

Note: Technically canceling is an "error" and I didn't know any other way to trigger that but obviously we don't show a notification in this case as it would be pointless. This was just for testing and not part of this patch :)

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik updated this revision to Diff 5312.Jul 19 2016, 2:27 PM
broulik retitled this revision from to [Notifications] Only keep job finished notification in case of an error.
broulik updated this object.
broulik edited the test plan for this revision. (Show Details)
broulik added reviewers: Plasma, Plasma: Design.
broulik set the repository for this revision to R120 Plasma Workspace.
Restricted Application added a project: Plasma. · View Herald TranscriptJul 19 2016, 2:27 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

+1 from me (of course, since it was my request ;) )

graesslin added inline comments.
applets/notifications/package/contents/ui/Jobs.qml
88

why !!

broulik added inline comments.Jul 19 2016, 2:42 PM
applets/notifications/package/contents/ui/Jobs.qml
88

!! is the JavaScript-way of casting to a bool, error is the KIO error number.

I could do Boolean(error) or error ? true : false if you like. I do a toBool() in the notification action but I'd prefer being explicit here

graesslin accepted this revision.Jul 19 2016, 2:56 PM
graesslin added a reviewer: graesslin.

asking the testing question: how can we autotest this?

applets/notifications/package/contents/ui/Jobs.qml
88

ok, I wasn't aware of this language element. It looks weird (well all of JavaScript looks weird to my C++-eyes).

This revision is now accepted and ready to land.Jul 19 2016, 2:56 PM
mart added a subscriber: mart.Jul 19 2016, 5:03 PM

so i don't know what happened to the files i was copying, meh :(

mart added a comment.Jul 19 2016, 5:03 PM

(no, don't like it)

You do know, except it won't stick around forever (I got the habit of copying files, then clearing the notification history, it's so tedious). If something went wrong you'll either have the persistent error notification or a KIO error dialog anyway...

@mart If you started a file transfer job at some point and now you neither have a progress indicator anymore, nor see an error message, what is supposed to have happened other than that it succeeded?

Of all the file transfer job, the vast majority will complete successfully (hopefully, otherwise we have way bigger problems). Notifying about the success at the point it happens makes sense because it shows you "Ah good, now I can start working with the files at their destination!". However, long after that moment, if you see no indication of failure, you know it succeeded.

Now that the notification queue does what it's supposed to do again in Plasma 5.7, it's hugely annoying that it fills up with lots of "Coyping finished" messages that you have to manually remove although they tell you nothing you don't already know.

mart added a comment.Jul 19 2016, 5:12 PM

if they would auto clear, but still stay for some minutes, i would be fine

In D2223#41238, @mart wrote:

if they would auto clear, but still stay for some minutes, i would be fine

Even in some minutes, quite a few notifications can pile up in the queue if you do lots of operations.
This would only be acceptable if they could all be collapsed into a single notification, but I don't know if that is possible,

mart added a comment.Jul 19 2016, 6:29 PM

Even in some minutes, quite a few notifications can pile up in the queue if you do lots of operations.
This would only be acceptable if they could all be collapsed into a single notification, but I don't know if that is possible,

anyways, I trust your judgement on the matter! let's do it and see for a release how it works.

This revision was automatically updated to reflect the committed changes.