D13674: Make it possible to go up to root again

[Jaime Torres Amate]

jtamate added a comment.


  In D13674#288162 , @dfaure wrote:
  
  > somehow stub out any actual kioslave usage, and *just* test URLs.
  
  
  Please, check https://phabricator.kde.org/D13939

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[David Faure]

dfaure added a comment.


  The unittest fails in CI, please take a look.
  
  
https://build.kde.org/view/Frameworks/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.10/313/testReport/(root)/TestSuite/kiofilewidgets_kfilewidgettest/
  
  When I tried running the test locally, I got a prompt from sftp about 
verifying the identity of 127.0.0.1  not great for a unittest, to ask for 
user input.
  This also means the test is dependent on kio_sftp being installed? That's no 
good, it's not part of KIO, so it's not available for CI.
  
  Please use a URL that is sure to work (maybe FTP on a public site), or 
better, somehow stub out any actual kioslave usage, and *just* test URLs.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1cea9463f471: Make it possible to go up to root again 
(authored by jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13674?vs=36534&id=36611

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Jaime Torres Amate]

jtamate updated this revision to Diff 36534.
jtamate added a comment.


  Moved the comment.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13674?vs=36533&id=36534

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[David Faure]

dfaure added a comment.


  Thanks!

INLINE COMMENTS

> kfilewidgettest.cpp:226
> +
> +// When going up from file:///home/, it should become file:/// not 
> file:///home/user
> +void testCdUpToRoot()

This comment should move to line 220, since it matches that specific row of 
test data ;)

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Jaime Torres Amate]

jtamate updated this revision to Diff 36533.
jtamate marked an inline comment as done.
jtamate added a comment.


  Removed comment and added autotest.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13674?vs=36504&id=36533

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfilewidget.cpp:1543
>  // tokenize() expects it because it uses 
> QUrl::adjusted(QUrl::RemoveFilename)
> +// special cases, avoid transform ":///" into "",
> +// that in a file dialog is transformed into user home directory.

I think this comment only adds more confusion, because the code doesn't show "" 
anywhere, it's a side effect of creating an invalid url (or something, I 
actually wonder why file: would be invalid...)

IMHO it's clear without the added comment. "append slash if needed", and then 
the if checks if there's already a trailing slash.

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Albert Astals Cid]

aacid added a comment.


  Can this be  auto tested?

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Jaime Torres Amate]

jtamate updated this revision to Diff 36504.
jtamate marked an inline comment as done.
jtamate edited the summary of this revision.
jtamate added a comment.


  Your suggested code works.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13674?vs=36499&id=36504

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfilewidget.cpp:1547
>  QUrl u(url);
> -if (!u.path().isEmpty()) {
> +if (u != fileRootUrl && !u.path().isEmpty()) {
>  u.setPath(u.path() + '/');

Shouldn't this be generalized to all schemes? I.e.

  if (!u.path().isEmpty() && u.path() != "/")

We don't want to turn smb:/// into smb:.

But wait, we also simply don't want to append a slash if there was already one.
The comment says "if needed", the code doesn't check for that.

New suggestion:

  if (!u.path().isEmpty() && !u.path().endsWith('/'))

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13674: Make it possible to go up to root again

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  If u == "file:///", then u.setPath(u.path() + '/') results in u == "".
  Avoid making this wrong transformation.

TEST PLAN
  In a File open dialog, go to a nfts disk root, that results in currentUrl = 
file:///d//
  Before, when I pressed the upCd dir (^) I was redirected to my home 
directory, after I see the root directory.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns