Changeset View
Standalone View
kcmkwin/kwincompositing/qml/Effect.qml
| Show All 23 Lines | |||||
| 24 | import QtQuick.Controls 2.0 as QQC2 | 24 | import QtQuick.Controls 2.0 as QQC2 | ||
| 25 | import QtQuick.Layouts 1.0 | 25 | import QtQuick.Layouts 1.0 | ||
| 26 | import org.kde.kwin.kwincompositing 1.0 | 26 | import org.kde.kwin.kwincompositing 1.0 | ||
| 27 | 27 | | |||
| 28 | Rectangle { | 28 | Rectangle { | ||
| 29 | id: item | 29 | id: item | ||
| 30 | width: parent.width | 30 | width: parent.width | ||
| 31 | height: rowEffect.implicitHeight | 31 | height: rowEffect.implicitHeight | ||
| 32 | color: item.ListView.isCurrentItem ? effectView.backgroundActiveColor : index % 2 ? effectView.backgroundNormalColor : effectView.backgroundAlternateColor | 32 | color: index % 2 ? effectView.backgroundNormalColor : effectView.backgroundAlternateColor | ||
| 33 | signal changed() | 33 | signal changed() | ||
| 34 | property int checkedState: model.EffectStatusRole | 34 | property int checkedState: model.EffectStatusRole | ||
| 35 | 35 | | |||
| 36 | MouseArea { | | |||
davidedmundson: I assume this change is
>Remove effect list item selection
Why are we removing it? | |||||
Because UI-wise, there is no point in making something selectable when there is nothing you can do with selected items. ngraham: Because UI-wise, there is no point in making something selectable when there is nothing you can… | |||||
davidedmundson: It's trying to look identical to KPluginSelector | |||||
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. ngraham: The same logic applies to KPluginSelector: you don't need to and shouldn't make items… | |||||
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. davidedmundson: A current index is often needed for keyboard nav.
If something is in a list, up/down/page… | |||||
| 37 | anchors.fill: parent | | |||
| 38 | onClicked: { | | |||
| 39 | effectView.currentIndex = index; | | |||
| 40 | } | | |||
| 41 | } | | |||
| 42 | | ||||
| 43 | RowLayout { | 36 | RowLayout { | ||
| 44 | id: rowEffect | 37 | id: rowEffect | ||
| 45 | property int maximumWidth: parent.width - 2 * spacing | 38 | property int maximumWidth: parent.width - 2 * spacing | ||
| 46 | width: maximumWidth | 39 | width: maximumWidth | ||
| 47 | Layout.maximumWidth: maximumWidth | 40 | Layout.maximumWidth: maximumWidth | ||
| 48 | x: spacing | 41 | x: spacing | ||
| 49 | 42 | | |||
| 50 | RowLayout { | 43 | RowLayout { | ||
| ▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Line(s) | 110 | QQC2.Label { | |||
| 120 | font.weight: Font.Bold | 113 | font.weight: Font.Bold | ||
| 121 | visible: false | 114 | visible: false | ||
| 122 | wrapMode: Text.Wrap | 115 | wrapMode: Text.Wrap | ||
| 123 | Layout.maximumWidth: parent.maximumWidth | 116 | Layout.maximumWidth: parent.maximumWidth | ||
| 124 | } | 117 | } | ||
| 125 | Loader { | 118 | Loader { | ||
| 126 | id: videoItem | 119 | id: videoItem | ||
| 127 | active: false | 120 | active: false | ||
| 121 | visible: false | ||||
| 128 | source: "Video.qml" | 122 | source: "Video.qml" | ||
| 129 | function showHide() { | 123 | function showHide() { | ||
| 130 | if (!videoItem.active) { | 124 | if (!videoItem.active) { | ||
| 131 | videoItem.active = true; | 125 | videoItem.active = true; | ||
| 126 | videoItem.visible = true; | ||||
| 132 | } else { | 127 | } else { | ||
| 133 | videoItem.item.showHide(); | 128 | videoItem.active = false; | ||
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. davidedmundson: Removing this leaves dead code at the top of Video.qml that can also be removed.
---
I do… | |||||
| 129 | videoItem.visible = false; | ||||
| 134 | } | 130 | } | ||
| 135 | } | 131 | } | ||
| 136 | onLoaded: { | 132 | onLoaded: { | ||
| 137 | videoItem.item.showHide(); | 133 | videoItem.item.showHide(); | ||
| 138 | } | 134 | } | ||
| 139 | } | 135 | } | ||
| 140 | } | 136 | } | ||
| 141 | Item { | 137 | Item { | ||
| Show All 29 Lines | |||||
I assume this change is
Why are we removing it?