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

[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

[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

Help fixing minor issue in notifications

[Karthik Periagaram]

Hello everyone,


I need some help in fixing this bug[1].


I have the plasma-browser integration add-on enabled in my browser and
when downloading from places like google drive or similar file
upload/sharing websites, the notification in the system tray is
structured like this,


Downloading

[Title] [collapse button]

Source: https://www.foo.com/long-meaningless-base64-string

Destination: /path/to/bar/meaningful-file-name.ext

[size] of [total size]

[speed] ([time] remaining)

[progress bar] [pause] [stop]


The problem is that the [Title] field is the long, meaningless base64
string at the end of the source URL and not the much more meaningful
file name I save the file to. I filed the bug under
plasma-browser-integration, but I suspect the right repo to fix this
in is actually plasma-workspace. However, my reasoning has some...
gaps, so I would appreciate someone confirming I'm on the right track.
Here's my thought process:


The trail starts in plasma-browser-integration where the JSON object
downloadItem[2] is parsed to get the url/finalUrl (source) and the
filename (destination). This creates a DownloadJob (derived of KJob),
emiting a description with the source, the destination and a title,
"Downloading"[3]. From here, "somehow", a KJob ends up as one of
several JobView[4] instances populating a KuiServerEngine[5], one of
Plasma's DataEngines. The KuiServerEngine provides a method to set up
data fields[6]. Again, "somehow", "something" calls this method
(surely not this other class also confusingly called JobView[7]?) but
it ends up setting "labelX", "labelNameX" and "labelFileNameX" for X=0
(source) and 1(destination). Now, these data fields are eventually
read by JobDelegate.qml[8] which goes on to create the [Title] by
slicing the "labelFileName0" (instead of "labelFileName1") after the
last '/' character[9].


I propose the following patch[10] to fix this bug, but due to the
afore-mentioned gaps in knowledge, I'm not sure I'm handling all
possible corner cases. Please chime in on the review for my patch if
you have any questions (or answers!).


Thanks,

Karthik


[1] https://bugs.kde.org/show_bug.cgi?id=396744

[2] https://developer.chrome.com/extensions/downloads#type-DownloadItem

[3] 
https://cgit.kde.org/plasma-browser-integration.git/tree/host/downloadjob.cpp#n81

[4] 
https://cgit.kde.org/plasma-workspace.git/tree/dataengines/applicationjobs/kuiserverengine.h#n59

[5] 
https://cgit.kde.org/plasma-workspace.git/tree/dataengines/applicationjobs/kuiserverengine.h#n36

[6] 
https://cgit.kde.org/plasma-workspace.git/tree/dataengines/applicationjobs/kuiserverengine.cpp#n256

[7] https://cgit.kde.org/plasma-workspace.git/tree/kuiserver/jobview.cpp#n214

[8] 
https://cgit.kde.org/plasma-workspace.git/tree/applets/notifications/package/contents/ui/JobDelegate.qml#n37

[9] 
https://cgit.kde.org/plasma-workspace.git/tree/applets/notifications/package/contents/ui/JobDelegate.qml#n68

[10] https://phabricator.kde.org/D14857

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