Differential D24505

Avoid emitting twice twice selectionChanged when keyboard changes the selection, fix slotChangeCurrentItem
ClosedPublic
Actions

Authored by meven on Oct 9 2019, 8:16 AM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Dolphin
Commits
R318:56a38da57a02: Avoid emitting twice twice selectionChanged when keyboard changes the selection…
Summary

In KItemListController::slotChangeCurrentItem searchFromNextItem use was bugged :

The two branches of if (searchFromNextItem) both looked for the next keyboard with indexForKeyboardSearch(text, currentIndex (the first one with just a +1 modulo).
But when searchFromNextItem is false, we are supposed to start to look for the next indexKeyboard from the start of the list 0, not from the currentIndex

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D24505
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17669
Build 17687: arc lint + arc unit
meven created this revision.Oct 9 2019, 8:16 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 9 2019, 8:16 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
meven requested review of this revision.Oct 9 2019, 8:16 AM
elvisangelaccio requested changes to this revision.Oct 13 2019, 2:08 PM

I still don't understand if we are fixing an issue or if this is just a removal of a redundant signal. AFAICS we are only removing a selectionChanged() instance called by clearSelection() in "search mode", but this signal is also emitted in many other places (e.g. is emitted 3 times if I change selection with the mouse).

Please explain the rationale for this patch in the commit message.

src/kitemviews/kitemlistcontroller.cpp
480

Can you explain the reason for this change?

src/kitemviews/kitemlistselectionmanager.cpp
178–181

Please move this info in the apidox for replaceSelection() in the header file.

This revision now requires changes to proceed.Oct 13 2019, 2:08 PM
meven updated this revision to Diff 67876.Oct 14 2019, 6:25 AM
meven marked an inline comment as done.

Move comment to header

meven marked an inline comment as done.Oct 14 2019, 6:25 AM
meven added inline comments.
src/kitemviews/kitemlistcontroller.cpp
480

This function way bugged :
The two branches of if (searchFromNextItem) both looked for the next keyboard with indexForKeyboardSearch(text, currentIndex (the first one with just a +1 modulo).
But when searchFromNextItem is false, we are supposed to start to look for the next indexKeyboard from the start of the list 0, not from the currentIndex.

elvisangelaccio added a comment.Oct 20 2019, 9:50 AM
In D24505#546485, @elvisangelaccio wrote:

I still don't understand if we are fixing an issue or if this is just a removal of a redundant signal. AFAICS we are only removing a selectionChanged() instance called by clearSelection() in "search mode", but this signal is also emitted in many other places (e.g. is emitted 3 times if I change selection with the mouse).

Please explain the rationale for this patch in the commit message.

Ping @meven

src/kitemviews/kitemlistcontroller.cpp
480

I see. Please put this information in the commit message because it is valuable and it shouldn't be lost in a phabricator inline comment.

meven retitled this revision from Avoid emitting twice twice selectionChanged when keyboard changes the selection to Avoid emitting twice twice selectionChanged when keyboard changes the selection, fix slotChangeCurrentItem.Oct 20 2019, 10:35 AM
meven edited the summary of this revision. (Show Details)
meven updated this revision to Diff 68343.Oct 20 2019, 10:37 AM
meven edited the summary of this revision. (Show Details)

Update commit message

elvisangelaccio accepted this revision.Oct 20 2019, 11:34 AM
This revision is now accepted and ready to land.Oct 20 2019, 11:34 AM
Closed by commit R318:56a38da57a02: Avoid emitting twice twice selectionChanged when keyboard changes the selection… (authored by meven). · Explain WhyOct 20 2019, 2:05 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 67876

View Options

src/kitemviews/kitemlistcontroller.cpp

Loading...
View Options

src/kitemviews/kitemlistselectionmanager.h

Loading...
View Options

src/kitemviews/kitemlistselectionmanager.cpp

Loading...