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

Authored by ngraham on Dec 16 2018, 3:00 AM.

Details

Summary

BUG: 400423
BUG: 355594
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

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.Dec 16 2018, 3:00 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 16 2018, 3:00 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 16 2018, 3:00 AM
ngraham edited the test plan for this revision. (Show Details)Dec 16 2018, 3:01 AM
Codezela added a subscriber: Codezela.EditedDec 16 2018, 7:38 AM

can we change enter different name to just rename
for simplicity
or maybe i have something with large buttons 😀

elvisangelaccio requested changes to this revision.Dec 16 2018, 10:22 AM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
src/filewidgets/knewfilemenu.cpp
915

This breaks creation of non-local folders, because QDir(QString()) always exists.

This revision now requires changes to proceed.Dec 16 2018, 10:22 AM

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

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

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

dfaure added a subscriber: dfaure.Dec 16 2018, 8:45 PM

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

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.

ngraham updated this revision to Diff 47699.Dec 16 2018, 9:30 PM
ngraham edited the test plan for this revision. (Show Details)

Use a better approach that works in all cases

ngraham marked an inline comment as done.Dec 16 2018, 9:32 PM
dfaure requested changes to this revision.Dec 16 2018, 10:22 PM

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.

src/filewidgets/knewfilemenu.cpp
906

QLatin1Char('/') would be enough

908

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

910

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

This revision now requires changes to proceed.Dec 16 2018, 10:22 PM
ngraham updated this revision to Diff 47703.Dec 16 2018, 11:06 PM
ngraham marked 3 inline comments as done.

Address review comments (thanks @dfaure!)

dfaure accepted this revision.Dec 17 2018, 1:33 PM
anthonyfieroni added inline comments.
src/filewidgets/knewfilemenu.cpp
916

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

ngraham marked an inline comment as done.Dec 17 2018, 2:41 PM
ngraham added inline comments.
src/filewidgets/knewfilemenu.cpp
916

It displays on the current tab, as expected.

ngraham marked 2 inline comments as done.Dec 17 2018, 2:42 PM
anthonyfieroni added inline comments.Dec 17 2018, 2:43 PM
src/filewidgets/knewfilemenu.cpp
916

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

ngraham added inline comments.Dec 17 2018, 2:58 PM
src/filewidgets/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?

anthonyfieroni added inline comments.Dec 17 2018, 3:48 PM
src/filewidgets/knewfilemenu.cpp
916

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

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://).

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

elvisangelaccio accepted this revision.Jan 1 2019, 10:32 PM

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.

This revision is now accepted and ready to land.Jan 1 2019, 10:32 PM
This revision was automatically updated to reflect the committed changes.

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

shubham edited the summary of this revision. (Show Details)Jan 9 2019, 5:58 PM