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

Changeset View

Standalone View

View Options

src/controls/templates/FormLayout.qml

Show All 36 Lines32 GridLayout {
37 columnSpacing: Kirigami.Units.smallSpacing 37​ columnSpacing: Kirigami.Units.smallSpacing
38 property var knownItems: [] 38​ property var knownItems: []
39 anchors { 39​ anchors {
40 left: parent.left 40​ left: parent.left
41 top: parent.top 41​ top: parent.top
42 right: parent.right 42​ right: parent.right
43 } 43​ }
44 44​
45 Timer { 45​ Timer {
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
46 id: hintCompression 46​ id: hintCompression
47 onTriggered: { 47​ onTriggered: {
48 if (root.wideMode) { 48​ if (root.wideMode) {
49 lay.wideImplicitWidth = lay.implicitWidth; 49​ lay.wideImplicitWidth = lay.implicitWidth;
50 } 50​ }
51 } 51​ }
52 } 52​ }
53 onImplicitWidthChanged: hintCompression.restart(); 53​ onImplicitWidthChanged: hintCompression.restart();
54 Component.onCompleted: wideImplicitWidth = lay.implicitWidth; 54​ Component.onCompleted: wideImplicitWidth = lay.implicitWidth;
55 } 55​ }
56 56​
57 Item { 57​ Item {
58 id: temp 58​ id: temp
59 } 59​ }
60 60​
61 Timer { 61​ Timer {
62 id: relayoutTimer 62​ id: relayoutTimer
63 interval: 0 63​ interval: 0
64 onTriggered: { 64​ onTriggered: {
65 var __items = children; 65​ var __items = children;
66 //exclude the layout and temp 66​ //exclude the layout and temp
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…
67 for (var i = 2; i < __items.length; ++i) { 67​ for (var i = 2; i < __items.length; ++i) {
68 var item = __items[i]; 68​ var item = __items[i];
69 69​
70 //skip items that are already there 70​ //skip items that are already there
71 if (lay.knownItems.indexOf(item) != -1 || 71​ if (lay.knownItems.indexOf(item) != -1 ||
davidedmundsonUnsubmitted
Not Done

typo

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