BUG: 377978
Details
Diff Detail
- Repository
- R241 KIO
- Branch
- dir
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7312 Build 7330: arc lint + arc unit
Neat idea.
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
440 | That's not accurate; the dir just begins with ~. | |
442 | 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. | |
907 | 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. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
440 | Oops, never mind, ignore this. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
442 | Actually maybe this: Naming a folder \$1\ is not recommended because it may be confusing or dangerous when using the terminal to delete things. |
My string change request isn't addressed, and the string still doesn't end with a period.
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
439 | Please simply write name == QStringLiteral (...) instead of operator==. Same below. |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
439 | This is faster than simple == |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
439 | Really? But isn't == not exactly the same? Can you elaborate? |
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) | |
911 | extra line added |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
439 | == internally calls operator==() |
src/filewidgets/knewfilemenu.cpp | ||
---|---|---|
439 | The compiler automatically replaces the operators into the calls to the appropriate functions. There is no speed penalty. |
@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?
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 '.':
- they get the messagebox that confirmCreatingHiddenDir shows
- they tick the "do not ask again"
- 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 '~':
- confirmCreatingDir is called
- the "confirm_create_hidden_tilde_dir" key is set, so no message box is shown
- the directory is created directly, without user confirmation
@pino The test case you told do not behave as you expect (I confirmed).
- Create directory named .foo (Results in a warning)
- User checks "Do not ask again".
- User tries to create directory named .xyz (This time NO WARNING appears, as it is obvious)
- 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.
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 | ||
---|---|---|
422 | this is redundant now, as done in both the if branches below |
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.
@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.
I've submitted a proposed replacement for this as D21907: Show feedback inline when creating new files or folders.