Differential D17416 Diff 47136 kcmkwin/kwincompositing/qml/Effect.qml

Changeset View

Standalone View

View Options

kcmkwin/kwincompositing/qml/Effect.qml

Context not available.
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 { ​
davidedmundsonUnsubmitted
Not Done

I assume this change is

Remove effect list item selection

Why are we removing it?

davidedmundson: I assume this change is >Remove effect list item selection Why are we removing it?
ngrahamUnsubmitted
Not Done

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…
davidedmundsonUnsubmitted
Not Done

It's trying to look identical to KPluginSelector

davidedmundson: It's trying to look identical to KPluginSelector
ngrahamUnsubmitted
Not Done

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…
davidedmundsonUnsubmitted
Not Done

A current index is often needed for keyboard nav.
If something is in a list, up/down/page up/down keys work way better than tabbing through.

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
Context not available.
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;
davidedmundsonUnsubmitted
Done

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: {
Context not available.