Allow creating directory named '~' and throw a warning before creating it.
Needs ReviewPublic

Authored by shubham on Jan 19 2019, 2:45 PM.

Details

Reviewers
ngraham
Summary

BUG: 377978

Test Plan
  1. Create a directory named '~'.
  2. Warning dialog appears.

Diff Detail

Repository
R241 KIO
Branch
dir
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7345
Build 7363: arc lint + arc unit
shubham created this revision.Jan 19 2019, 2:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 19 2019, 2:45 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
shubham requested review of this revision.Jan 19 2019, 2:45 PM
shubham edited the test plan for this revision. (Show Details)Jan 19 2019, 2:47 PM
shubham retitled this revision from Give warning before creating directory named '~' escaped by '\' to Throw warning before creating directory named '~' escaped by '\'.Jan 19 2019, 2:51 PM
shubham updated this revision to Diff 49883.Jan 19 2019, 2:59 PM
This comment was removed by shubham.
ngraham requested changes to this revision.Jan 19 2019, 3:00 PM

Neat idea.

src/filewidgets/knewfilemenu.cpp
439

That's not accurate; the dir just begins with ~.

441

Hmm, that might be too technical an explanation. How about this instead?

Beginning a file name with a tilde character ("~") is not recommended because it may be confusing or dangerous if files are deleted using the terminal.

906

Now the name of this function is no longer accurate since you're using it to display a message for a name that wouldn't result in in the file or folder being hidden.

This revision now requires changes to proceed.Jan 19 2019, 3:00 PM
shubham retitled this revision from Throw warning before creating directory named '~' escaped by '\' to Throw warning before creating directory named '~' .Jan 19 2019, 3:00 PM
shubham edited the test plan for this revision. (Show Details)
ngraham added inline comments.Jan 19 2019, 3:01 PM
src/filewidgets/knewfilemenu.cpp
439

Oops, never mind, ignore this.

ngraham added inline comments.Jan 19 2019, 3:04 PM
src/filewidgets/knewfilemenu.cpp
441

Actually maybe this:

Naming a folder \$1\ is not recommended because it may be confusing or dangerous when using the terminal to delete things.

This comment was removed by shubham.
shubham marked an inline comment as done.Jan 19 2019, 3:05 PM
shubham updated this revision to Diff 49885.Jan 19 2019, 3:15 PM
shubham retitled this revision from Throw warning before creating directory named '~' to Throw warning before creating directory named '~'.
  1. Better error message string.
  2. Change name of the function.
shubham marked an inline comment as done.Jan 19 2019, 3:15 PM
shubham edited the test plan for this revision. (Show Details)Jan 19 2019, 3:23 PM
shubham updated this revision to Diff 49888.EditedJan 19 2019, 3:24 PM

Change error string.

ngraham requested changes to this revision.Jan 20 2019, 1:51 AM

My string change request isn't addressed, and the string still doesn't end with a period.

This revision now requires changes to proceed.Jan 20 2019, 1:51 AM
shubham updated this revision to Diff 49914.Jan 20 2019, 3:35 AM

Use error string message as per recommendation.

shubham edited the test plan for this revision. (Show Details)Jan 20 2019, 3:36 AM
shubham retitled this revision from Throw warning before creating directory named '~' to Allow creating directory named '~' and throw a warning before creating it..Jan 20 2019, 7:14 AM
dhaumann added inline comments.
src/filewidgets/knewfilemenu.cpp
438

Please simply write name == QStringLiteral (...) instead of operator==. Same below.

shubham added inline comments.Jan 20 2019, 12:13 PM
src/filewidgets/knewfilemenu.cpp
438

This is faster than simple ==

dhaumann added inline comments.Jan 20 2019, 1:37 PM
src/filewidgets/knewfilemenu.cpp
438

Really? But isn't == not exactly the same? Can you elaborate?

pino added a subscriber: pino.Jan 20 2019, 1:53 PM

Also, not related to the code: @shubham, you seem to often remove your own comments. This is a bad practice for many POV of views (transparency, breaks the logic of a conversation, etc). As these reviews send notification emails to mailing lists usually, then your removed messages are archived, and thus removing them is useless.
Please stop doing this anti-social practice, thank you.

src/filewidgets/knewfilemenu.cpp
408–413

renaming confirmCreatingHiddenDir and using it also for another case means that the "confirm_create_hidden_dir" messagebox confirmation also applies to the other cases (filename == '~' in this case)

910

extra line added

shubham added inline comments.Jan 20 2019, 1:56 PM
src/filewidgets/knewfilemenu.cpp
438
==

internally calls

operator==()
pino added inline comments.Jan 20 2019, 1:58 PM
src/filewidgets/knewfilemenu.cpp
438

The compiler automatically replaces the operators into the calls to the appropriate functions. There is no speed penalty.

shubham added a comment.EditedJan 20 2019, 1:59 PM

@pino Sorry for inconvenience caused. BTW I did not get what you meant by inline commnet at 408?

@pino got it what you meant. Also do I need to change the _k_slotCreateHiddenDirectory( ) slot name?

shubham updated this revision to Diff 49935.Jan 20 2019, 2:23 PM
  1. Use ==
  2. Rename slot's name
shubham marked 3 inline comments as done.Jan 20 2019, 2:24 PM
pino added a comment.Jan 20 2019, 2:37 PM

@pino got it what you meant.

Definitely not. Let me explain it again:

  • so far, KNewFileMenuPrivate::confirmCreatingHiddenDir is used only to ask to the user whether choice of a directory starting with '.' is wanted; KNewFileMenuPrivate::confirmCreatingHiddenDir checks whether ask by using the message box "do not ask again" key "confirm_create_hidden_tilde_dir"
  • a user tries to create a directory starting with '.':
    1. they get the messagebox that confirmCreatingHiddenDir shows
    2. they tick the "do not ask again"
    3. they proceed
  • as result of the point above, the key "confirm_create_hidden_tilde_dir" is set
  • your changes rename KNewFileMenuPrivate::confirmCreatingHiddenDir to confirmCreatingDir, and make it used also when a directory starts with '~'
  • the same user tries to create a directory starting with '~':
    1. confirmCreatingDir is called
    2. the "confirm_create_hidden_tilde_dir" key is set, so no message box is shown
    3. the directory is created directly, without user confirmation
shubham added a comment.EditedJan 20 2019, 2:53 PM

@pino The test case you told do not behave as you expect (I confirmed).

  1. Create directory named .foo (Results in a warning)
  2. User checks "Do not ask again".
  3. User tries to create directory named .xyz (This time NO WARNING appears, as it is obvious)
  4. User tries to create directory named ~ (A warning is shown)

So the conclusion is that the 2 warning kmessageboxes are treated individually, and their key is also set individually.

pino added a comment.Jan 20 2019, 3:04 PM

Ah yes, now I see it better, the whole KMessageBox::shouldBeShownContinue() check is bogus, since that key is not set by anything.

src/filewidgets/knewfilemenu.cpp
421–422

this is redundant now, as done in both the if branches below

shubham updated this revision to Diff 49943.Jan 20 2019, 3:48 PM
shubham marked an inline comment as done.

Remove redundant setting of window title.

shubham retitled this revision from Allow creating directory named '~' and throw a warning before creating it. to Create directory named '~' and throw a warning before creating it successfully..Jan 20 2019, 3:58 PM
shubham retitled this revision from Create directory named '~' and throw a warning before creating it successfully. to Allow creating directory named '~' and throw a warning before creating it..Jan 20 2019, 4:01 PM

I am of the opinion that such dialogs are not very user friendly and honestly very much against them. Not commenting on the usefulness of this particular warning, but dolphin has quite a few of these kind of dialogs with the same process: Input something -> Get error -> Restart. Feedback should instead be given inplace.

A new dialog built especially for Dolphin's new file/folder/X should take over getting the input from the user and prevent continuing or warn the user about his actions:

  • User leaves empty input? "Name cannot be empty".
  • User types existing folder name? Show him an error message on the spot
  • User types "~" as part of the file name? Show this warning
  • User types names with /, warn that a path will be created instead of just a single directory.
  • User types a name that starts with a dot? Show him the current warning about hidden files etc.

I am strongly of the opinion that such dialog is vastly more user friendly to what we currently have and want to implement. Pinging @ngraham since he's particularly concerned with UX/Usability these days. Looking forward to everyone's thoughts.

Ping? Can someone please review?

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

@ngraham I think this patch is behaving exactly it should be, are you trying for a fix so that these warnings are not used?

Yes, I think people are correct that these modal dialogs are annoying. I'm working on something to show the errors and warnings inline at the moment when you're typing the file/folder name in the first place.

@ngraham Are you still working on this?