Allow accepting by double-click in save dialog
ClosedPublic

Authored by anemeth on Apr 26 2018, 10:14 AM.

Details

Summary

Always accept the dialog after double-clicking on a file without having to click on the Save button first – irrespective of the single/double click mouse settings. Depending on the application, this will either trigger the dialog's or the app's overwrite warning dialog.

Some history about this:
Accepting the dialog by clicking on a file was removed in: 4ccd77256a0c
Accepting by double-clicking was added in: f729c291cdbb
Accepting by double-clicking was removed again by accident in: 7f08f13809bc

BUG: 267749

Test Plan

Double clicking on the icon:

Diff Detail

Repository
R241 KIO
Branch
doubleclick_save (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
anemeth edited the summary of this revision. (Show Details)Apr 26 2018, 10:17 AM

I'm one of those that choose a file and add a "_2" to the filenames. ;-)

Please, please, ask the KDE Usability Project about this change.

I'm one of those that choose a file and add a "_2" to the filenames. ;-)

Please, please, ask the KDE Usability Project about this change.

This patch is part of task T8552 which also tackles usability.
If you have double click enabled, upon single clicking the icon it gets selected where you can add "_2".
If you have single click enabled, well, I'm not quite sure about that...
@ngraham what do you think about this?

ngraham edited the summary of this revision. (Show Details)Apr 26 2018, 1:59 PM

Does the file dialog have those selector overlays Dolphin has?

Does the file dialog have those selector overlays Dolphin has?

No; see https://bugs.kde.org/show_bug.cgi?id=185793

We'll need to fix that before this lands.

@jtamate, it's a valid concern, but if you don't want the single-click behavior, you should turn off single-click. Once we fix the lack of selection markers, then it will be easier to select an existing file whose name you want the saved file 's name to begin with.

We also need to fix the apps that erroneously supply their own overwrite dialog instead of letting the dialog handle it--as illustrated in Alex's video: the overwrite dialog appears only after the file dialog is gone, because it's a specia, thing that Kate displays instead of using the save dialog's own overwrite warning.

ngraham requested changes to this revision.Apr 26 2018, 4:16 PM

In double-click mode, this eliminates the desirable behavior whereby after a single click, the name field becomes focused so you can type a new name. We want to keep that, while also showing the overwrite dialog on double-click.

This revision now requires changes to proceed.Apr 26 2018, 4:16 PM
anemeth updated this revision to Diff 33156.Apr 26 2018, 4:29 PM

Focus filename lineEdit on highlight

Whoops, some unintentional changes made it in, will fix soon...

anemeth updated this revision to Diff 33157.Apr 26 2018, 4:34 PM

Remove unintentional changes

anemeth updated this revision to Diff 33159.Apr 26 2018, 4:35 PM

Sorry, now fixed

Thanks, that's better. This makes me realize that there's a pre-existing bug: the selected text includes the filename extension, and it probably shouldn't. We should fix that in another patch though.

ngraham accepted this revision.Apr 26 2018, 4:47 PM

(As mentioned before, this can't and won't land until other things have been fixed and implemented first)

This revision is now accepted and ready to land.Apr 26 2018, 4:47 PM
rkflx added a subscriber: rkflx.Apr 26 2018, 7:05 PM
anemeth updated this revision to Diff 33165.Apr 26 2018, 7:05 PM
anemeth removed a subscriber: rkflx.

Moved filename line edit focus to D12545

rkflx added a subscriber: rkflx.Apr 26 2018, 7:06 PM

After sleeping over this, I don't feel comfortable the way this is supposed to work for single click: Selection markers only really make sense when enabling multi-select mode. Abusing a single selection marker in single-select mode to mean "put the filename in the line edit so it is editable" feels really weird, and if you are doing this more often than overwriting (which might be very likely!) hitting the small icon is quite cumbersome.

IOW, I don't think the workaround which was promised to @jtamate's concern is good enough, and suggesting to switch to double click only to sustain a "pure" single click does not make sense to me either.

The question for single click is: What is the primary action which should be triggered immediately? Is it overwriting, or is it putting the filename in the field? For Dolphin and the Open dialog it is clear that actually opening is the primary action. For Save I'd argue that overwriting is only the secondary option, and thus should be triggered by the second click only.

I did some digging, and indeed this seems to be how it was originally implemented: 4ccd77256a0c added what this patch is now trying to remove again. Then right after that, f729c291cdbb allowed double clicking to overwrite without moving the mouse to the Save button. Somehow the latter commit got broken later on, I guess in 7f08f13809bc ("Hope I didn't miss anything." haha). Maybe that slot should have triggered only for KFileWidget::Saving?

Given the concerns with selection markers in single-select mode and thus for Save in general, and them being potentially far off in the future, can we think about changing this patch to double-click to overwrite, so single-click would still allow to append _2 to the filename, a likely more common operation? Users who set click behaviour to double click everywhere would not be affected by this change at all!


Historical side note: In KDE3 with single click enabled, the first click would select in file dialogs, the second click would accept. In KDE4 accepting was refined to single click for Open, but the double click was kept for Save, which makes sense to me.

(And please think twice before making this discussion about a general single vs. double click debate again; I'm trying to improve single click to make more sense for the common use cases here, as we agreed upon as a sensible direction. Thanks ;)

rkflx added a comment.EditedApr 27 2018, 10:34 AM

https://bugs.kde.org/show_bug.cgi?id=267749

Wow, reading the bug again, my suggestions is fitting right in! Double click to trigger Save for single click users. Only after some confusion in Comment 1 the only slightly related question of overwriting came up in Comment 4, but this is not what the bug was originally about…

The "double" in the title was literally meant for every click mode, not only for double-click mode.

anemeth updated this revision to Diff 33203.Apr 27 2018, 12:07 PM

Only doubleclick triggers save overwrite

@rkflx thanks for digging through those old commits.
I added that code again, but for saving only.
Unfortunately I could not test it with single click because the setting has vanished (??)

anemeth updated this revision to Diff 33204.Apr 27 2018, 12:14 PM

Add comment

Given the concerns with selection markers in single-select mode and thus for Save in general, and them being potentially far off in the future, can we think about changing this patch to double-click to overwrite, so single-click would still allow to append _2 to the filename, a likely more common operation? Users who set click behaviour to double click everywhere would not be affected by this change at all!

(And please think twice before making this discussion about a general single vs. double click debate again; I'm trying to improve single click to make more sense for the common use cases here, as we agreed upon as a sensible direction. Thanks ;)

Makes sense. I'll let you be the judge of what behavior makes the most sense for single-click, since I don't use it. However to maybe set the record straight: I don't hate single-click; in fact, I love it conceptually! I'm very interested in improving the UX, particularly regarding ease-of-use with selections. If reverting to double-click makes sense here for usability, then let's do it.

@rkflx thanks for digging through those old commits.
I added that code again, but for saving only.
Unfortunately I could not test it with single click because the setting has vanished (??)

Yep, see https://bugs.kde.org/show_bug.cgi?id=393547

Hopefully Roman will fix this soon. In the meantime, you'll have to toggle it by editing kate ~/.config/kdeglobals; add SingleClick=(true|false) to the [KDE] section.

rkflx requested changes to this revision.Apr 27 2018, 6:55 PM

Thanks, works great now for single click users. Glad we could skip a huge discussion ;)

However, for double click mode and Save, descending into directories is kinda broken. You might want to fix that before shipping…

Activate files in the filepicker without having to click on the Open/Save button first, but instead single/double clicking on the icon, depending on the mouse settings.

Please update your summary too, and ideally reference those 3 commits I mentioned, as they contain useful information and you copied most of the patch from there.

(As mentioned before, this can't and won't land until other things have been fixed and implemented first)

@ngraham Is this still the case with the changed scope of the patch?

src/filewidgets/kfilewidget.cpp
1177

Do you mean "override" instead of "trigger" here?

2146–2152

I'd move that right under :_k_slotIconSizeSliderMoved, to keep ordering at least somewhat consistent with the declaration.

This revision now requires changes to proceed.Apr 27 2018, 6:55 PM

@ngraham Is this still the case with the changed scope of the patch?

Yeah, it's fine now that we're going to handle the single-click overwrite use case in another patch.

ngraham edited the summary of this revision. (Show Details)Apr 27 2018, 10:10 PM

[remove] BUG: 267749

Now I'm confused: Isn't what we do here exactly what the bug reporter wanted?

Oops, I was the one who was confused, sorry.

ngraham retitled this revision from Select item without clicking the Open/Save button to Always prompt for overwrite on double-click in save dialog.Apr 28 2018, 4:49 AM
ngraham edited the summary of this revision. (Show Details)

We still need to fix Kate's save dialog before we can land this, because right now it handles the overwrite case itself rather than delegating to the dialog like most KDE apps do. And we should do a sweep of other KDE apps that also have this legacy behavior left over from before the save dialog handled overwrite itself.

We still need to fix Kate's save dialog before we can land this, because right now it handles the overwrite case itself rather than delegating to the dialog like most KDE apps do. And we should do a sweep of other KDE apps that also have this legacy behavior left over from before the save dialog handled overwrite itself.

Does it work if a newer KIO is used with an older version of those applications?

We still need to fix Kate's save dialog before we can land this, because right now it handles the overwrite case itself rather than delegating to the dialog like most KDE apps do. And we should do a sweep of other KDE apps that also have this legacy behavior left over from before the save dialog handled overwrite itself.

Does it work if a newer KIO is used with an older version of those applications?

In Kate's case, it overrides the default save dialog to not handle overwrite, just using a newer KIO wouldn't help and we would definitely want to ship that fix before rolling out the KIO change.

Does it work if a newer KIO is used with an older version of those applications?

In Kate's case, it overrides the default save dialog to not handle overwrite, just using a newer KIO wouldn't help and we would definitely want to ship that fix before rolling out the KIO change.

If I understand it correctly if a the current Kate (or other applications handling the overwrite case) is used with KIO patched with this change, then it will still work because Kate will handle this feature as before. So a patched KIO should be released before Kate and then Kate should be fixed and depend on this newer version. Is all of this correct?

Does it work if a newer KIO is used with an older version of those applications?

In Kate's case, it overrides the default save dialog to not handle overwrite, just using a newer KIO wouldn't help and we would definitely want to ship that fix before rolling out the KIO change.

If I understand it correctly if a the current Kate (or other applications handling the overwrite case) is used with KIO patched with this change, then it will still work because Kate will handle this feature as before.

Correct. It will just be a bit annoying in the case that you didn't mean to overwrite, because the "are you sure" dialog will only appear afterthe save dialog has disappeared, so you'll need to open it again.

So a patched KIO should be released before Kate and then Kate should be fixed and depend on this newer version. Is all of this correct?

My preference would be to release a patched Kate first, if we think that's appropriate for an 18.04.1 point release. Here are our options, along with what will happen for each case:

  • Release patched KIO before patched Kate: Double-clicking on a file triggers overwrite in Kate itself, so the dialog will have annoyingly gone away
  • Release patched Kate before patched KIO: No drawbacks, but users don't get the desirable prompt-overwrite-from-the-save-dialog-on-double-click behavior
  • Release both simultaneously (mostly impossible due to different release schedules): users-double-clicking on a file in Kate's Save dialog get the prompt above the dialog, and canceling it humanely keeps the dialog open.

Whatever happens, just consider that it's always possible (and it will happen) for users to use a newer KIO with an older Kate; so regardless of the Kate patch, this case should work properly.

It will always work properly, it's just a question of how users will be annoyed. :) FWIW right now with the status quo, Kate users are faced with two annoyances:

  • Double-clicking on a file doesn't trigger overwrite
  • Once the overwrite confirmation dialog appears, the Save dialog has already disappeared, so if you change your mind, you need to manually open it again.

This KIO patch removes annoyance #1, and the as-yet-nonexistent Kate patch will remove annoyance #2

TBH, I don't get what the overwrite and release schedule discussion has to do with this patch. All it does is add an additional method to trigger slotOk(), i.e. instead of clicking on Save, users can simply double-click.

The fact that either the dialog or the app ask before overwriting in response to the slot is not changed in any way here. Don't judge a Diff by its title, in particular as long as the request to "Please update your summary too" is still in state "red" ;)


Regarding what you were discussing: If I understood Peter's analysis in D12346 correctly, the API will always allow apps to opt-out of the dialog providing the overwrite confirmation, and instead add their own. Therefore to me it seems like it is only a matter of going through all apps and opt-in to the already existing functionality, i.e. I don't see why this would require changes in KIO which would result in dependency issues (unless I'm mistaken?).

anemeth updated this revision to Diff 33333.Apr 30 2018, 1:38 PM
anemeth marked 2 inline comments as done.Apr 30 2018, 1:39 PM
anemeth edited the summary of this revision. (Show Details)Apr 30 2018, 1:54 PM
anemeth added a comment.EditedApr 30 2018, 1:56 PM

However, for double click mode and Save, descending into directories is kinda broken. You might want to fix that before shipping…

I can't reproduce this error.
Can you post a video about it?

rkflx retitled this revision from Always prompt for overwrite on double-click in save dialog to Allow accepting by double-click in save dialog.Apr 30 2018, 10:44 PM
rkflx edited the summary of this revision. (Show Details)

However, for double click mode and Save, descending into directories is kinda broken. You might want to fix that before shipping…

I can't reproduce this error.
Can you post a video about it?

Ah, sorry. Didn't realize there were more steps required to trigger the problem:

  • Set mouse behaviour to "double-click".
  • Double-click on file to trigger "Overwrite" warning, press Cancel on the warning dialog.
  • Try to enter a directory by double-clicking. This will also result in a "Overwrite" warning, while it shouldn't.

Selecting a file by

Sorry that I changed this in the summary, but I find it very confusing to talk about "select" here, because it can have so many different meanings…

ngraham requested changes to this revision.May 1 2018, 1:57 AM

For me this also breaks entering folders with double-click even before I cancel the first overwrite: it immediately closes the dialog and saves in the current directory.

This revision now requires changes to proceed.May 1 2018, 1:57 AM
anemeth updated this revision to Diff 33900.May 9 2018, 6:02 PM
  • Merge branch 'master' of git://anongit.kde.org/kio into arcpatch-D12538
  • Double clicking to save only works on files
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 9 2018, 6:02 PM

Nice! Behaviorally, this works well now and I didn't find any regressions. Just some code style comments below:

src/filewidgets/kfilewidget.cpp
1178

Unnecessary unrelated whitespace change

1844

Let's use new-style signal/slot syntax for new code. See https://wiki.qt.io/New_Signal_Slot_Syntax

rkflx added inline comments.May 9 2018, 9:30 PM
src/filewidgets/kfilewidget.cpp
1844

Is this even possible without a lot of refactoring (which would be out-of-scope for this patch)?

2161

I find inf as a variable name slightly too cryptic.

But actually I'd just write this inline and go without QFileInfo and the QUrl conversion altogether, because KFileItem should already provide what is needed:

&& ops->selectedItems().first().isFile()
ngraham added inline comments.May 9 2018, 9:37 PM
src/filewidgets/kfilewidget.cpp
1844

Not for everything, just this one.

rkflx added inline comments.May 9 2018, 9:39 PM
src/filewidgets/kfilewidget.cpp
1844

Well, how should that line look like then?

anemeth added inline comments.May 9 2018, 9:40 PM
src/filewidgets/kfilewidget.cpp
1844

I'm having a lot of trouble finding the correct syntax...
And the compiler is giving me 100 lines of cryptic error message.
Could you help me out on this one?

anemeth updated this revision to Diff 33914.May 9 2018, 9:43 PM
  • Remove QFileInfo
anemeth marked 2 inline comments as done.May 9 2018, 9:44 PM
ngraham added inline comments.May 9 2018, 9:59 PM
src/filewidgets/kfilewidget.cpp
1844

Hmm, you're right, I can't get it to work without a lot of refactoring either. Let's leave this alone for now and do the connect syntax porting later.

ngraham accepted this revision.May 9 2018, 10:13 PM
rkflx accepted this revision.May 10 2018, 6:08 AM
This revision is now accepted and ready to land.May 10 2018, 6:08 AM
This revision was automatically updated to reflect the committed changes.