Details
- Reviewers
crossi hchain meven bport davidedmundson mart ngraham - Group Reviewers
Frameworks Plasma - Commits
- R296:24211925f835: Introduce SettingState* elements to ease KCM writing
Diff Detail
- Repository
- R296 KDeclarative
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 23263 Build 23281: arc lint + arc unit
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!
From code POV this is ok, but neet to wait feedback from VDG to adapt state indicator
src/qmlcontrols/kcmcontrols/qml/SettingStateIndicator.qml | ||
---|---|---|
31 | This is a faux-button. Which means it needs all the relevant: roles to be manually added Should it be in the tab focus chain? (or we could just inherit ToolButton) | |
38 | 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. 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. | |
41 | Perhaps bind to icon.valid? | |
49 | You could make the root Item a MouseArea instead. | |
src/qmlcontrols/kcmcontrols/settingstatebindingprivate.cpp | ||
30 ↗ | (On Diff #78469) | Odd. Wouldn't we just get QQuickRowLayout et al? Or is that if you do a custom item with a Layout as a base? |
82 ↗ | (On Diff #78469) | Too bad QQuickItemChangeListener is private. Also, does any of this need event compression? |
90 ↗ | (On Diff #78469) | Check if it actually changed before emitting |
107 ↗ | (On Diff #78469) | Check if it actually changed before emitting |
src/qmlcontrols/kcmcontrols/settingstatebindingprivate.h | ||
29 ↗ | (On Diff #78469) | Coding style, QQuickItem *target |
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 | ||
30 ↗ | (On Diff #78469) | 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. |
82 ↗ | (On Diff #78469) | That really didn't feel like it required event compression to be honest at least I couldn't perceive any visible impact. |
90 ↗ | (On Diff #78469) | 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. |
107 ↗ | (On Diff #78469) | Same answer than the previous such case. |