FormLayout
ClosedPublic

Authored by mart on Nov 3 2017, 3:17 PM.

Details

Summary

Add a new component called FormLayout, which aims to
behave similar to Qwidget's QFormLayout: 2 columns
with labels aligned accordingly to the HIG
when the available space is too small, collapse
to one column only, intended especially for mobile.

Buddy items don't yet have keyboard shortcuts, but apart text decoration, they shouldbe easy to implement.

intended usage is based on the FormData attached property:
FormLayout {

 TextField {
     FormData.label: "Label:"
 }
 //separed by a line, no titles
 Kirigami.Separator {
     Kirigami.FormData.isSection: true
 }
 TextField {
     FormData.label: "Label2:"
 }
 //separed by a line, and a title
 Kirigami.Separator {
     Kirigami.FormData.isSection: true
     Kirigami.FormData.label: "Section title"
 }
}
Test Plan

the gallery has a FormLayout page used to test it

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
mart reopened this revision.Nov 14 2017, 10:50 AM
mart updated this revision to Diff 22324.Nov 14 2017, 10:50 AM
  • an early prototype of a FormLayout
  • Form->FormData
  • add gallery page
  • make FormLayout a template
  • use Headings for all the titles
  • remove some redundant code
  • support for dynamic elements
  • add heuristic to exclude repeaters from layout
  • event compress relayouts
  • resize items that are separator
  • new way to switch from wide to compact
  • remove debug
  • introduce a mnemonics mechanism
  • split mnemonics in a new attached
  • text 100% wide
  • Revert "text 100% wide"
  • remove dead code
mart added inline comments.Nov 14 2017, 10:53 AM
src/controls/templates/FormLayout.qml
122

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 added a comment.Nov 14 2017, 11:02 AM
In D8641#167459, @hein wrote:

So if I understand correctly, the Mnemonic stuff currently doesn't seem to have the same aims as KAcceleratorManager does ...

KAcceleratorManager acts on a hierarchy of widgets, traverses it, assigns weights to actionable things and possible mnemonics and overall smartly tries to distribute mnemonics across the hierarchy.

If I understand correctly this is more like a convenience wrapper around QKeySequence::mnemonic() that you can get both the label and a sequence to pass to shortcut to.

no, it's kindof like kacceleratormanager.. but much more primitive, as it has just a single global pool in which accelerators are unique.

so what it would need and i may try, is to make and hash per different window, and then have something about widgets hyerarchies.
tough i was thinking that may be enough if is global per-window and shortcuts are unique based on only one enabled at the moment (so like if there is a stackview, only the topmost current page would be enabled)

I'm not really sure about those static mappings members ... there currently doesn't seem to be any duplicate mnemonic tracking yet, right?

basically it just avoids any duplicates... first come first served

My feeling is that this is still too much boiler plate for the dev. At least within a FormLayout, which has knowledge of what labels go with what, all the assignment should probably just happen automatically without needing to have your own Shortcut {} instances and getting the labels out of the Mnemonic thing. Do you think we can do that somehow?

there would be boiler plate for who writes their own controls (which oone should try hard to avoid anyways)
using a formlayout just works and for primitive controls should just work automatically as well, if this is done in the style.

Overall the code in KAcceleratorManagement is quite good and nicely commented, there's likely stuff of value in there (e.g. the algorithm it uses to weight mnemonic characters).

i'll look at the algorythm to see if is reusable

I like the idea of the attached mnemonic handler, it's good that you've kept it separate from the form layout.

i'll look at the algorythm to see if is reusable

Can we try to avoid having a diff that keeps growing and growing with features.
We'll end up with bugs in the original form part not getting seen.

It should be easy to keep these mostly separate.

src/controls/templates/FormLayout.qml
114

Flat mode:

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

src/formlayoutattached.h
34

constant

src/mnemonicattached.cpp
31

we can filter the window, instead of the app.

It's massively faster; qapp gets a lot of stuff.

50

Note that the alt to show is a Breeze special feature for widgets.

mart updated this revision to Diff 22345.Nov 14 2017, 1:47 PM
  • partial support for weights
  • make the item with more weigth "win"
mart updated this revision to Diff 22401.Nov 15 2017, 4:51 PM
  • mnemoinictext property, which always has the &
  • support mnemonics in the global drawer
  • different weigths for differnt control types
  • filter on the window not the app
mart added inline comments.Nov 15 2017, 4:59 PM
src/mnemonicattached.cpp
50

i think it's fine assuming the style supports that... (and assume that the one which is really supported is breeze) i wouldn't know how to detect it from here?

mart updated this revision to Diff 22410.Nov 15 2017, 6:52 PM
  • && means we want & and not a mnemonic
broulik added inline comments.
examples/gallerydata/contents/ui/gallery/FormLayoutGallery.qml
80

property Item?

That's in a couple of places

src/controls/templates/FormLayout.qml
46

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.

src/formlayoutattached.cpp
29

This class does not have an eventFilter

src/formlayoutattached.h
38

nullptr

39

override

57

Unused, there's also no property.

58

Also unused

62

Unused

63

Unused

src/mnemonicattached.cpp
53

Double lookup (contains+value), also below

63

isEmpty

168

Can we emit this stuff only if actually changed or can this be assumed here?

230

`!isEmpty?

288

(m_weights.constEnd() - 1).key()? Avoids creating a keys() temporary list

src/mnemonicattached.h
39

Use Q_ENUM

49

nullptr

mart updated this revision to Diff 22462.Nov 16 2017, 2:39 PM
  • if there is a rendercontrol, watch the target window
mart updated this revision to Diff 22468.Nov 16 2017, 3:32 PM
mart marked 10 inline comments as done.
  • adress comments
src/controls/templates/FormLayout.qml
46

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

src/formlayoutattached.cpp
29

ah, leftover when the mnemonics stuff was implemented here

src/formlayoutattached.h
57

other leftovers, sorry :)

This revision was automatically updated to reflect the committed changes.
mart reopened this revision.Nov 24 2017, 2:27 PM
davidedmundson requested changes to this revision.Nov 27 2017, 9:26 AM

Still has comments from my original review not replied to.
Maybe using the "done" feature in phab, will make it eaiser to track things?

Also this needs API documentation throughout.

This revision now requires changes to proceed.Nov 27 2017, 9:26 AM
mart updated this revision to Diff 22997.Nov 27 2017, 11:26 AM
mart marked 14 inline comments as done.
  • an early prototype of a FormLayout
  • Form->FormData
  • add gallery page
  • make FormLayout a template
  • use Headings for all the titles
  • remove some redundant code
  • support for dynamic elements
  • add heuristic to exclude repeaters from layout
  • event compress relayouts
  • resize items that are separator
  • new way to switch from wide to compact
  • remove debug
  • introduce a mnemonics mechanism
  • split mnemonics in a new attached
  • text 100% wide
  • Revert "text 100% wide"
  • remove dead code
  • partial support for weights
  • make the item with more weigth "win"
  • mnemoinictext property, which always has the &
  • support mnemonics in the global drawer
  • different weigths for differnt control types
  • filter on the window not the app
  • && means we want & and not a mnemonic
  • if there is a rendercontrol, watch the target window
  • adress comments
  • make sure that in component.oncompleted() things are actually relayouted
  • adress small issues
This revision was automatically updated to reflect the committed changes.
mart reopened this revision.Nov 27 2017, 12:52 PM
mart updated this revision to Diff 23008.Nov 27 2017, 12:52 PM
  • an early prototype of a FormLayout
  • Form->FormData
  • add gallery page
  • make FormLayout a template
  • use Headings for all the titles
  • remove some redundant code
  • support for dynamic elements
  • add heuristic to exclude repeaters from layout
  • event compress relayouts
  • resize items that are separator
  • new way to switch from wide to compact
  • remove debug
  • introduce a mnemonics mechanism
  • split mnemonics in a new attached
  • text 100% wide
  • Revert "text 100% wide"
  • remove dead code
  • partial support for weights
  • make the item with more weigth "win"
  • mnemoinictext property, which always has the &
  • support mnemonics in the global drawer
  • different weigths for differnt control types
  • filter on the window not the app
  • && means we want & and not a mnemonic
  • if there is a rendercontrol, watch the target window
  • adress comments
  • make sure that in component.oncompleted() things are actually relayouted
  • adress small issues
  • document apis
hein added a comment.Nov 27 2017, 2:20 PM

In general it looks quite good to me.

src/controls/templates/FormLayout.qml
67

Doesn't Kirigami have wideMode logic somewhere else too?

115

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

src/mnemonicattached.h
163

This can go with D8827, right?

mart added inline comments.Nov 27 2017, 3:51 PM
src/controls/templates/FormLayout.qml
67

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

115

what's x in y?

src/mnemonicattached.h
163

no, mnemonics support is here, D8827 is the part in qqc2-desktop-style, another repo

mart marked 6 inline comments as done.Nov 27 2017, 3:52 PM
hein added inline comments.Nov 28 2017, 10:23 AM
src/controls/templates/FormLayout.qml
67

Should it be centralized or ...?

115

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

src/mnemonicattached.h
163

Ah, ok, I guess I thought window-level stuff might happen there.

mart updated this revision to Diff 23075.Nov 28 2017, 10:45 AM
  • hasOwnProperty
mart added inline comments.Nov 28 2017, 10:48 AM
src/controls/templates/FormLayout.qml
67

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?)

src/mnemonicattached.h
163

yeah, i plan to add another granularity level to separe mnemonics per-window but since adds more complexity, i agreed beforehand to not make the diff grow more, so i'll push this beforehand and then give it the per-window support

hein accepted this revision.Nov 28 2017, 10:52 AM
davidedmundson requested changes to this revision.Nov 28 2017, 11:03 AM
davidedmundson added inline comments.
src/controls/templates/FormLayout.qml
114

Is this done?

122

I think you'll need to try again.

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

153–154

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

Stil not addressed.

This revision now requires changes to proceed.Nov 28 2017, 11:03 AM
mart updated this revision to Diff 23088.Nov 28 2017, 1:17 PM
  • just use item.Layout.preferredHeight
  • prefer Layout.preferredHeight to implicitHeight
mart added inline comments.Nov 28 2017, 1:19 PM
src/controls/templates/FormLayout.qml
114

no, will do a related patch to breeze

122

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.

153–154

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

davidedmundson added inline comments.Nov 28 2017, 1:52 PM
src/controls/templates/FormLayout.qml
153–154

why are we doing something different for width and height

mart updated this revision to Diff 23092.Nov 28 2017, 2:53 PM
  • same logic for width
mart added inline comments.Nov 28 2017, 2:56 PM
src/controls/templates/FormLayout.qml
153–154

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

davidedmundson accepted this revision.Nov 28 2017, 3:20 PM

Please don't push new API 5 days before a release tagging.

We'll still be able to make the Plasma 5.12 if we wait till Saturday.

src/controls/templates/FormLayout.qml
201–204

you should be able to just

Layout.fillHeight: true

then do all this in the verticalAlignment

This revision is now accepted and ready to land.Nov 28 2017, 3:20 PM
This revision was automatically updated to reflect the committed changes.