D17416: [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

davidedmundson added inline comments.

INLINE COMMENTS

> ngraham wrote in 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.

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.

> Effect.qml:133
>                      } else {
> -                        videoItem.item.showHide();
>                      }

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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D17416

To: GB_2, #kwin, #vdg, ngraham
Cc: davidedmundson, ngraham, #vdg, kwin, #kwin, squeakypancakes, alexde, IohannesPetros, mkulinski, trickyricky26, ragreen, jackyalcine, Pitel, iodelay, crozbo, ndavis, bwowk, ZrenBot, firef, skadinna, lesliezhai, ali-mohamed, hardening, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, mbohlender, mart