ervin marked 15 inline comments as done.
ervin added inline comments.
INLINE COMMENTS
> broulik wrote in SettingStateBinding.qml:58
> This description didn't really help me in understanding what it does until I
> read the code further down.
Went for something much more verbose, but at least it says what it does.
> broulik wrote in SettingStateBinding.qml:75
> Can you explain this.
> Can't you
>
> indicatorComponent.createObject(target, {
> settingsState: settingsState
> });
Good point.
> broulik wrote in SettingStateBinding.qml:91
> We could also bind `parent: target` instead?
Tried it and QQuickItem was very angry at me, I didn't push further. :-)
> broulik wrote in SettingStateBinding.qml:97
> Please check if this works with right-to-left languages (run with `-reverse`
> argument to test)
Again that's one of those comments I'd have appreciated on the widgets version
a little while ago. ;-)
> davidedmundson wrote in SettingStateIndicator.qml:30
> 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)
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).
> broulik wrote in SettingStateIndicator.qml:40
> Perhaps bind to `icon.valid`?
Done completely differently with ToolButton anyway.
> broulik wrote in SettingStateIndicator.qml:48
> 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?
See my reply to @davidedmundson above, I switched to ToolButton.
> broulik wrote in settingstatebindingprivate.cpp:30
> Odd. Wouldn't we just get `QQuickRowLayout` et al? Or is that if you do a
> custom item with a `Layout` as a base?
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.
> broulik wrote in settingstatebindingprivate.cpp:82
> Too bad `QQuickItemChangeListener` is private.
>
> Also, does any of this need event compression?
That really didn't feel like it required event compression to be honest at
least I couldn't perceive any visible impact.
> broulik wrote in settingstatebindingprivate.cpp:90
> Check if it actually changed before emitting
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.
> broulik wrote in settingstatebindingprivate.cpp:107
> Check if it actually changed before emitting
Same answer than the previous such case.
REPOSITORY
R296 KDeclarative
REVISION DETAIL
https://phabricator.kde.org/D27840
To: ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham,
#frameworks, #plasma
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,
bruns
