Differential D13152

Fix loop of FocusIn events
ClosedPublic
Actions

Authored by elvisangelaccio on May 27 2018, 3:40 PM.

Details

Reviewers
anthonyfieroni
Group Reviewers
Dolphin
Commits
R318:9616edbb66a8: Fix loop of FocusIn events
Summary

Commit 43da84eefc7d introduced the risk of entering an endless loop of
FocusIn/FocusOut events sent to two DolphinSearchBox instances when
opening a second tab (see D11871).

This happens because we deactivate the first tab when we open a new one, but
since the setActive(true) is delayed with a QTimer, both the old tab
and the new one become active and receive their own FocusIn event
(which starts the loop of focus in/out events).

To prevent this issue, we schedule the searchbox activation only if the
searchbox is not already active.

Test Plan
  • Search something in dolphin
  • Open a new tab after the search ends
  • Check that dolphin does not eat the CPU

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 27 2018, 3:40 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
elvisangelaccio requested review of this revision.May 27 2018, 3:40 PM
anthonyfieroni added a comment.EditedMay 27 2018, 3:44 PM

I try this, but it does not work in all cases. I can't test right now, but i'll provide more info soon.
Edit: I remember, now when change tabs focus is not in search box, which is annoying.

elvisangelaccio planned changes to this revision.Jun 2 2018, 10:58 AM
In D13152#269115, @anthonyfieroni wrote:

I try this, but it does not work in all cases. I can't test right now, but i'll provide more info soon.
Edit: I remember, now when change tabs focus is not in search box, which is annoying.

Crap. You're right, if we decouple setActive() from setFocus() we lose this feature. :/

I'm working on another approach...

elvisangelaccio updated this revision to Diff 35396.Jun 2 2018, 11:41 AM
  • Don't schedule the activation if we are already active
elvisangelaccio edited the summary of this revision. (Show Details)Jun 2 2018, 11:43 AM
elvisangelaccio edited the test plan for this revision. (Show Details)
elvisangelaccio edited the summary of this revision. (Show Details)
elvisangelaccio updated this revision to Diff 35397.Jun 2 2018, 11:44 AM
  • Bring back setFocus() call
anthonyfieroni added a comment.Jun 2 2018, 1:57 PM

I try this too, but loop still present, because DolphinViewContainer deactivates other one.

elvisangelaccio added a comment.Jun 2 2018, 2:07 PM
In D13152#272642, @anthonyfieroni wrote:

I try this too, but loop still present

When?

anthonyfieroni added a comment.Jun 2 2018, 6:34 PM

When you have more than one tab with search box in click repeatedly between.

elvisangelaccio added a comment.Jun 3 2018, 1:01 PM
In D13152#272717, @anthonyfieroni wrote:

When you have more than one tab with search box in click repeatedly between.

Cannot reproduce doing the following:

  1. CTRL+F and type something
  2. Open a new tab after search ends
  3. Open another tab
  4. Switch repeatedly between the three tabs

Am I missing something?

anthonyfieroni accepted this revision.EditedJun 10 2018, 7:04 PM

Looks like it works, but i'm pretty sure that i test patch like this. So let fix it, then will see :)

This revision is now accepted and ready to land.Jun 10 2018, 7:04 PM
Closed by commit R318:9616edbb66a8: Fix loop of FocusIn events (authored by elvisangelaccio). · Explain WhyJun 13 2018, 7:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/search/dolphinsearchbox.cpp (12 lines)

Diff 36113

View Options

src/search/dolphinsearchbox.cpp

Loading...