Introduce SettingState* elements to ease KCM writing
ClosedPublic

Authored by ervin on Mar 4 2020, 4:42 PM.

Details

Summary

This is the QML based counterpart of D27540. Unlike with the KCModule
case, this is not automatically propagated to the ConfigModules, they
will all have to be adapted to make use of it.

I provide another patch which ports a few ConfigModule to see
how it looks: D27841.

Diff Detail

Repository
R296 KDeclarative
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ervin created this revision.Mar 4 2020, 4:42 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 4 2020, 4:42 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ervin requested review of this revision.Mar 4 2020, 4:42 PM

This patch doesn't apply on top of KDeclarative for me:

    This diff is against commit 3d8757d5dfea2360304e2c8e7d0d575d04b00735, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (a1282da765c1b909d03d6c94eb77fd99e4374d74)
    [Y/n] y

Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.h...
Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp...
Checking patch src/qmlcontrols/kcmcontrols/qml/qmldir...
Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml...
Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml...
Checking patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp...
Checking patch src/qmlcontrols/kcmcontrols/CMakeLists.txt...
error: while searching for:

set(kcmcontrols_SRCS
    kcmcontrolsplugin.cpp
)

add_library(kcmcontrolsplugin SHARED ${kcmcontrols_SRCS})

error: patch failed: src/qmlcontrols/kcmcontrols/CMakeLists.txt:2
Hunk #2 succeeded at 12 (offset -1 lines).
Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.h cleanly.
Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/qmldir cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml cleanly.
Applied patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp cleanly.
Applying patch src/qmlcontrols/kcmcontrols/CMakeLists.txt with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.

 Patch Failed!
ervin added a comment.Mar 5 2020, 5:22 PM

This patch doesn't apply on top of KDeclarative for me:

    This diff is against commit 3d8757d5dfea2360304e2c8e7d0d575d04b00735, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (a1282da765c1b909d03d6c94eb77fd99e4374d74)
    [Y/n] y

Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.h...
Checking patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp...
Checking patch src/qmlcontrols/kcmcontrols/qml/qmldir...
Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml...
Checking patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml...
Checking patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp...
Checking patch src/qmlcontrols/kcmcontrols/CMakeLists.txt...
error: while searching for:

set(kcmcontrols_SRCS
    kcmcontrolsplugin.cpp
)

add_library(kcmcontrolsplugin SHARED ${kcmcontrols_SRCS})

error: patch failed: src/qmlcontrols/kcmcontrols/CMakeLists.txt:2
Hunk #2 succeeded at 12 (offset -1 lines).
Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.h cleanly.
Applied patch src/qmlcontrols/kcmcontrols/settingstateproxy.cpp cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/qmldir cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml cleanly.
Applied patch src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml cleanly.
Applied patch src/qmlcontrols/kcmcontrols/kcmcontrolsplugin.cpp cleanly.
Applying patch src/qmlcontrols/kcmcontrols/CMakeLists.txt with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.

 Patch Failed!

Ah right, sorry it depends on D27839.

bport accepted this revision.Mar 17 2020, 9:20 AM

From code POV this is ok, but neet to wait feedback from VDG to adapt state indicator

This revision is now accepted and ready to land.Mar 17 2020, 9:20 AM
ervin updated this revision to Diff 77848.Mar 17 2020, 5:04 PM

Take feedback about the GUI into account

ervin updated this revision to Diff 78469.Mar 25 2020, 3:52 PM

Have the indicators line up vertically automatically when applicable

davidedmundson added inline comments.Mar 27 2020, 9:49 AM
src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
31

This is a faux-button.

Which means it needs all the relevant:
Accessible.blah

roles to be manually added

Should it be in the tab focus chain?

(or we could just inherit ToolButton)

39

Setting root width/height inside a component is almost always not what you want.

Use implicitWidth/implicitHeight.

Cool

src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
41

*whose

59

This description didn't really help me in understanding what it does until I read the code further down.

76

Can you explain this.
Can't you

indicatorComponent.createObject(target, {
    settingsState: settingsState
});
92

We could also bind parent: target instead?

98

Please check if this works with right-to-left languages (run with -reverse argument to test)

99

Always Math.round whenever you divide sizes to ensure integer alignment

src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
39

Probably should be Kirigami.Units.iconSizes.small.
Generally, when creating reusable components, avoid giving them a width and height.
Instead, define an implicitWidth and implicitHeight.

41

Perhaps bind to icon.valid?

49

You could make the root Item a MouseArea instead.
Also, I think this should get some kind of hover and/or pressed indication?
Also, what about Accessible and keyboard focus? Should this be a proper ToolButton control instead?

src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
31

Odd. Wouldn't we just get QQuickRowLayout et al? Or is that if you do a custom item with a Layout as a base?

83

Too bad QQuickItemChangeListener is private.

Also, does any of this need event compression?

91

Check if it actually changed before emitting

108

Check if it actually changed before emitting

src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h
30

Coding style, QQuickItem *target

ervin marked 15 inline comments as done.Mar 27 2020, 7:50 PM
ervin added inline comments.
src/qmlcontrols/kcmcontrols/qml/SettingStateBinding.qml
59

Went for something much more verbose, but at least it says what it does.

76

Good point.

92

Tried it and QQuickItem was very angry at me, I didn't push further. :-)

98

Again that's one of those comments I'd have appreciated on the widgets version a little while ago. ;-)

src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml
31

Note this is the case for the QtWidgets implementation as well. I switched to ToolButton here but I likely need to do something similar in my other patch... would have been nice to have this comment earlier there (it's been ready a bit longer), you guys have a bias against widgets based patches. ;-)

I think it shouldn't temper with focus at all though, this is almost impossible to get correct in that context I think, so it'll simply reject focus (not too much of an issue in that context IMHO).

41

Done completely differently with ToolButton anyway.

49

See my reply to @davidedmundson above, I switched to ToolButton.

src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp
31

Or if you make something like Kirigami.FormLayout which has Item as base... obviously it's mostly an heuristic so can't be 100% perfect.

83

That really didn't feel like it required event compression to be honest at least I couldn't perceive any visible impact.

91

Well it will have changed in 90% of the cases and checking means doing the computation anyway, which will happen when the getter is called. We'd pay the price twice.

An option is of course to cache the result of the getter to the price of higher complexity. Again I didn't perceive any visible impact on use. Should I go for it anyway?

Note I'm not at all denying this implementation is in some way inefficient, I'm just pointing out the trade off in term of readability and maintainability of the system.

108

Same answer than the previous such case.

ervin updated this revision to Diff 78698.Mar 27 2020, 7:50 PM
ervin marked 10 inline comments as done.

Addresses Kai and David comments.

ervin updated this revision to Diff 80305.Apr 16 2020, 6:15 PM

Fix issues found with Cyril's patches on various KCMs.

davidedmundson accepted this revision.Apr 20 2020, 1:42 PM
This revision was automatically updated to reflect the committed changes.