D14857: Label job notifications with destination file name

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:499d145e1a36: Label job notifications with destination 
file name (authored by karthikp, committed by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14857?vs=39781=39905

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

AFFECTED FILES
  applets/notifications/package/contents/ui/JobDelegate.qml

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Nathaniel Graham]

ngraham added a comment.


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

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

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Karthik Periagaram]

karthikp added a comment.


  I see, thanks! Here's my name and email,
  
  Karthik Periagaram
  karthik.periaga...@gmail.com

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

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Nathaniel Graham]

ngraham added a comment.


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

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

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Karthik Periagaram]

karthikp added a comment.


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

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

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Andres Betts]

abetts accepted this revision.
abetts added a comment.


  +1

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

To: karthikp, #vdg, ngraham, abetts
Cc: abetts, ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D14857: Label job notifications with destination file name

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  +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.

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

To: karthikp, #vdg, ngraham
Cc: ngraham, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14857: Label job notifications with destination file name

[Karthik Periagaram]

karthikp updated this revision to Diff 39781.
karthikp added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14857?vs=39775=39781

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

AFFECTED FILES
  applets/notifications/package/contents/ui/JobDelegate.qml

To: karthikp, #vdg
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14857: Label job notifications with destination file name

[Kai Uwe Broulik]

broulik added a comment.


  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.

INLINE COMMENTS

> JobDelegate.qml:70
>  text: {
> -var label = labelFileName0;
> +var label = labelFileName1? labelFileName1 : labelFileName0;
>  var lastSlashIdx = label.lastIndexOf("/");

Could be simplified to

  var label = labelFileName1 || labelFileName0;

REPOSITORY
  R120 Plasma Workspace

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

To: karthikp
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14857: Label job notifications with destination file name

[Karthik Periagaram]

karthikp created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
karthikp requested review of this revision.

REVISION SUMMARY
  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.

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/notifications/package/contents/ui/JobDelegate.qml

To: karthikp
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart