When the selection is deselected, restart the keyboard search from the beginning
ClosedPublic

Authored by meven on Sep 4 2019, 10:29 AM.

Details

Summary

BUG: 411538
FIXED-IN: 19.12

Test Plan

Open a directory with 3 files starting with the same letter.

  1. Press this letter key twice
  2. The second file is selected
  3. Deselect the file with the mouse or using Esc
  4. Wait 1 second
  5. Press the same key again

Before:
The third file gets selected

After:
The first file get selected

ctest

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D23716_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17502
Build 17520: arc lint + arc unit
meven created this revision.Sep 4 2019, 10:29 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptSep 4 2019, 10:29 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
meven requested review of this revision.Sep 4 2019, 10:29 AM
meven edited the test plan for this revision. (Show Details)Sep 4 2019, 10:31 AM
meven edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.Sep 4 2019, 3:57 PM

Nice. But I'm wondering why we even need the 1 second timeout for the case where the selection is cleared.

meven updated this revision to Diff 65420.Sep 5 2019, 12:45 PM

Add a KItemListSelectionManager::replaceSelection to avoid sending twice selectionChanged in a row on KItemListController::slotChangeCurrentItem

meven edited the test plan for this revision. (Show Details)Sep 5 2019, 12:45 PM

Nice. But I'm wondering why we even need the 1 second timeout for the case where the selection is cleared.

emit changeCurrentItem triggers KItemListController::slotChangeCurrentItem which calls in a row:

m_selectionManager->clearSelection();
m_selectionManager->setSelected(index, 1);

Triggering KItemListSelectionManager::SelectionChanged twice in a row and KItemListKeyboardSearchManager::slotSelectionChanged twice
The first time with : previous.isEmpty(): false current.isEmpty(): true previous.count(): 1 current.count(): 0 # deselect everything
Second time with : previous.isEmpty(): true current.isEmpty(): false previous.count(): 0 current.count(): 1 # select the new element

This caused a bug :
Let's you have three files : e1 e2 e3 in folder.
You type e then e and then e you expect the third file to be selected e3.
Without this check, the first file was selected and all subsequent e typing stays on the same file.

I have changed this to not be needed anymore adding KItemListSelectionManager::replaceSelection

meven updated this revision to Diff 65422.Sep 5 2019, 12:50 PM

Clean commit

ngraham accepted this revision.Sep 5 2019, 2:18 PM

LGTM but please wait for a thumbs-up from @elvisangelaccio as well.

This revision is now accepted and ready to land.Sep 5 2019, 2:18 PM
elvisangelaccio requested changes to this revision.Oct 6 2019, 11:24 AM

Patch seems to work fine, but the lack of commit message makes it hard to understand where the actual bugfix is. Please simplify the patch as mentioned inline.

src/kitemviews/kitemlistselectionmanager.cpp
181

Is selectionChanged() being emitted twice part of the bug? Or is this an unrelated refactoring that could go to another commit?

src/kitemviews/private/kitemlistkeyboardsearchmanager.cpp
37–47

This is an unrelated refactoring that should go in its own commit. Feel free to commit this single change without review.

This revision now requires changes to proceed.Oct 6 2019, 11:24 AM
meven updated this revision to Diff 67590.Oct 10 2019, 7:28 AM

Rebasing

meven marked 2 inline comments as done.Oct 10 2019, 7:30 AM
meven added inline comments.
src/kitemviews/kitemlistselectionmanager.cpp
181

I opened a separated diff D24505

meven updated this revision to Diff 68599.Oct 23 2019, 12:01 PM
meven marked an inline comment as done.

Rebase on master after D24505 changes

elvisangelaccio accepted this revision.Oct 26 2019, 8:32 AM
This revision is now accepted and ready to land.Oct 26 2019, 8:32 AM
This revision was automatically updated to reflect the committed changes.