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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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,

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

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

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

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,

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

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

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

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:

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

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

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

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

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

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

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

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,

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

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

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

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

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

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,

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

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

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

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,

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

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

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

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

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

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

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

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

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

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:

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

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 /

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

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

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

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

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

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,

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

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

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

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:

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

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