Show feedback inline when creating new files or folders
ClosedPublic

Authored by ngraham on Jun 19 2019, 4:15 PM.

Details

Summary

Right now, the new file/folder dialog allows you to give it an invalid name, and
afterwards it will show you a dialog window complaining at you. This is not very
user-friendly, and becomes more user-unfriendly if we want to use it to show
warnings rather than errors. Finally it doesn't scale well because as we add more
conditions to check for, we'll need to add more dialog windows.

This patch implements the name feedback/warning/error inline in the original name
dialog itself, and disables the OK button for invalid names.

This patch obsoletes D18384, D18563, and D18599.

Test Plan

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Jun 19 2019, 4:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 19 2019, 4:15 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Jun 19 2019, 4:15 PM
meven added a subscriber: meven.Jun 19 2019, 4:42 PM
meven added inline comments.
src/filewidgets/knewfilemenu.cpp
1054

I would rather disable the ok button, as long as the user has not entered text, rather than using an error.

ngraham updated this revision to Diff 60079.Jun 19 2019, 5:03 PM
ngraham marked an inline comment as done.

Don't show an ugly error when the text field is blank

meven added a comment.Jun 19 2019, 6:05 PM

It made me think about D17595.
Would it make sense to upstream this to KIO and unify behavior with Open/Save Dialog (it currently has a folder name check using popups).
This is a different matter so it should not stop progress here.

src/filewidgets/knewfilemenu.cpp
1078

What about Windows ? maybe use QDir::separator() ?

It made me think about D17595.
Would it make sense to upstream this to KIO and unify behavior with Open/Save Dialog (it currently has a folder name check using popups).
This is a different matter so it should not stop progress here.

This is already in KIO. :)

aacid added a subscriber: aacid.Jun 19 2019, 6:27 PM
aacid added inline comments.
src/filewidgets/knewfilemenu.cpp
1078

read the docu for QDir::separator() , you seldom ever want to use it.

https://agateau.com/2015/qdir-separator-considered-harmful/

Have not looked at the patch, just the gif: I am rather sceptical if instant feedback is a good idea here. First, having an empty line edit will happen a lot, usually without the user wanting to press OK. That means the user will already get feedback in the middle of the workflow even though he did not yet have the intention to click OK yet.
The same may hold true for the other cases. Too much on-the-fly feedback may be annoying and stress the user unnecessarily.

Finally, resizing the dialog depending on the error message also looks rather hard on my eyes.

The much better fix would be: create the folder "New Folder" inline in the list view and immediately switch to edit mode, this way you do not need a dialog at all but don't loose anything. If the folder already exists, name it "New Folder (i)". Yes, that's how it's done in Windows as well, but I think it makes a lot of sense.

Comments? :)

The gif depicts a rather extreme case where you're deliberately trying to test all the feedback modes in quick succession. :) At @meven's request, I've already removed the red error message when the text field is empty.

Do the warnings need to below the buttons? I feel like they are kinda out of place there.

The much better fix would be: create the folder "New Folder" inline in the list view and immediately switch to edit mode, this way you do not need a dialog at all but don't loose anything. If the folder already exists, name it "New Folder (i)". Yes, that's how it's done in Windows as well, but I think it makes a lot of sense.

Comments? :)

Unfortunately we cannot do this for the dialogs from KNewFileMenu because KIO has no knowledge of the client's view. This dialog is invoked not only from Dolphin, but in the file dialogs and many other places. Dolphin itself could implement this as it already has a "rename inline" mode, but it would be a lot of extra code. And KDirOperator which is used for the file dialogs does not have that feature so it would have to gain it first.

Finally, resizing the dialog depending on the error message also looks rather hard on my eyes.

Do the warnings need to below the buttons? I feel like they are kinda out of place there.

I experimented with putting the warnings elsewhere, but then the problem is that they cause items in the user interface below them to jump around as they appear and disappear. I know from experience that many people really hate this.

filipf accepted this revision.Jun 20 2019, 12:59 PM
filipf added a subscriber: filipf.

Looks good from a visual and usability POV.

This revision is now accepted and ready to land.Jun 20 2019, 12:59 PM

Any more comments?

ngraham planned changes to this revision.Jun 20 2019, 1:41 PM

Going to better handle Windows and also fix a bug that slipped in.

ngraham updated this revision to Diff 60137.Jun 20 2019, 2:47 PM
  • Do the right thing on Windows
  • Don't show any warnings when creating filenames with slashes in them (KIO currently allows it but substitutes a different character)
This revision is now accepted and ready to land.Jun 20 2019, 2:47 PM

If there are no formal objections, I'd like to land this.

meven added inline comments.Jun 22 2019, 8:55 AM
src/filewidgets/knewfilemenu.cpp
1099

Shouldn't we disable the ok button here ?

1109

Shouldn't we disable the ok button here too ?

1114

Same here

1122

Same here

ngraham updated this revision to Diff 60299.Jun 22 2019, 9:53 AM
ngraham marked 4 inline comments as done.

Disable Ok button for all error conditions (but not for warning conditions)

ngraham added inline comments.Jun 22 2019, 9:53 AM
src/filewidgets/knewfilemenu.cpp
1099

No, because this isn't an error condition (it's just a warning).

1122

No, because this isn't an error condition (it's just a warning).

ngraham marked 2 inline comments as done.Jun 22 2019, 9:54 AM
meven accepted this revision.Jun 22 2019, 10:12 AM

This is look good to me !

This revision was automatically updated to reflect the committed changes.

These two points were not discussed anymore:

  1. Isn't there a better solution by creating the folder and immediately select it + change to edit mode?
  2. Resizing dialogs are usually not preferred.

If everyone else thinks this change is a good idea - then fine. I sm rather sceptical ;)

These two points were not discussed anymore:

I addressed them in https://phabricator.kde.org/D21907#482181:

  1. Isn't there a better solution by creating the folder and immediately select it + change to edit mode?

Not really. The proposed behavior would need to be implemented in every single app rather than in KIO, because KIO doesn't know about the view that will hold the new file or folder that's being created. Even if we did do add view-specifric behavior to every app, then we would still have the problem here in KIO's new file/folder dialog, which is app- and view-agnostic. I think it's nicer to have it here in KIO, and have every app use the same component, rather than re-implement the same set of features in many places for different views.

  1. Resizing dialogs are usually not preferred.

Yeah, I can understand this concern. In general I find that people get upset when the window or the window content changes when a KMessageWidget appears, which is a legitimate criticism. However this is a general problem with the KMessageWidget's visual design itself, not really with this specific patch. I would be in favor of making a "popover" version that shows the message in a little pop-up frame that doesn't resize the window or its content. GNOME and macOS do this and it's really quite nice.

Of course someone would need to write that component. :)

So it boils down to "let's try this and improve later if necessary" - well then :)