D17617: Display error instead of silently failing when asked to create folder that already exists

[Shubham]

shubham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham added a comment.


  Unfortunately this broke the `knewfilemenu` test, sorry about that. Here's a 
patch that fixes it: https://phabricator.kde.org/D17911

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:ef57863a73c0: Display error instead of silently failing 
when asked to create folder that… (authored by ngraham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17617?vs=47703=48500

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Elvis Angelaccio]

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


  In D17617#384214 , @ngraham wrote:
  
  > I don't have access to a webdav server to test with. Do you know of a 
public one?
  
  
  There is `share.kde.org` which you can access with your KDE identity account.
  
  > Also, this feels like an issue with the webdav ioslave, so we might want to 
fix that issue in another patch
  
  You're probably right. Feel free to land this patch if that's the case.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17617

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham added a comment.


  I don't have access to a webdav server to test with. Do you know of a public 
one?
  
  Also, this feels like an issue with the webdav ioslave, so we might want to 
fix that issue in another patch

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Much better now, but I noticed that with `webdavs://` the errore message does 
not show the name of the new folder:
  
A folder named  already exists.
  
  (while it works fine with `file://`, `ftp://` and `sftp://`).

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham added a comment.


  @elvisangelaccio?

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> ngraham wrote in knewfilemenu.cpp:916
> I don't see how that makes sense. What's wrong with the current approach of 
> expecting the jobs themselves to emit errors?

Makes sense because it's done in one standard way, possible change in future 
will be easy.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> ngraham wrote in knewfilemenu.cpp:916
> It displays on the current tab, as expected.

I still think it's better to emit errorMessage instead. Can you check it will 
be more difficult ?

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham marked 2 inline comments as done.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham marked an inline comment as done.
ngraham added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in knewfilemenu.cpp:916
> Can you verify that if it has tabs it will display on current tab not in all.

It displays on the current tab, as expected.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> knewfilemenu.cpp:916
>  job->uiDelegate()->setAutoErrorHandlingEnabled(true);
> -KIO::FileUndoManager::self()->recordJob(KIO::FileUndoManager::Mkpath, 
> QList(), url, job);
> +KJobWidgets::setWindow(job, m_parentWidget);
>  

Can you verify that if it has tabs it will display on current tab not in all.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: anthonyfieroni, dfaure, emateli, elvisangelaccio, Codezela, 
kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[David Faure]

dfaure accepted this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham updated this revision to Diff 47703.
ngraham marked 3 inline comments as done.
ngraham added a comment.


  Address review comments (thanks @dfaure!)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17617?vs=47699=47703

BRANCH
  arcpatch-D17617

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  BTW if you also want the mkpath case to error-if-already-exists, I guess this 
could be implemented in KIO::mkpath with a flag [and then the if()/else() isn't 
needed anymore, actually]. Not sure it's worth it though.

INLINE COMMENTS

> knewfilemenu.cpp:906
> +// If the name contains any slashes, use mkpath so that a/b/c works.
> +if (name.contains(QStringLiteral("/"))) {
> +job = KIO::mkpath(url, baseUrl);

QLatin1Char('/') would be enough

> knewfilemenu.cpp:908
> +job = KIO::mkpath(url, baseUrl);
> +job->setProperty("mkpathUrl", url);
> +KJobWidgets::setWindow(job, m_parentWidget);

(could have been renamed to something more generic, in all 3 places)

> knewfilemenu.cpp:910
> +KJobWidgets::setWindow(job, m_parentWidget);
> +job->uiDelegate()->setAutoErrorHandlingEnabled(true);
> +
> KIO::FileUndoManager::self()->recordJob(KIO::FileUndoManager::Mkpath, 
> QList(), url, job);

This could be moved out of the if()/else(), no? In fact, same thing for 
setWindow and setProperty. No point in duplicating these lines.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio, dfaure
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham marked an inline comment as done.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham updated this revision to Diff 47699.
ngraham edited the test plan for this revision.
ngraham added a comment.


  Use a better approach that works in all cases

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17617?vs=47649=47699

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: ngraham, #dolphin, elvisangelaccio
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[David Faure]

dfaure added a comment.


  What might be easier is:
  
  - if the user entered something without '/', use KIO::mkdir()   (which will 
fail with ERR_DIR_ALREADY_EXIST if the dir already exists)
  - if the user entered something with '/', use KIO::mkpath() as the code 
currently does.
  
  Much simpler and faster than an (async and racy) existence check before hand.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[David Faure]

dfaure added a comment.


  In D17617#378199 , @ngraham wrote:
  
  > Oh darn, now do I check for the existence of a directory in a 
network-transparent, KIO-ish way? Use a `listJob` or something?
  
  
  KIO::stat()

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: dfaure, emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Emirald Mateli]

emateli added a comment.


  FWIW: The patch over at D11304  changes 
the behavior to select the currently existing folder instead of doing nothing.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham added a comment.


  Oh darn, now do I check for the existence of a directory in a 
network-transparent, KIO-ish way? Use a `listJob` or something?

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Note that `mkdir` also doesn't warn if the folder already exists. I guess 
`KNewFileMenu` was designed to behave accordingly.

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin, elvisangelaccio
Cc: elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Hazem Salem]

Codezela added a comment.


  can we change enter different name to just rename
  for simplicity

REPOSITORY
  R241 KIO

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

To: ngraham, #dolphin
Cc: Codezela, kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

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

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham created this revision.
ngraham added a reviewer: Dolphin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  BUG: 400423
  FIXED-IN: 5.54

TEST PLAN
  1. `mkdir ~/test`
  2. In Dolphin, try to create `~/test`: folder not created
  3. In Dolphin, try to create `~/test/foo`: folder created
  4. In Dolphin, try to create `~/testo`: folder created

REPOSITORY
  R241 KIO

BRANCH
  dont-try-to-create-existing-dir (branched from master)

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

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