Makes some improvements to the Effects KCM (details in title).
Details
- Reviewers
ngraham davidedmundson - Group Reviewers
KWin VDG - Commits
- R108:787c39cd420a: [kcmkwin/compositing] Remove effect list item selection, fix list item size…
Open the Effects KCM.
Diff Detail
- Repository
- R108 KWin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Cool!
Should these list items even become selected when clicked? Typically items are only selectable when there are some actions you can apply to selected items, which isn't the case here.
Also, since this seems to be an open-ended "improve the Effects KCM" patch, I might recommend showing the video in a tastefully constructed pop-up rather than expanding the height of the corresponding list item to accommodate the video, which just doesn't look good.
Remove effect list item selection and use a Breeze BusyIndicator.
Opening a new dialog for the video would need some more work, so it's not included here.
Hmm, why did you change the busy indicator? I think the current one is fine. The new one is something I haven't seen used anywhere else.
I think the default QML busy indicator looks bad and doesn't really fit in Breeze.
What other QML busy indicator could be used?
Ah, much better! This looks great to me now. KWin folks, any any concerns, or should we land this?
kcmkwin/kwincompositing/qml/Effect.qml | ||
---|---|---|
36 | I assume this change is
Why are we removing it? |
kcmkwin/kwincompositing/qml/Effect.qml | ||
---|---|---|
36 | Because UI-wise, there is no point in making something selectable when there is nothing you can do with selected items. |
kcmkwin/kwincompositing/qml/Effect.qml | ||
---|---|---|
36 | It's trying to look identical to KPluginSelector |
kcmkwin/kwincompositing/qml/Effect.qml | ||
---|---|---|
36 | The same logic applies to KPluginSelector: you don't need to and shouldn't make items selectable when there are not actions that can be applied to selected items. No toolbar buttons, can't right-click on them, etc. |
kcmkwin/kwincompositing/qml/Effect.qml | ||
---|---|---|
36 | A current index is often needed for keyboard nav. But keyboard nav seems especially broken here, anyway. I'm not particularly convinced, but I'm not going to object in this specific case either. | |
133 | Removing this leaves dead code at the top of Video.qml that can also be removed. I do like that you're setting the loader to inactive to unload the component, rather than just stopping the video. That's smart. |