KFileWidget In saving single file mode an enter/return press on the KDirOperator triggers slotOk
ClosedPublic

Authored by meven on Mar 17 2019, 1:11 PM.

Details

Summary

Draw inspiration from inspiration from https://phabricator.kde.org/D12538 for reference.

The issue at hand:

  • the KDirOperator did not signal when it received an enter/return event when it should.
  • This prevents expected behavior

BUG: 385189
FIXED-IN: 5.57

Test Plan

1 compile kio and its test kfilewidgettest_saving_gui.cpp
2 launch kfilewidgettest_saving_gui
4 click on an place on the left
5 Hit enter
6 The dialog closes, the filename selected is printed out in the terminal

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9712
Build 9730: arc lint + arc unit
meven created this revision.Mar 17 2019, 1:11 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 17 2019, 1:11 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Mar 17 2019, 1:11 PM
meven updated this revision to Diff 54081.Mar 17 2019, 1:13 PM

Remove blank line

meven updated this revision to Diff 54082.Mar 17 2019, 1:15 PM

Add deleted blank line

meven retitled this revision from In saving single file mode an enter/return press on the KDirOperator triggers slotOk to KFileWidget In saving single file mode an enter/return press on the KDirOperator triggers slotOk.Mar 18 2019, 9:46 AM

Thanks for the patch, and for adding a test!

Question: how come the return/enter key currently works for this use case in the open dialog without all this extra code?

meven added a comment.EditedMar 18 2019, 8:32 PM

Thanks for the patch, and for adding a test!

Question: how come the return/enter key currently works for this use case in the open dialog without all this extra code?

In the open dialog or save dialog with select + enter or select + double/simple click cases, the event activated is emitted which triggers slotOk.
But Activated is emitted only when a file is selected.

Here we expect enter/return to work without selecting a file.
We can view the KDirOperator here as both a file selector (to overwrite a file) or a directory selector to save the file in the selected directory depending on the user interaction.

ngraham accepted this revision.Mar 18 2019, 10:07 PM

Makes sense, thanks. This fix works and looks sane to me.

Maybe wait until at least one more Dolphin and/or Frameworks person has reviewed before committing, though.

This revision is now accepted and ready to land.Mar 18 2019, 10:07 PM

Makes sense, thanks. This fix works and looks sane to me.

Maybe wait until at least one more Dolphin and/or Frameworks person has reviewed before committing, though.

I still lack a bit experience with Qt, and my solution feels a bit overkill : listening to key Press and emitting a specific event.
I could be missing a more idiomatic way to do this.
I'd very much appreciate a second review.

At least now the issue at hand is evident.

I think I will clean up the test code a bit.

dfaure requested changes to this revision.Mar 24 2019, 9:56 PM

The added signal seems fine to me. Much less black magic than an event filter (which would have been the other option).

src/filewidgets/kdiroperator.h
916

Missing @since 5.57

tests/kfilewidgettest_gui.cpp
30 ↗(On Diff #54082)

This bit is what's non-idiomatic, IMHO.

Why not capture app by reference?

This revision now requires changes to proceed.Mar 24 2019, 9:56 PM
meven edited the summary of this revision. (Show Details)Mar 26 2019, 5:34 PM
meven edited the test plan for this revision. (Show Details)Mar 26 2019, 5:40 PM
meven updated this revision to Diff 54868.Mar 26 2019, 5:41 PM
meven marked an inline comment as done.

Updated after review, add another test file instead of modifying it.

ngraham added inline comments.Mar 26 2019, 5:42 PM
tests/kfilewidgettest_saving_gui.cpp
2 ↗(On Diff #54868)

Your copyright, since you wrote this file

meven updated this revision to Diff 54869.Mar 26 2019, 5:42 PM

update commit message

meven updated this revision to Diff 54924.Mar 27 2019, 10:25 AM

fix copyright

meven marked an inline comment as done.Mar 27 2019, 10:26 AM
meven added inline comments.
src/filewidgets/kdiroperator.h
916

Thanks

@dfaure is this good now?

dfaure accepted this revision.Mar 28 2019, 9:02 PM

Remember to update the commit log so it doesn't keep saying "First attempt, not necessarily the right one." :-)

This revision is now accepted and ready to land.Mar 28 2019, 9:02 PM
meven edited the summary of this revision. (Show Details)Mar 28 2019, 9:23 PM
This revision was automatically updated to reflect the committed changes.
meven added a comment.Mar 28 2019, 9:39 PM

Remember to update the commit log so it doesn't keep saying "First attempt, not necessarily the right one." :-)

Thanks !