[kcmkwin/compositing] Remove effect list item selection, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator
ClosedPublic

Authored by GB_2 on Dec 7 2018, 8:13 PM.

Details

Summary

Makes some improvements to the Effects KCM (details in title).


Test Plan

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.
GB_2 created this revision.Dec 7 2018, 8:13 PM
Restricted Application added a subscriber: kwin. · View Herald TranscriptDec 7 2018, 8:13 PM
GB_2 requested review of this revision.Dec 7 2018, 8:13 PM
ngraham added a subscriber: ngraham.Dec 8 2018, 4:19 AM

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.

GB_2 retitled this revision from [Effects KCM] Use a better selection color, fix list item size after hiding the effect video and use a real button the play button to [Effects KCM] Use a better selection color, fix list item size after hiding the effect video and use a real button as the play button.Dec 8 2018, 12:39 PM
GB_2 updated this revision to Diff 47117.Dec 8 2018, 3:00 PM
GB_2 edited the summary of this revision. (Show Details)

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.

GB_2 added a comment.Dec 8 2018, 3:49 PM

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?

GB_2 updated this revision to Diff 47136.Dec 8 2018, 5:33 PM
GB_2 retitled this revision from [Effects KCM] Use a better selection color, fix list item size after hiding the effect video and use a real button as the play button to [Effects KCM] Use a better selection color, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator.
GB_2 edited the summary of this revision. (Show Details)

Use the right busy indicator.

GB_2 retitled this revision from [Effects KCM] Use a better selection color, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator to [Effects KCM] Remove effect list item selection, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator.Dec 8 2018, 5:40 PM
ngraham accepted this revision.Dec 9 2018, 5:47 PM

Ah, much better! This looks great to me now. KWin folks, any any concerns, or should we land this?

This revision is now accepted and ready to land.Dec 9 2018, 5:47 PM
davidedmundson added inline comments.
kcmkwin/kwincompositing/qml/Effect.qml
36

I assume this change is

Remove effect list item selection

Why are we removing it?

GB_2 added a comment.Dec 9 2018, 5:55 PM

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.

ngraham added inline comments.Dec 9 2018, 5:55 PM
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.

davidedmundson added inline comments.Dec 9 2018, 6:11 PM
kcmkwin/kwincompositing/qml/Effect.qml
36

It's trying to look identical to KPluginSelector

ngraham added inline comments.Dec 9 2018, 6:26 PM
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.

zzag retitled this revision from [Effects KCM] Remove effect list item selection, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator to [kcmkwin/compositing] Remove effect list item selection, fix list item size after hiding the effect video, use a real button as the play button and use the right busy indicator.Dec 9 2018, 8:01 PM
davidedmundson added inline comments.Dec 10 2018, 9:34 AM
kcmkwin/kwincompositing/qml/Effect.qml
36

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.

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.

GB_2 updated this revision to Diff 47311.Dec 10 2018, 6:37 PM

Remove unused code.

GB_2 marked an inline comment as done.Dec 10 2018, 6:37 PM
davidedmundson accepted this revision.Dec 11 2018, 10:13 AM
This revision was automatically updated to reflect the committed changes.