D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[Ahmad Samir]

ahmadsamir added a comment.


  This wasn't landed on master, but rather phabricator is confused by me 
pushing to work/ branch on gitlab :)

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[Ahmad Samir]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f2984ff28e72: [StatJob] Make mostLocalUrl ignore remote 
(ftp, http...etc) URLs (authored by ahmadsamir).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29782?vs=82974=83130#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29782?vs=82974=83130

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/tests/testtrash.h

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[David Faure]

dfaure added a comment.


  OK, I picked testtrash because kio_trash *is* a :local protocol. If it 
doesn't use UDS_LOCAL_PATH, at least it means the job will actually go through 
(no early filtering out). That's at least interesting to test too.
  
  Good that http will give no error. That's something else the test can check 
:-)
  
  kio_remote is part of kio and sets UDS_LOCAL_PATH, maybe it can be used for 
testing the case where we'll actually find a local path. I'm afraid there isn't 
much testing of that kioslave though, so this sounds like a bigger effort.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[Ahmad Samir]

ahmadsamir added a comment.


  I've just realised, we won't get an error with an http url; in that case we 
return the url the statjob was called on as-is and cancel the job.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[Ahmad Samir]

ahmadsamir added a comment.


  In D29782#672157 , @dfaure wrote:
  
  > Can you add a unittest for a KIO::mostLocalUrl() in testtrash.cpp (which is 
:local, so it should work)
  
  
  I don't see where the trash ioslave sets UDS_LOCAL_PATH; I think it doesn't 
set it, I could be wrong, though.
  
  > and another one in jobtest.cpp for a http URL (e.g. to test which error 
code we're getting, if any?)
  
  I'll look into that next.
  
  > + call mostLocalUrl() on a "normal" StatJob in testtrash.cpp
  
  Same as the first point :)
  
  [...]

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[David Faure]

dfaure added a comment.


  Can you add a unittest for a KIO::mostLocalUrl() in testtrash.cpp (which is 
:local, so it should work)
  and another one in jobtest.cpp for a http URL (e.g. to test which error code 
we're getting, if any?)
  
  + call mostLocalUrl() on a "normal" StatJob in testtrash.cpp
  
  Thanks.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29782: [StatJob] Make mostLocalUrl ignore remote (ftp, http...etc) URLs

[Ahmad Samir]

ahmadsamir updated this revision to Diff 82974.
ahmadsamir retitled this revision from "[StatJob] Change mostLocalUrl to only 
handle protocols with Class=:local" to "[StatJob] Make mostLocalUrl ignore 
remote (ftp, http...etc) URLs".
ahmadsamir edited the summary of this revision.
ahmadsamir edited the test plan for this revision.
ahmadsamir added a comment.


  Improve

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29782?vs=82959=82974

BRANCH
  l-most-local-url-local (branched from master)

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns