Don't forward keys until the recipient is ready
ClosedPublic

Authored by leinir on Apr 12 2017, 11:33 AM.

Details

Summary

This avoids numerous crashes when using the keyboard navigation in Discover during searches. It would seem that, without this, we occasionally (which during a search with auto-updating results means regularly) send key events to a component which is not yet ready to receive them... It would be nice if this were caught by Qt, but i guess that might be a little expensive to do generically.

nb: i would like this to be a typesafe comparison, but... yay qml ;)

Test Plan

Before: Discover crashed a lot during searches
Now: Discover crashes /way/ less during searches :)

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Apr 12 2017, 11:33 AM
Restricted Application added a project: Kirigami. · View Herald TranscriptApr 12 2017, 11:33 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol accepted this revision.Apr 12 2017, 11:59 AM

Thanks!

This revision is now accepted and ready to land.Apr 12 2017, 11:59 AM
apol requested changes to this revision.Apr 12 2017, 1:43 PM

I just tested this, I get this error message:
file:///home/apol/devel/kde5/lib64/qml/org/kde/kirigami.2/ScrollablePage.qml:132: TypeError: Cannot read property 'Component' of null

This revision now requires changes to proceed.Apr 12 2017, 1:43 PM
apol added a comment.Apr 12 2017, 1:46 PM
In D5408#101552, @apol wrote:

I just tested this, I get this error message:
file:///home/apol/devel/kde5/lib64/qml/org/kde/kirigami.2/ScrollablePage.qml:132: TypeError: Cannot read property 'Component' of null

This seems to do the trick:
Keys.forwardTo: root.keyboardNavigationEnabled && ("currentItem" in root.flickable) && root.flickable.currentItem && root.flickable.currentItem.Component.status == Component.Ready ? [ root.flickable.currentItem ] : []

apol added a comment.Apr 13 2017, 1:19 AM

Add BUG: 378339

leinir updated this revision to Diff 13374.Apr 13 2017, 8:20 AM
leinir edited edge metadata.

Add the missing check identified by Aleix. Thanks :)

In D5408#101657, @apol wrote:

Add BUG: 378339

That's not how i've been triggering it (i've been using the arrow keys for list navigation, which also causes it), but yup, that's the backtrace :)

mart accepted this revision.Apr 18 2017, 10:24 AM
This revision now requires changes to proceed.Apr 18 2017, 10:24 AM
apol accepted this revision.Apr 18 2017, 10:43 AM
This revision is now accepted and ready to land.Apr 18 2017, 10:43 AM
This revision was automatically updated to reflect the committed changes.