Differential D8641 Diff 23317 src/controls/templates/FormLayout.qml

Changeset View

Standalone View

View Options

src/controls/templates/FormLayout.qml

  • This file was added.
1​/*
2​ * Copyright 2017 Marco Martin <mart@kde.org>
3​ *
4​ * This program is free software; you can redistribute it and/or modify
5​ * it under the terms of the GNU Library General Public License as
6​ * published by the Free Software Foundation; either version 2 or
7​ * (at your option) any later version.
8​ *
9​ * This program is distributed in the hope that it will be useful,
10​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
11​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12​ * GNU Library General Public License for more details
13​ *
14​ * You should have received a copy of the GNU Library General Public
15​ * License along with this program; if not, write to the
16​ * Free Software Foundation, Inc.,
17​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
18​ */
19​
20​import QtQuick 2.6
21​import QtQuick.Layouts 1.2
22​import QtQuick.Controls 2.2
23​import org.kde.kirigami 2.3 as Kirigami
24​
25​/**
26​ * This is the base class for Form layouts conforming to the
27​ * Kirigami Human interface guidelines. The layout will
28​ * be divided in two columns: on the right there will be a column
29​ * of fields, on the left their labels specified in the FormData attached
30​ * property.
31​ *
32​ * Example:
33​ * @code
34​ * import org.kde.kirigami 2.3 as Kirigami
35​ * Kirigami.FormLayout {
36​ * TextField {
37​ * Kirigami.FormData.label: "Label:"
38​ * }
39​ * Kirigami.Separator {
40​ * Kirigami.FormData.label: "Section Title"
41​ * Kirigami.FormData.isSection: true
42​ * }
43​ * TextField {
44​ * Kirigami.FormData.label: "Label:"
45​ * }
46​ * TextField {
broulikUnsubmitted
Not Done

Use Qt.callLater (new in Qt 5.8) if you pass it an actual function (ie. not an inline one) it will compress subsequent calls to one: https://doc.qt.io/qt-5/qml-qtqml-qt.html#callLater-method

There's probably more places in the code where you could do this.

broulik: Use `Qt.callLater` (new in Qt 5.8) if you pass it an actual function (ie. not an inline one) it…
martAuthorUnsubmitted
Not Done

it's in frameworks, so i want to keep it still working as much as possible with 5.7

mart: it's in frameworks, so i want to keep it still working as much as possible with 5.7
47​ * }
48​ * }
49​ * @endcode
50​ * @inherits QtQuick.Controls.Control
51​ * @since 2.3
52​ */
53​Control {
54​ id: root
55​
56​ /**
57​ * wideMode: bool
58​ * If true the layout will be optimized for a wide screen, such as
59​ * a desktop machine (the labels will be on a left column,
60​ * the fields on a right column beside it), if false (such as on a phone)
61​ * everything is laid out in a single column.
62​ * by default this will be based on wether the application is
63​ * wide enough for the layout of being in such mode.
64​ * It can be overridden by reasigning the property
65​ */
66​ property bool wideMode: width >= lay.wideImplicitWidth
67​
heinUnsubmitted
Not Done

Doesn't Kirigami have wideMode logic somewhere else too?

hein: Doesn't Kirigami have wideMode logic somewhere else too?
martAuthorUnsubmitted
Not Done

yes, they are in some controls, independent from each other

mart: yes, they are in some controls, independent from each other
heinUnsubmitted
Not Done

Should it be centralized or ...?

hein: Should it be centralized or ...?
martAuthorUnsubmitted
Not Done

I don't think so, reasons for switching to one or the other are different.
this one is purely wether the contents fit or not, the global one for the app which is how many columns are shown in the pagerow is depending on a fixed size in gridunits, tough if this depended only from that it may lead to layouting bugs, as when the label+field don't fit in a single line it's absolutely necessary it switches to the compact mode.
on some forms it will tens more to switch, in germal it will end up more in compact... but if it doesn't fit it doesn't fit (the alternative would be i guess to elide labels, but i think it's worse?)

mart: I don't think so, reasons for switching to one or the other are different. this one is purely…
68​ implicitWidth: lay.implicitWidth
69​ implicitHeight: lay.implicitHeight
70​ Layout.preferredHeight: lay.implicitHeight
71​
72​ GridLayout {
davidedmundsonUnsubmitted
Not Done

typo

davidedmundson: typo
73​ id: lay
74​ property int wideImplicitWidth
75​ columns: root.wideMode ? 2 : 1
76​ rowSpacing: Kirigami.Units.smallSpacing
77​ columnSpacing: Kirigami.Units.smallSpacing
78​ property var knownItems: []
79​ anchors {
80​ left: parent.left
81​ top: parent.top
82​ right: parent.right
83​ }
84​
85​ Timer {
86​ id: hintCompression
87​ onTriggered: {
davidedmundsonUnsubmitted
Not Done

why do something different for width and height?

Also I think we're better off doing

implicitheight: item.implicitHeight
Layout.preferredheight: item.Layout.preferredHeight

For layouts. Layout.preferred takes precedence over implicit if set, I think it makes sense to just copy that.

davidedmundson: why do something different for width and height? Also I think we're better off doing…
88​ if (root.wideMode) {
89​ lay.wideImplicitWidth = lay.implicitWidth;
90​ }
91​ }
92​ }
93​ onImplicitWidthChanged: hintCompression.restart();
94​ Component.onCompleted: wideImplicitWidth = lay.implicitWidth;
95​ }
96​
97​ Item {
98​ id: temp
99​ }
100​
101​ Timer {
102​ id: relayoutTimer
103​ interval: 0
104​ onTriggered: {
105​ var __items = children;
106​ //exclude the layout and temp
107​ for (var i = 2; i < __items.length; ++i) {
108​ var item = __items[i];
109​
110​ //skip items that are already there
111​ if (lay.knownItems.indexOf(item) != -1 ||
112​ //exclude Repeaters
113​ //NOTE: this is an heuristic but there are't better ways
114​ (item.hasOwnProperty("model") && item.model !== undefined && item.children.length == 0)) {
davidedmundsonUnsubmitted
Not Done

I'd like it if we can make this on the desktop style look the same as QGroupBox looks in flat mode. Even if that means changing our QStyle.

Currently this means spanning both columns and then centre aligning.

davidedmundson: I'd like it if we can make this on the desktop style look the same as QGroupBox looks in flat…
martAuthorUnsubmitted
Not Done

any example how a groupbox looks in flat mode?

mart: any example how a groupbox looks in flat mode?
davidedmundsonUnsubmitted
Not Done

Flat mode:

main page of current mouse KCM:
button order, icons.

davidedmundson: Flat mode: main page of current mouse KCM: button order, icons.
davidedmundsonUnsubmitted
Not Done

Is this done?

davidedmundson: Is this done?
martAuthorUnsubmitted
Not Done

no, will do a related patch to breeze

mart: no, will do a related patch to breeze
115​ continue;
heinUnsubmitted
Not Done

Isn't this going to cause tons of ReferenceErrors without "x in y" style checks?

hein: Isn't this going to cause tons of ReferenceErrors without "x in y" style checks?
martAuthorUnsubmitted
Not Done

what's x in y?

mart: what's x in y?
heinUnsubmitted
Not Done

I meant the "in" operator, or alternatively object.hasOwnProperty("foo").

hein: I meant the "in" operator, or alternatively object.hasOwnProperty("foo").
116​ }
117​ lay.knownItems.push(item);
118​
davidedmundsonUnsubmitted
Not Done

visible: item.visible might make sense.

it allows a dev to hide a UI component and label if it's not relevantl together

davidedmundson: visible: item.visible might make sense. it allows a dev to hide a UI component and label if…
119​ var itemContainer = itemComponent.createObject(temp, {"item": item})
120​
121​ //if section, label goes after the separator
davidedmundsonUnsubmitted
Not Done

why are we giving it a height if it's empty?

davidedmundson: why are we giving it a height if it's empty?
martAuthorUnsubmitted
Not Done

to recycle it as spacer if there is no text (instantiating a different item if there is no label would be both more complex and wouldn't allow for change at runtime)

mart: to recycle it as spacer if there is no text (instantiating a different item if there is no…
122​ if (item.Kirigami.FormData.isSection) {
davidedmundsonUnsubmitted
Not Done

In general with text, I think it's better to set the item to occupy the whole space it /can/ take up, then use the Text.Alignment property to position the text within it.

Otherwise if this wraps, your second line would start on the left not the right.

Related; what should happen if a user sets a really really really long label?

davidedmundson: In general with text, I think it's better to set the item to occupy the whole space it /can/…
martAuthorUnsubmitted
Not Done

I tried that, but it messes with the gridlayout size hints and then i get the whole layout aligned at the right of the parent instead of the left

mart: I tried that, but it messes with the gridlayout size hints and then i get the whole layout…
davidedmundsonUnsubmitted
Not Done

I think you'll need to try again.

Have you tested this code with a really really long label?

davidedmundson: I think you'll need to try again. Have you tested this code with a really really long label?
martAuthorUnsubmitted
Not Done

a really long label will make the form switch to compact mode, which is what's expected.
making the label flexible aligns the whole layout to the right, which is exactly what's expected to happen in a GridLayout.

mart: a really long label will make the form switch to compact mode, which is what's expected. making…
123​ //put an extra spacer
124​ var placeHolder = placeHolderComponent.createObject(lay, {"item": item});
davidedmundsonUnsubmitted
Not Done

What's the rationale for this "if buddy.height > height *2" stuff?

davidedmundson: What's the rationale for this "if buddy.height > height *2" stuff?
martAuthorUnsubmitted
Not Done

when the item is very tall (for instance a columnlayout of other items such as is necessary for radiobuttons to work) to align the label at top instead at the middle, which looks weird

mart: when the item is very tall (for instance a columnlayout of other items such as is necessary for…
125​ placeHolder.Layout.colSpan = 2;
126​ itemContainer.parent = lay;
127​ }
128​
129​ var buddy = buddyComponent.createObject(lay, {"item": item})
130​
131​ itemContainer.parent = lay;
132​ //if section, wee need another placeholder
133​ if (item.Kirigami.FormData.isSection) {
134​ var placeHolder = placeHolderComponent.createObject(lay, {"item": item});
135​ placeHolder.parent = lay;
136​ }
137​ }
138​ }
139​ }
140​
141​ onChildrenChanged: relayoutTimer.restart();
142​
143​ Component.onCompleted: relayoutTimer.triggered();
144​ Component {
145​ id: itemComponent
146​ Item {
147​ id: container
148​ property var item
149​ enabled: item.enabled
150​ visible: item.visible
151​ implicitWidth: item.implicitWidth
152​ Layout.preferredWidth: item.Layout.preferredWidth > 0 ? item.Layout.preferredWidth : item.implicitWidth
153​ Layout.preferredHeight: item.Layout.preferredHeight > 0 ? item.Layout.preferredHeight : item.implicitHeight
154​
davidedmundsonUnsubmitted
Not Done

I left a comment in my very first review of this!

Stil not addressed.

davidedmundson: I left a comment in my very first review of this! Stil not addressed.
martAuthorUnsubmitted
Not Done

I made now "prefer" preferredheight to implicitheight, but sometimes layout.preferredheight is not set, and it needs to fallback to implicitheight hor the layout will look completely broken

mart: I made now "prefer" preferredheight to implicitheight, but sometimes layout.preferredheight is…
davidedmundsonUnsubmitted
Not Done

why are we doing something different for width and height

davidedmundson: >why are we doing something different for width and height
martAuthorUnsubmitted
Not Done

indeed the width should have the same logic, which now does,the width simply didn't trigger visible layout bugs

mart: indeed the width should have the same logic, which now does,the width simply didn't trigger…
155​ Layout.alignment: (root.wideMode ? Qt.AlignLeft | Qt.AlignVCenter : Qt.AlignHCenter | Qt.AlignTop)
156​ Layout.fillWidth: item.Kirigami.FormData.isSection
157​ Layout.columnSpan: item.Kirigami.FormData.isSection ? lay.columns : 1
158​ onItemChanged: {
159​ if (!item) {
160​ container.destroy();
161​ }
162​ }
163​ onXChanged: item.x = x;
164​ onYChanged: item.y = y;
165​ onWidthChanged: item.width = width;
166​ }
167​ }
168​ Component {
169​ id: placeHolderComponent
170​ Item {
171​ property var item
172​ enabled: item.enabled
173​ visible: item.visible
174​ width: Kirigami.Units.smallSpacing
175​ height: Kirigami.Units.smallSpacing
176​ onItemChanged: {
177​ if (!item) {
178​ labelItem.destroy();
179​ }
180​ }
181​ }
182​ }
183​ Component {
184​ id: buddyComponent
185​ Kirigami.Heading {
186​ id: labelItem
187​
188​ property var item
189​ enabled: item.enabled
190​ visible: item.visible
191​ Kirigami.MnemonicData.enabled: item.Kirigami.FormData.buddyFor && item.Kirigami.FormData.buddyFor.activeFocusOnTab
192​ Kirigami.MnemonicData.controlType: Kirigami.MnemonicData.FormLabel
193​ Kirigami.MnemonicData.label: item.Kirigami.FormData.label
194​ text: Kirigami.MnemonicData.richTextLabel
195​
196​ level: item.Kirigami.FormData.isSection ? 3 : 5
197​
198​ Layout.preferredHeight: item.Kirigami.FormData.label.length > 0 ? implicitHeight : Kirigami.Units.smallSpacing
199​
200​ Layout.alignment: root.wideMode
201​ ? (Qt.AlignRight | (item.Kirigami.FormData.buddyFor.height > height * 2 ? Qt.AlignTop : Qt.AlignVCenter))
202​ : (Qt.AlignLeft | Qt.AlignBottom)
203​ verticalAlignment: root.wideMode ? Text.AlignVCenter : Text.AlignBottom
204​
davidedmundsonUnsubmitted
Not Done

you should be able to just

Layout.fillHeight: true

then do all this in the verticalAlignment

davidedmundson: you should be able to just Layout.fillHeight: true then do all this in the verticalAlignment
205​ Layout.topMargin: item.Kirigami.FormData.buddyFor.height > implicitHeight * 2 ? Kirigami.Units.smallSpacing/2 : 0
206​ onItemChanged: {
207​ if (!item) {
208​ labelItem.destroy();
209​ }
210​ }
211​ Shortcut {
212​ sequence: labelItem.Kirigami.MnemonicData.sequence
213​ onActivated: item.Kirigami.FormData.buddyFor.forceActiveFocus()
214​ }
215​ }
216​ }
217​}