BUG: 400423
BUG: 355594
FIXED-IN: 5.54
Details
- Reviewers
elvisangelaccio dfaure - Group Reviewers
Dolphin - Commits
- R241:ef57863a73c0: Display error instead of silently failing when asked to create folder that…
- mkdir ~/test
- In Dolphin, try to create ~/test: folder not created
- In Dolphin, try to create ~/test/foo: folder created
- In Dolphin, try to create ~/testo: folder created
Diff Detail
- Repository
- R241 KIO
- Branch
- dont-try-to-create-existing-dir (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 6065 Build 6083: arc lint + arc unit
can we change enter different name to just rename
for simplicity
or maybe i have something with large buttons 😀
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
915 | This breaks creation of non-local folders, because QDir(QString()) always exists. |
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.
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.
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. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
926 | Can you verify that if it has tabs it will display on current tab not in all. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
926 | It displays on the current tab, as expected. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
926 | I still think it's better to emit errorMessage instead. Can you check it will be more difficult ? |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
926 | I don't see how that makes sense. What's wrong with the current approach of expecting the jobs themselves to emit errors? |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
926 | 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
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.
Unfortunately this broke the knewfilemenu test, sorry about that. Here's a patch that fixes it: https://phabricator.kde.org/D17911