D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R120:e680ff581a13: [applets/mediacontroller] Visually refresh 
media controller plasmoid (authored by cblack).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=79962=79963

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 79962.
cblack added 1 blocking reviewer(s): broulik.
cblack added a comment.
This revision now requires review to proceed.


  Trim whitespace

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=79961=79962

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 79961.
cblack added a comment.


  Address feedback

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=79960=79961

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 79960.
cblack added a comment.


  Smaller fallback icon

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=79944=79960

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 79944.
cblack added a comment.


  Wrap progressbar to prevent height filling

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=79886=79944

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 79886.
cblack added a comment.


  Fallback to album cover icon without album art or desktop file name

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77796=79886

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added a comment.


  @broulik ping

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  In D27160#628867 , @ngraham wrote:
  
  > LGTM now. @broulik, are you good with this?
  >
  > There are some funky margins that I think are the fault of the pop-up 
itself, due to the recent header patch. Will investigate that.
  
  
  Should be fixed with D28089 .

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a reviewer: broulik.
ngraham added a comment.


  LGTM now. @broulik, are you good with this?
  
  There are some funky margins that I think are the fault of the pop-up itself, 
due to the recent header patch. Will investigate that.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham, broulik
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77796.
cblack added a comment.


  FillWidth the combobox

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77794=77796

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Erm
  
  F8180076: Screenshot_20200316_193743.png 


REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77794.
cblack added a comment.


  Unflatten combobox

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77781=77794

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  I'm not sure I agree, but this flat version doesn't look like a combobox at 
all. TBH I don't like it.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added a comment.


  In D27160#628825 , @ngraham wrote:
  
  > Hmm, something's kinda weird now. The combobox is flat so it doesn't look 
like a combobox, which IMO is a regression. Also its alignment feels a bit odd 
and the pop-up uses a nonstandard highlight style with an incorrect text color:
  >
  > F8179996: Screenshot_20200316_182535.png 

  >
  > What was wrong with the original combobox?
  
  
  A raised combobox has too much visual prominence compared to its importance 
here.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Hmm, something's kinda weird now. The combobox is flat so it doesn't look 
like a combobox, which IMO is a regression. Also its alignment feels a bit odd 
and the pop-up uses a nonstandard highlight style with an incorrect text color:
  
  F8179996: Screenshot_20200316_182535.png 

  
  What was wrong with the original combobox?

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77781.
cblack added a comment.


  Always show combobox

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77763=77781

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  That's a deal-breaker then. I quite commonly have the following use case:
  
  1. Play music in Elisa
  2. Pause the music using my keyboard's play/pause button
  3. Watch some YouTube video
  4. the YouTube video ends
  5. I want to play the music in Elisa again
  6. I hit the play.pause button on my keyboard and nothing happens, because 
it's still targeting the YouTube video
  
  SInce this is a visual refresh, let's avoid adding or removing features. If 
you think that needs adjustment, let's discuss it and do it in a separate patch.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added a comment.


  In D27160#628688 , @ngraham wrote:
  
  > Detecting when more than one stream is playing seems broken too. If I play 
two at once, the UI never shows a combobox to let you select which one to 
control.
  
  
  The combobox can be configured to show manually.
  
  The usecase where a user is going to be playing two pieces of media at once 
isn't really common, so it's hidden by default to reduce unneeded visual 
elements.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Detecting when more than one stream is playing seems broken too. If I play 
two at once, the UI never shows a combobox to let you select which one to 
control.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Nice, it's gone for me now.
  
  However the margins are a bit funky with the new PlasmoidHeader design for 
the System Tray popups: F8179677: Screenshot_20200316_141954.png 


REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77763.
cblack added a comment.


  Replace PE.Heading with Kirigami.Heading

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77684=77763

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/config/config.qml
  applets/mediacontroller/contents/config/main.xml
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/configView.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Darn. Guess that's gotta get fixed first then. :/

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added a comment.


  In D27160#628387 , @ngraham wrote:
  
  > I'm still seeing the binding loop. :(
  
  
  Looks like this is a bug with Plasma's Label component, and not this patch. 
The Label component the height of the component based off of the height of the 
text, which varies based off of the height of the component, creating a binding 
loop.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  I'm still seeing the binding loop. :(

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77684.
cblack marked 7 inline comments as done.
cblack added a comment.


  Edit inaccurate comment.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=77683=77684

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/config/config.qml
  applets/mediacontroller/contents/config/main.xml
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/configView.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack updated this revision to Diff 77683.
cblack added a comment.


  Don't refer to parents' width

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27160?vs=75144=77683

BRANCH
  media-plasmoid-relayout (branched from master)

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

AFFECTED FILES
  applets/mediacontroller/contents/config/config.qml
  applets/mediacontroller/contents/config/main.xml
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/configView.qml
  applets/mediacontroller/contents/ui/main.qml

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> cblack wrote in ExpandedRepresentation.qml:261
> This advice doesn't seem to be applicable here, as the `implicitWidth` of the 
> ColumnLayout child isn't able to be mutated due to the fact that all of its 
> child items (text) have fixed `implicitWidth`s.

True, I didn't want to copy whole answer from Stack Overflow :) In this case 
`Layout.preferredWidth` should not be set to `parent.width / 2`, better to use 
`Layout.preferredWidth = 50`. I sometimes add % in a comment: 
`Layout.preferredWidth = 50//%`.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> kmaterka wrote in ExpandedRepresentation.qml:261
> I have not seen your code yet, I will check tomorrow. Probably because you 
> have something like this:
> 
>   Layout.preferredWidth: parent.width / 2
>   Layout.preferredHeight: parent.height / 2
> 
> Check the last answer from: How to design a multi-level fluid layout in QML 
> .
>  In short words: when possible, do not use Layout.preferredWidth, prefer 
> implicitWidth instead. If you want to have 50-50 ratio between items, set 
> implicitWidth in both items to the same value.

This advice doesn't seem to be applicable here, as the `implicitWidth` of the 
ColumnLayout child isn't able to be mutated due to the fact that all of its 
child items (text) have fixed `implicitWidth`s.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kmaterka wrote in ExpandedRepresentation.qml:261
> My last comment was not precise. Just check this 
>  for all details :)

As a general rule, you shouldn't refer to parents width in Layouts, including: 
`Layout.maximumWidth: parent.width`. It a (very) common mistake :)

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kmaterka wrote in ExpandedRepresentation.qml:261
> I have not seen your code yet, I will check tomorrow. Probably because you 
> have something like this:
> 
>   Layout.preferredWidth: parent.width / 2
>   Layout.preferredHeight: parent.height / 2
> 
> Check the last answer from: How to design a multi-level fluid layout in QML 
> .
>  In short words: when possible, do not use Layout.preferredWidth, prefer 
> implicitWidth instead. If you want to have 50-50 ratio between items, set 
> implicitWidth in both items to the same value.

My last comment was not precise. Just check this 
 for all details :)

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> cblack wrote in ExpandedRepresentation.qml:261
> I can't seem to reproduce a binding loop here.

I have not seen your code yet, I will check tomorrow. Probably because you have 
something like this:

  Layout.preferredWidth: parent.width / 2
  Layout.preferredHeight: parent.height / 2

Check the last answer from: How to design a multi-level fluid layout in QML 
.
 In short words: when possible, do not use Layout.preferredWidth, prefer 
implicitWidth instead. If you want to have 50-50 ratio between items, set 
implicitWidth in both items to the same value.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: kmaterka, iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, 
manueljlin, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> ngraham wrote in ExpandedRepresentation.qml:261
> there's a massive binding loop somewhere here:
> 
>   
> file:///home/nate/kde/usr/share/plasma/plasmoids/org.kde.plasma.mediacontroller/contents/ui/ExpandedRepresentation.qml:261:21:
>  QML Heading: Binding loop detected for property "height"

I can't seem to reproduce a binding loop here.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, manueljlin, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Ping. Would be nice to get this in.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, manueljlin, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> cblack wrote in ExpandedRepresentation.qml:145-148
> https://github.com/qt/qtdeclarative/blob/9893e71cea5de10193376c1733db627ef0783614/src/quick/items/qquickimage.cpp#L600-L603

That says the opposite of what you want.

We want image's texture provider to be used directly. Only then will this 
sourcesize matter.

This is saying when layer effect is enabled it uses the base blitting path.

We don't want to use layer here, we want the image as a source to the blur 
directly

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, manueljlin, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ExpandedRepresentation.qml:261
> +}
> +PlasmaExtras.Heading { // Song Album
> +color: (softwareRendering || !albumArt.visible) ? 
> PlasmaCore.ColorScope.textColor : "white"

there's a massive binding loop somewhere here:

  
file:///home/nate/kde/usr/share/plasma/plasmoids/org.kde.plasma.mediacontroller/contents/ui/ExpandedRepresentation.qml:261:21:
 QML Heading: Binding loop detected for property "height"

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin, ngraham
Cc: iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, manueljlin, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27160: [applets/mediacontroller] Visually refresh media controller plasmoid

[Nathaniel Graham]

ngraham added a comment.


  Overall looks much better I think! However the minimum height seems to be a 
bit tall now: F8098822: Screenshot_20200211_203232.png 

  
  Also I have a hard time putting my finger on why, but somehow the scrubber 
not filling the full width feels odd to me, especially when the applet is wider 
than it is tall: F8098826: Screenshot_20200211_203341.PNG 

  
  When the scrubber is the same width as the player controls below it, it seems 
fine. But when the window is wider and they no longer align vertically, it 
doesn't feel quite right to me.

REPOSITORY
  R120 Plasma Workspace

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

To: cblack, #vdg, #plasma, manueljlin
Cc: iasensio, ndavis, broulik, gvgeo, davidedmundson, ngraham, manueljlin, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart