Add an ActionTextField component
ClosedPublic

Authored by ognarb on Feb 3 2019, 11:30 PM.

Details

Summary

<old>This is based on the searchField from Discover. It's a suggestion from @ngraham and I find it's a great idea.

Note: Because it's based on the searchField from Discover licensed under GPL2-or-later and Kirigami is licensed under LGPL2-or-later, this patch need the authorization from @apol and @ngraham, before landing.</old>

Now it's a generic TextField with the possibility to add buttons left and a button right. Also, update the Sidebar example to use this component:

Test Plan

For now kirigami build. I wasn't only able to test this component by copying this component to the example directory.

Diff Detail

Repository
R169 Kirigami
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7841
Build 7859: arc lint + arc unit
ognarb created this revision.Feb 3 2019, 11:30 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptFeb 3 2019, 11:30 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ognarb requested review of this revision.Feb 3 2019, 11:30 PM
ognarb edited the summary of this revision. (Show Details)Feb 3 2019, 11:33 PM
ognarb added reviewers: apol, Kirigami, ngraham.
ognarb added a subscriber: apol.

Nice, I give my permission for the re-licensing.

src/controls/SearchField.qml
33

@apol Is this still applicable if it's a re-usable component?

mart requested changes to this revision.Feb 4 2019, 7:49 AM
mart added a subscriber: mart.

If we are going to have a specialized line edit, I think it shouldn't be specific for search, but be a common component of things that we have to boilerplate too often as they are missing from qqc2.
Namely, the clear button.
What I would like is a lineedit with two list properties leftactions and rightactions, which the clear button can be one of them (we have some fields around in plasma which have more than one action)

And then after that, maybe doing the specialization which is purely a search field

This revision now requires changes to proceed.Feb 4 2019, 7:49 AM
apol added a comment.Feb 4 2019, 11:46 AM

I'm fine with having this code relicensed.

src/controls/SearchField.qml
33

No, this is for Discover-specific behavior.

In D18716#404718, @mart wrote:

If we are going to have a specialized line edit, I think it shouldn't be specific for search, but be a common component of things that we have to boilerplate too often as they are missing from qqc2.
Namely, the clear button.
What I would like is a lineedit with two list properties leftactions and rightactions, which the clear button can be one of them (we have some fields around in plasma which have more than one action)

And then after that, maybe doing the specialization which is purely a search field

Interesting, but I have some question before I start implementing.

  • Do you have an example for a left action?
  • Should the buttons only be visible when the text is not empty? Or can this behavior also be configurable with a property?

Should the buttons only be visible when the text is not empty? Or can this behavior also be configurable with a property?

Action has a visible property which should be taken into account like normal here as well

I agree, it should be a generic TextField that just so happens to be useful as a search field if you'd like. Bonus points if one of the built-in actions is "show/hide password" so it can be used as a password field too.

mart added a comment.Feb 6 2019, 3:41 PM

Interesting, but I have some question before I start implementing.

  • Do you have an example for a left action?

right now only web browsers url bars come to mind, so perhaps not important yet to add, (just to be tought of so can be added in the future should the need arise)

  • Should the buttons only be visible when the text is not empty? Or can this behavior also be configurable with a property?

should be configurable i think: a clear action should only be visible when there is text, instead a filter icons, or the show password action should probably be always there.

ognarb updated this revision to Diff 51215.Feb 8 2019, 7:49 PM

Create a generic TextField instead

ognarb retitled this revision from Add a SearchField component to Add a TextField component.Feb 8 2019, 7:54 PM
ognarb edited the summary of this revision. (Show Details)
ognarb edited the test plan for this revision. (Show Details)
ognarb edited the summary of this revision. (Show Details)Feb 9 2019, 12:29 AM
ognarb edited the summary of this revision. (Show Details)
ognarb added inline comments.Feb 9 2019, 12:33 AM
src/controls/TextField.qml
101 ↗(On Diff #51215)

@mart I'm not sure if it's the right way to convert the icon from the action to the one needed by the toolButton (don't support icon.source)

src/kirigamiplugin.cpp
181 ↗(On Diff #51215)

@mart Should I use 2.6 or 2.7? I didn't find any information about that version I should use.

mart requested changes to this revision.Feb 9 2019, 10:09 AM
mart added inline comments.
src/controls/TextField.qml
74 ↗(On Diff #51215)

I would like those to actually be lists as we do have use cases for more than one icon

94 ↗(On Diff #51215)

this should be a repeater with the toolbutton as aproperty (maybe shouldn't even be a toolbotton, as usually they're just icons without any button graphics)

101 ↗(On Diff #51215)

you can try to use a private class.
PrivateActionToolButton {
kirigamiAction: the action
background: null
}
that should already manage everything about the action (and with background:null to disable any kind of button background

This revision now requires changes to proceed.Feb 9 2019, 10:09 AM
mart added a comment.Feb 14 2019, 10:19 AM

also, the name should possibily not conflict with QQC2 TextField, like ActionTextField or something like that.

src/controls/TextField.qml
21 ↗(On Diff #51215)

always namespace the controls import as
import QtQuick.Controls 2.1 as Controls

89 ↗(On Diff #51215)

the style already sets the correct values for duration and timeout, this line should be removed

106 ↗(On Diff #51215)

text will go under both buttons, in order to avoid that, probably anchors should be added to the TextField's contentItem to make space for the buttons.

mart added inline comments.Feb 14 2019, 10:41 AM
src/controls/TextField.qml
106 ↗(On Diff #51215)

err, TextField is not a Control, right.

so, it should set leftPadding and rightPadding to the space of the buttons

@mart thanks for the review, I don't have enough time this weekend, but I will start working on it next Tuesday :)

ognarb updated this revision to Diff 52125.Feb 19 2019, 11:46 PM
ognarb marked 2 inline comments as done.
  • use Controls namespace
  • use ActionTextField instead of TextField
ognarb planned changes to this revision.Feb 19 2019, 11:46 PM
ognarb retitled this revision from Add a TextField component to Add an ActionTextField component.
ognarb updated this revision to Diff 52127.Feb 20 2019, 12:42 AM

WIP: use repeater and list of actions

ognarb planned changes to this revision.Feb 20 2019, 9:47 AM
ognarb updated this revision to Diff 52141.EditedFeb 20 2019, 1:42 PM
  • Use list of actions
  • Text don't go under buttons now
  • Fix some warning in the testcase Sidebar.qml

@mart That do you recommand me to use for the icons instead of ToolButton? I tried to do it with Kirigami.Icon and a MouseArea but then I add some alignment problem. Does Kirigami already provide some sort of button (click event + icon) without button styling?

Even though we use clickable icons rather than toolbuttons in the QWidgets version, I think we should use ToolButtons if the visual style isn't too bad. People complained about the lack of pressed and hover states when the SwipeListItem used clickable icons instead of toolbuttons.

Then again there's an argument to be made for consistency. I would be okay with using clickable icons instead of toolbuttons as long as we make sure to use a pointing hand cursor when hovered.

ognarb updated this revision to Diff 52204.Feb 21 2019, 1:14 PM
  • Use Kirigami.Icon instead of Controls.ToolBouton
  • Fix doc
mart added a comment.Feb 21 2019, 3:22 PM

it''s aaaalmost perfect,
just a little point on the icon loading, then is good to go

examples/simpleexamples/MultipleColumnsGallery.qml
61 ↗(On Diff #52204)

unrelated change, separate commit?

src/controls/ActionTextField.qml
103 ↗(On Diff #52204)

icon.name, so will work also with qqc2 actions

or better:
source: modelData.icon.name.length > 0 ? modelData.icon.name : modelData.icon.source

124 ↗(On Diff #52204)

icon.name

or better:
source: modelData.icon.name.length > 0 ? modelData.icon.name : modelData.icon.source

src/kirigamiplugin.cpp
181 ↗(On Diff #51215)

2.7

ognarb updated this revision to Diff 52214.Feb 21 2019, 4:16 PM
ognarb marked 2 inline comments as done.
  • Revert unrelated change
  • Use icon.name instead of iconName
ognarb edited the summary of this revision. (Show Details)Feb 21 2019, 4:33 PM
ognarb updated this revision to Diff 52411.Feb 23 2019, 9:29 PM
  • Fix typo in doc
  • Code refactorization
mart accepted this revision.Feb 25 2019, 9:28 AM
This revision is now accepted and ready to land.Feb 25 2019, 9:28 AM
This revision was automatically updated to reflect the committed changes.