Label job notifications with destination file name

Authored by karthikp on Aug 15 2018, 9:25 AM.



If a destination file name is present, it is a better label for a job
than the source file name. This is particularly true for web downloads
where the source file name may be a gibberish base64 string while the
destination file name would be selected by the user.

If there is no destination file name, we fall back to using the source
file name.

BUG: 396744

Test Plan

Compiles, built-in tests pass. Not sure about coverage.

Diff Detail

R120 Plasma Workspace
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
karthikp created this revision.Aug 15 2018, 9:25 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 15 2018, 9:25 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
karthikp requested review of this revision.Aug 15 2018, 9:25 AM

Thanks for your patch and explanation on the mailing list!

The idea was mostly to show "which file it is currently copying" but I can see there are more cases where the source is weird than to where the destination is wrong?
I'll add VDG to get some usability/design feedback on that one.


Could be simplified to

var label = labelFileName1 || labelFileName0;
broulik added a reviewer: VDG.Aug 15 2018, 9:49 AM
karthikp updated this revision to Diff 39781.Aug 15 2018, 10:02 AM


Thanks, I'm not confident in my javascript powers... yet. :)

ngraham accepted this revision.Aug 16 2018, 8:54 PM
ngraham added a subscriber: ngraham.

+1, dest filename makes more sense IMHO. Semantically, the whole point of the move or copy operation is to end up with a destination file, so that's the file that should be focused on--whatever its name is.

This revision is now accepted and ready to land.Aug 16 2018, 8:54 PM
abetts accepted this revision.Aug 16 2018, 10:24 PM
abetts added a subscriber: abetts.


karthikp marked an inline comment as done.Aug 16 2018, 10:27 PM

Noob question (this is my first patch through phabricator and I don't have commit rights, naturally):
How does one normally proceed now, do I mail or attach here, the patch (as in git format-patch) so someone can git am -s it into the next release's dev branch? Does phabricator do that automatically ("land" it)? Should I create a task for it on some workboard (or is that thinking backwards)?

So now that it's gone through the VDG (Visual Design Group) review, it should go through a code review and final sign-off by the maintainer. Looks like Kai has already done the former, so if he's satisfied now, I'd say it can land! Because you submitted this patch using the web interface, we will need you to provide your real name and email address before landing it for you. Can you provide those in a comment here, please? Nothing further is needed beyond that; we will land it for you if it's ultimately approved by the Plasma developers (which appears likely at this point!).

I see, thanks! Here's my name and email,

Karthik Periagaram

Thank you very much! I'll let @broulik or any of the other Plasma developers take this show from here. :)

This revision was automatically updated to reflect the committed changes.