Differential D8641 Diff 22462 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​Control {
26​ id: root
27​ implicitWidth: lay.implicitWidth
28​ implicitHeight: lay.implicitHeight
29​ Layout.preferredHeight: lay.implicitHeight
30​ property bool wideMode: width >= lay.wideImplicitWidth
31​
32​ GridLayout {
33​ id: lay
34​ property int wideImplicitWidth
35​ columns: root.wideMode ? 2 : 1
36​ rowSpacing: Kirigami.Units.smallSpacing
37​ columnSpacing: Kirigami.Units.smallSpacing
38​ property var knownItems: []
39​ anchors {
40​ left: parent.left
41​ top: parent.top
42​ right: parent.right
43​ }
44​
45​ Timer {
46​ id: hintCompression
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​ onTriggered: {
48​ if (root.wideMode) {
49​ lay.wideImplicitWidth = lay.implicitWidth;
50​ }
51​ }
52​ }
53​ onImplicitWidthChanged: hintCompression.restart();
54​ Component.onCompleted: wideImplicitWidth = lay.implicitWidth;
55​ }
56​
57​ Item {
58​ id: temp
59​ }
60​
61​ Timer {
62​ id: relayoutTimer
63​ interval: 0
64​ onTriggered: {
65​ var __items = children;
66​ //exclude the layout and temp
67​ for (var i = 2; i < __items.length; ++i) {
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​ var item = __items[i];
69​
70​ //skip items that are already there
71​ if (lay.knownItems.indexOf(item) != -1 ||
72​ //exclude Repeaters
davidedmundsonUnsubmitted
Not Done

typo

davidedmundson: typo
73​ //NOTE: this is an heuristic but there are't better ways
74​ (item.model !== undefined && item.children.length == 0)) {
75​ continue;
76​ }
77​ lay.knownItems.push(item);
78​
79​ var itemContainer = itemComponent.createObject(temp, {"item": item})
80​
81​ //if section, label goes after the separator
82​ if (item.Kirigami.FormData.isSection) {
83​ //put an extra spacer
84​ var placeHolder = placeHolderComponent.createObject(lay, {"item": item});
85​ placeHolder.Layout.colSpan = 2;
86​ itemContainer.parent = lay;
87​ }
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​
89​ var buddy = buddyComponent.createObject(lay, {"item": item})
90​
91​ itemContainer.parent = lay;
92​ //if section, wee need another placeholder
93​ if (item.Kirigami.FormData.isSection) {
94​ var placeHolder = placeHolderComponent.createObject(lay, {"item": item});
95​ placeHolder.parent = lay;
96​ }
97​ }
98​ }
99​ }
100​
101​ onChildrenChanged: relayoutTimer.restart();
102​
103​ Component.onCompleted: childrenChanged()
104​ Component {
105​ id: itemComponent
106​ Item {
107​ id: container
108​ property var item
109​ enabled: item.enabled
110​ visible: item.visible
111​ implicitWidth: item.implicitWidth
112​ Layout.preferredWidth: item.Layout.preferredWidth
113​ Layout.preferredHeight: Math.max(item.Layout.preferredHeight, item.implicitHeight)
114​
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​ Layout.alignment: (root.wideMode ? Qt.AlignLeft | Qt.AlignVCenter : Qt.AlignHCenter | Qt.AlignTop)
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​ Layout.fillWidth: item.Kirigami.FormData.isSection
117​ Layout.columnSpan: item.Kirigami.FormData.isSection ? lay.columns : 1
118​ onItemChanged: {
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​ if (!item) {
120​ container.destroy();
121​ }
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​ }
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​ onXChanged: item.x = x;
124​ onYChanged: item.y = y;
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​ onWidthChanged: item.width = width;
126​ }
127​ }
128​ Component {
129​ id: placeHolderComponent
130​ Item {
131​ property var item
132​ enabled: item.enabled
133​ visible: item.visible
134​ width: Kirigami.Units.smallSpacing
135​ height: Kirigami.Units.smallSpacing
136​ onItemChanged: {
137​ if (!item) {
138​ labelItem.destroy();
139​ }
140​ }
141​ }
142​ }
143​ Component {
144​ id: buddyComponent
145​ Kirigami.Heading {
146​ id: labelItem
147​
148​ property var item
149​ enabled: item.enabled
150​ visible: item.visible
151​ Kirigami.MnemonicData.enabled: item.Kirigami.FormData.buddyFor && item.Kirigami.FormData.buddyFor.activeFocusOnTab
152​ Kirigami.MnemonicData.controlType: Kirigami.MnemonicData.FormLabel
153​ Kirigami.MnemonicData.label: item.Kirigami.FormData.label
154​ text: Kirigami.MnemonicData.richTextLabel
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​
156​ level: item.Kirigami.FormData.isSection ? 3 : 5
157​
158​ Layout.preferredHeight: item.Kirigami.FormData.label.length > 0 ? implicitHeight : Kirigami.Units.smallSpacing
159​
160​ Layout.alignment: root.wideMode
161​ ? (Qt.AlignRight | (item.Kirigami.FormData.buddyFor.height > height * 2 ? Qt.AlignTop : Qt.AlignVCenter))
162​ : (Qt.AlignLeft | Qt.AlignBottom)
163​ verticalAlignment: root.wideMode ? Text.AlignVCenter : Text.AlignBottom
164​
165​ Layout.topMargin: item.Kirigami.FormData.buddyFor.height > implicitHeight * 2 ? Kirigami.Units.smallSpacing/2 : 0
166​ onItemChanged: {
167​ if (!item) {
168​ labelItem.destroy();
169​ }
170​ }
171​ Shortcut {
172​ sequence: labelItem.Kirigami.MnemonicData.sequence
173​ onActivated: item.Kirigami.FormData.buddyFor.forceActiveFocus()
174​ }
175​ }
176​ }
177​}
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