Use Kirigami.SearchField
ClosedPublic

Authored by ognarb on Jul 26 2019, 12:15 PM.

Details

Reviewers
ngraham
Group Reviewers
Elisa
Commits
R255:8b1c8587d54c: Use Kirigami.SearchField
Summary

Bump KDE Framework version to 5.57 for Kirigami 2.8

Depends on D22781

Test Plan

Run.

No visual change:

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ognarb requested review of this revision.Jul 26 2019, 12:15 PM
ognarb created this revision.
ognarb edited the test plan for this revision. (Show Details)Jul 26 2019, 12:20 PM
ognarb added a reviewer: Elisa.
ngraham edited the summary of this revision. (Show Details)Jul 26 2019, 2:20 PM
ngraham requested changes to this revision.Jul 26 2019, 2:23 PM
ngraham added a subscriber: ngraham.

+1 on the concept. Actual search field works fine too.

Using the Ctrl+F shortcut only works intermittently though. I seem to have to hit it three times before the search fiels appears or disappears. Maybe it's conflicting with the shortcut set on the action elsewhere in this file.

This revision now requires changes to proceed.Jul 26 2019, 2:23 PM
ognarb updated this revision to Diff 62619.Jul 26 2019, 4:50 PM

Use ActionTextField instead of SearchField

Move again to framework 5.56

+1 on the concept. Actual search field works fine too.

Using the Ctrl+F shortcut only works intermittently though. I seem to have to hit it three times before the search fiels appears or disappears. Maybe it's conflicting with the shortcut set on the action elsewhere in this file.

I probably broke it even more, but I get the same behavior using master, but I only need to hit two times to toggle the search bar. Could you please it confirm it?

Hmm, using an actual SearchField is semantically correct here. If it has bugs that make it not work for our use case here, we should fix them upstream in Kirigami rather than working around them by using a more basic roll-your-own component.

For example, perhaps we could add a public property that tells it to not set its own shortcut. Or maybe we could remove the shortcut on the action and connect the search row's visibility to the text field gaining focus?

Just thinking out loud here.

Hmm, using an actual SearchField is semantically correct here. If it has bugs that make it not work for our use case here, we should fix them upstream in Kirigami rather than working around them by using a more basic roll-your-own component.

For example, perhaps we could add a public property that tells it to not set its own shortcut. Or maybe we could remove the shortcut on the action and connect the search row's visibility to the text field gaining focus?

Just thinking out loud here.

It was just a test, but even if we use the ActionTextField the bug is still present for me.

ognarb updated this revision to Diff 62671.EditedJul 28 2019, 9:39 AM

Use SearchField with focusSequcence: null
Use Kirigami 5.58

Tested with D22781 and it works.

ognarb updated this revision to Diff 62685.Jul 28 2019, 3:36 PM

Use Kirigami 5.57 because SearchField is from 5.57 and not 5.58

ngraham edited the summary of this revision. (Show Details)Aug 2 2019, 12:03 AM
ngraham requested changes to this revision.Aug 2 2019, 12:07 AM

Works great now, just needs one more little change:

src/qml/NavigationActionBar.qml
22

The minimum version is 2.8, not 2.7

This revision now requires changes to proceed.Aug 2 2019, 12:07 AM
ognarb updated this revision to Diff 62961.Aug 2 2019, 8:44 AM

Using Kirigami 2.8

I forgot to change that.

ngraham accepted this revision.Aug 2 2019, 2:55 PM

Thanks Carl!

This revision is now accepted and ready to land.Aug 2 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.