Don't create directory tree when a new folder has a '/' in the name
AbandonedPublic

Authored by shubham on Jan 27 2019, 9:01 AM.

Details

Summary

BUG: 296825
UNIX system does not support '/' in any valid identifier name and creates a directory tree instead, but for Windows it's the opposite.

This patch removes the behavior to create a directory tree when the user attempts to create a folder with '/' in its name on a UNIX-based system.

Test Plan
  1. Create new directory named a/b/c
  2. Inline error message appears

Inline error message:

Diff Detail

Repository
R241 KIO
Branch
dir
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7611
Build 7629: arc lint + arc unit
shubham created this revision.Jan 27 2019, 9:01 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 27 2019, 9:01 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
shubham requested review of this revision.Jan 27 2019, 9:01 AM
shubham edited the summary of this revision. (Show Details)Jan 27 2019, 9:05 AM
shubham retitled this revision from Don' allow '/' in new directory's name to Don't allow '/' in new directory's name.
ngraham requested changes to this revision.Jan 27 2019, 5:41 PM
ngraham added reviewers: Frameworks, Dolphin.
ngraham added inline comments.
src/filewidgets/knewfilemenu.cpp
905

If Windows allows slashes in the filename, shouldn't they just be a part of the filename and not create a directory tree? If so, then we don't even need this #ifdef condition at all; KIO::mkdir(url); will take care of the behavior on each individual platform

This revision now requires changes to proceed.Jan 27 2019, 5:41 PM
shubham updated this revision to Diff 50410.Jan 28 2019, 6:20 AM

Don't create directory tree on Windows, instead create a directory

shubham marked an inline comment as done.Jan 28 2019, 6:20 AM
shubham edited the test plan for this revision. (Show Details)

If there already exists a folder named a, then creating directory named a/b still works, and creates a child directory named b inside a.

shubham edited the test plan for this revision. (Show Details)Jan 28 2019, 6:31 AM
ngraham accepted this revision as: ngraham.Jan 29 2019, 4:09 PM
ngraham retitled this revision from Don't allow '/' in new directory's name to Don't create directory tree when a new folder has a '/' in the name.
ngraham edited the summary of this revision. (Show Details)
ngraham added reviewers: dfaure, elvisangelaccio, pino.
ngraham added subscribers: pino, elvisangelaccio, dfaure.

Makes sense to me. Please don't land this until you receive approval from @dfaure, @elvisangelaccio, @pino, or another experienced Frameworks dev.

This revision is now accepted and ready to land.Jan 29 2019, 4:10 PM

We might want to also consider improving the error message for this situation. I think it would be appropriate to do so in this patch.

shubham added a comment.EditedJan 29 2019, 4:11 PM

@ngraham Okay with changing error message string, please provide me with the best suited string.

How about "Could not create <folder path> because slashes are not allowed in file and folder names."

@ngraham The message you suggested is specific, on the other hand failure in creation of directory may also be due to the file/directory name reserved by OS (eg .. )

Then we should define a new error message specifically for this case. That's probably material for another patch.

ndavis added a subscriber: ndavis.EditedJan 29 2019, 6:25 PM

So if I want to make 2 folders at once, ~/a and ~/a/b, this patch removes that ability? If so, I don't like this change. It's a nice shortcut.

shubham added a comment.EditedJan 29 2019, 6:29 PM

@ndavis UNIX does not allow / in any valid identifier name.
Also looking at the bug report, it feels like people wanted to create a folder named "a/b/c" which UNEXPECTEDLY created directory tree. So in this, I have fix both the problems, first one which is not valid and the latter one which is unexpected behaviour.

markg added a subscriber: markg.Jan 29 2019, 8:39 PM

@ndavis UNIX does not allow / in any valid identifier name.
Also looking at the bug report, it feels like people wanted to create a folder named "a/b/c" which UNEXPECTEDLY created directory tree. So in this, I have fix both the problems, first one which is not valid and the latter one which is unexpected behaviour.

Define unexpected.

People that don't know much about technology but merely happen to use KDE (so that would be the ones that also call for help if a console opens) the making of a folder names "a/b/c" causing the tree to me made is indeed unexpected.
The more technical inclined people, the power users, will know that "/" is the directory separator and that typing "a/b/c" probably makes the whole tree. For those (and i bet those are still the majority of Plasma users) you would introduce a regression with this patch.

I'm not saying it should go or stay. I only ever used it a couple of times and one of them being when debugging something years ago.
I can imagine it to be useful for people who got used to it and now rely on it for their workflow.
I can also imagine it not feeling intuitive.

Fyi, mkdir "a/b/c" also doesn't work. mkdir -p "a/b/c" does!

I'm also a bit puzzled: are we removing a feature in order to show an error message instead?

ngraham requested changes to this revision.Jan 30 2019, 12:53 AM

It's rather ironic that in D18571, I endorse a hidden productivity booster feature, but here, I reject one. :)

I think I'm reconsidering. It's true that people get confused when they try to create folders with slashes in the name, but removing the feature entirely is probably not the best way to handle it.

Ideas?

This revision now requires changes to proceed.Jan 30 2019, 12:53 AM

-1 from me. I would prefer we stick to Unix methodologies.

KDE software is also known to be the more feature-rich and customizable options out there, and as mentioned commonly linked to power users. We would be wise to not introduce a feature removal/regression such as this.

It's rather ironic that in D18571, I endorse a hidden productivity booster feature, but here, I reject one. :)

I think I'm reconsidering. It's true that people get confused when they try to create folders with slashes in the name, but removing the feature entirely is probably not the best way to handle it.

Ideas?

Aren't you endorsing hidden productivity boosters in both?

Well now I am. :)

It's difficult to know the intentions of the user whether they wanted to create a single directory with slash in it or a directory tree. So this patch is stuck in between the two.

cfeck added a subscriber: cfeck.Jan 30 2019, 11:20 AM

It could ask what to do.

markg added a comment.Jan 30 2019, 3:24 PM

It's rather ironic that in D18571, I endorse a hidden productivity booster feature, but here, I reject one. :)

I think I'm reconsidering. It's true that people get confused when they try to create folders with slashes in the name, but removing the feature entirely is probably not the best way to handle it.

Ideas?

Well, one idea does com to mind. But it would be a lot of work!
The concept in Plasma is iirc still "strong by default, powerful when needed". Or something along those lines.

With that in mind, it could be an option to disable this by default and introduce a new tab in the dolphin settings page where you list the power features and allow the user to enable them all (or one by one).
It is a lot more code logic and does fall into the micro settings category (a place you normally don't want to be) but would be a possible solution in this case.

Not saying this is the right direction. Merely one idea.

Another idea is to warn the user but with a checkbox: "Don't show this warning again".

Here's an idea for an overall nicer user experience: instead of letting people choose whatever name they want in the new file & new folder dialogs and then displaying an error message (inline or in dialog form) once they click "OK", we could have the new file & new folder dialogs display an inline error message using a KMessageWidget if the name is invalid, and then disable the "OK" button until they change the name. Then for ambiguous cases like this one, it could display a warning message saying something along the lines of Since slashes are not permissible characters in folder names, this will create a folder tree rather than a folder named "a/b/c" or something like that. With this, we could remove all of those annoying pop-up dialog box errors we've added recently too.

Agree, something similar is being proposed over at D18384. These dialogs are getting out of hand.

andriusr added inline comments.
src/filewidgets/knewfilemenu.cpp
905

if this is a QUrl, then / is the separator on every platform (e.g.: file://C:/WINDOWS/System32)

Also, Windows does not allow filenames with either \ or /

Any more ideas?, I'm a bit confused

I'm working on an alternative user interface for this feature. Stay tuned!

@ngraham Are you working on it?

Yes, I plan to put it up for review today or tomorrow. Sorry for the long wait!

shubham abandoned this revision.Jun 19 2019, 4:35 PM