D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  Change to implicitHeight in D27117 .
  
  That line should be in D26574 , messed up 
splitting the patches.
  It keeps the height of the row fixed, so that it won't change when the button 
gets hidden.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> ListItemBase.qml:111
>  RowLayout {
> -Layout.fillWidth: true
> +Layout.minimumHeight: contextMenuButton.height
>  

That's asking for a binding loop.

An implicit size should never be based on a current size.

This could be:
Layout.minimumHeight: contextMenuButton.implicitHeight

Though I'm surprised you need this line at all.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  In D26271#604203 , @ngraham wrote:
  
  > @gvgeo though I tested this before and found it to be working, after 
landing the patch I'm now seeing that using the new radio buttons to change the 
default device does not automatically move all running streams to the new 
device, as it used to. Can you check this? Thanks!
  
  
  This should not happen. The changes to the `defaultButton` were minimum, 
except the transformation to RadioButton, onClicked stayed the same. I suspect 
something else is to blame, but **cannot replicate** in two systems, and cannot 
see something in the code.
  
  Can you check the console for any messages?
  Next step is to confirm that was working before these patches. And try to 
pinpoint the responsible patch, by applying them 1 by 1 and testing.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  @gvgeo though I tested this before and found it to be working, after landing 
the patch I'm now seeing that using the new radio buttons to change the default 
device does not automatically move all running streams to the new device, as it 
used to. Can you check this? Thanks!

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R115:8ada15ad8e5b: [Applet]Update layout based on T10470 
(authored by gvgeo, committed by ngraham).

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=74145&id=74796

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  I applied the patches in this series and tested and went through the code 
again and everything looks good to me. Shall we start landing them for 5.19? We 
can always tweak things later; there's loads of time before the next release.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout2 (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[Manuel Jesús de la Fuente]

manueljlin accepted this revision.
manueljlin added a comment.


  Nice, looks great! About the stream name, it doesn't look like it's useful 
most of the time so it might be better to remove it

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout2 (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 74145.
gvgeo added a comment.


  Correct update. Last was wrong one.

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=74141&id=74145

BRANCH
  layout2 (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 74141.
gvgeo added a comment.


  Update for latest scrollbar fixes (remove workaround).

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=73919&id=74141

BRANCH
  menu3 (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 73919.
gvgeo added a comment.


  rebase

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=73238&id=73919

BRANCH
  layout2 (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Christoph Feck]

cfeck added a comment.


  The radio stream I use (via mpv) shows the interpret and title information 
for all titles it plays. Also, when mpv plays a local file, ttitle information 
about the file is added there. I don't use any other media streams, so I cannot 
comment on the general availability of this additional stream information.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[Noah Davis]

ndavis added a subscriber: cfeck.
ndavis added a comment.


  Since it's @cfeck's bug report, I'll add him to the subscribers.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[Noah Davis]

ndavis added a comment.


  In D26271#593013 , @gvgeo wrote:
  
  > Found this request to display the name of the stream 
https://bugs.kde.org/show_bug.cgi?id=409453 .
  >  Which was done, but remove here. Maybe should leave it too?
  >  With a quick check, not many applications have this information.
  >  F7880660: Screenshot_20200113_135516.png 

  
  
  I'm skeptical of the need for this feature. It seems like something very few 
people will need. Out of all of those stream names, only one of them has 
information that could be considered possibly useful to most people. If there's 
a good way to not show the information except when it contains something 
useful, then go for it. Otherwise, I'd say leave it out.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  Found this request to display the name of the stream 
https://bugs.kde.org/show_bug.cgi?id=409453 .
  Which was done, but remove here. Maybe should leave it too?
  With a quick check, not many applications have this information.
  F7880660: Screenshot_20200113_135516.png 


REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  Can't include an untested patch because we are in a hurry.
  
  To clarify my comment's D26574#591702 
 part "If these patches ever get 
accepted".
  I wasn't aware of the release schedule, and my doubts are about code 
acceptance.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  I'm good with this now as an initial implementation of the mockup. However 
we're a bit close to the Plasma 5.18 tagging date (January 16th). My gut 
instinct is to land these early in the 5.19 cycle rather than rushing to get 
everything into 5.18 and potentially shipping with un-noticed regressions.
  
  On the other hand, the UI to switch the default device is much nicer with 
this patch series, and we've received a lot of user complaints about the big 
buttons I added to implement that in 5.17 (sorry).
  
  Thoughts?

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  layout (branched from master)

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 73238.
gvgeo added a comment.


  Separate bug fixes.
  Change defaultButton visibility
  Change defaultButton's RowLayout height

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72914&id=73238

BRANCH
  layout (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  Yeah, feel free to fix those bugs separately so we can get them in more 
quickly.
  
  Personally I don't think there's a major use case for manipulating the 
settings of a port when it's not actually being used.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  It makes possible to change volume/mute before plug something. But have no 
idea, if there is any real reason.
  
  ---
  
  About these 2 bugs https://bugs.kde.org/show_bug.cgi?id=393418, 
https://bugs.kde.org/show_bug.cgi?id=413448 .
  Should I separate the fixes from this patch? It is only 8 lines, and afraid 
will get more reports if this patch gets delayed.
  
Layout.minimumHeight: units.gridUnit * 8
Layout.minimumWidth: units.gridUnit * 14
Layout.preferredHeight: units.gridUnit * 21
Layout.preferredWidth: units.gridUnit * 24
Plasmoid.switchHeight: Layout.minimumHeight
Plasmoid.switchWidth: Layout.minimumWidth
.
.
.
   Layout.preferredHeight: main.Layout.preferredHeight
   Layout.preferredWidth: main.Layout.preferredWidth

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  > But this would made impossible to select an unavailable port from kcm too.
  
  Is there an actual use case for this? If not, option 2 might make the most 
sense.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo planned changes to this revision.
gvgeo added a comment.


  Found a bug. There is no update when a port that is not in use gets 
unavailable.
  
  Why is a problem in this patch:
  When using port 1, if port 2 becomes unavailable, is possible to switch to  
port that is unavailable. 
  The real problem is you switch to port 2 then. Info gets updated, the UI see 
only one available port, thus removing the options. Locking you to the 
unavailable port. Only solution is to use KCM.
  
  Don't know how to fix it.  As workaround could:
  
  1. Make menu options always available.
  2. Or confirm port is available after the update,  and switch back if need. 
But this would made impossible to select an unavailable port from kcm too.
  
  Bug affects also fb5631928121 


REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72914.
gvgeo added a comment.


  Add description check

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72854&id=72914

BRANCH
  layout (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  In D26271#588909 , @ngraham wrote:
  
  > Lovely. Any chance we could make the bottom divider touch the edges of the 
window?
  
  
  Not really, not from here. This space is part of system tray. If you use the 
plasma-pa widget only, will see that it touches exactly the edge.
  Bigger negative value  does not show in the system tray, but would make the 
line appear outside the widget if used on desktop.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  Lovely. Any chance we could make the bottom divider touch the edges of the 
window?
  
  I suppose we'll need to adjust the Breeze Plasma theme for the changes to the 
tab bar appearance.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added inline comments.

INLINE COMMENTS

> DeviceListItem.qml:36
> +var name  = model.data(model.index(i, 0), 
> model.role("Ports"))
> +[model.data(model.index(i, 0), 
> model.role("ActivePortIndex"))].description;
> +var length = Math.min(itemLength, name.length)

if (description)

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Noah Davis]

ndavis added a comment.


  In D26271#588570 , @gvgeo wrote:
  
  > It is a theme problem. Breeze light also has problem.
  >  Widgets take color from Plasma Style. While in applications are fine. 
  >  I checked with this widget   to compare.
  
  
  Interesting. I'll see if I can fix that.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Manuel Jesús de la Fuente]

manueljlin added a comment.


  If it isn't possible that's okay (it looks really nice right now, great job 
btw :D), although it'd be nice to make it consistent with the kirigami style 
divider

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  It is a theme problem. Breeze light also has problem.
  Widgets take color from Plasma Style. While in applications are fine. 
  I checked with this widget   to compare.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Noah Davis]

ndavis added a comment.


  Why do the unchecked radiobuttons have a dark outline in your screenshot?

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo marked 5 inline comments as done.
gvgeo added a comment.




REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72854.
gvgeo added a comment.


  leanup
  Remove separation lines
  Fix 2 bugs
  Full Name display when a port name sub-strings are same.
  Requested changes
  
  Issues:
  
  1. Check comment at the bottom of main.qml in D26256 

  
  These two strange margins (including defaultButton in this patch), are 
required to vertical align the icons in their center, and the texts with the 
slider.
  
  2. Setting preferredHeight in fullRepresentation makes widget on desktop to 
load with size >= preferredHeight on boot.

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72750&id=72854

BRANCH
  layout (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

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


  Very nice work! I continue to be impressed with your contributions.
  
  > What else to do, without displaying the full device names?
  
  Display full device names when there's more than one playback or input 
device, maybe. Or only when there are more than 2 and their names are identical.

INLINE COMMENTS

> ListItemBase.qml:116
> +id: defaultButton
> +Layout.leftMargin: units.smallSpacing * 0.75
> +spacing: units.smallSpacing * 1.5

Don't multiply by 0.75; this could result in a fractional value and the spacing 
doesn't doesn't need to be literally pixel-identical with the mockup

> ListItemBase.qml:117
> +Layout.leftMargin: units.smallSpacing * 0.75
> +spacing: units.smallSpacing * 1.5
> +checked: PulseObject.default

Don't override this here. If it looks bad, let's fix it in the RadioButton 
control itself

> ListItemBase.qml:127
> +visible: !defaultButton.visible
> +wrapMode: Text.NoWrap
> +elide: Text.ElideRight

This is the default wrap value; no need to explicitly set it

> ListItemBase.qml:317
> +// Ports
> +// By choice only shown when there are at least two available 
> ports.
> +if (PulseObject.ports && PulseObject.ports.length > 1) {

By choice -> Intentionally

> main.qml:317
>  Plasmoid.fullRepresentation: ColumnLayout {
> -spacing: units.smallSpacing
> +spacing: units.smallSpacing * 0.75
>  Layout.minimumHeight: main.Layout.minimumHeight

Revert this change

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72750.
gvgeo added a comment.


  Add visibility to the line that splits playback and record.
  On the application side, will show if there is 1 record and 1 playback
  application. Because there is no port name to easily differentiate them.

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72739&id=72750

BRANCH
  layout (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo marked 7 inline comments as done.
gvgeo added a comment.


  About the devices labels, I feel it is fine as it is. It is not a problem, to 
have two devices with the same port name.
  Only if could put a tooltip. But no, Radiobutton does not deserve a tooltip.
  What else to do, without displaying the full device names?
  
  I took a look at breeze, for the lines between list items and coloring and 
tab changes. I was lost and didn't know, what I was looking for exactly.
  I will keep an eye, but... I feel this will be part of a more general breeze 
evolution.
  By the time I will have decode how theme system works, it is possible that 
you will have already made the changes.

INLINE COMMENTS

> main.qml:407
>  
> -Header {
> -Layout.fillWidth: true
> -visible: sourceOutputView.count > 0 && 
> !streamsView.simpleMode
> -text: i18n("Recording Streams")
> +PlasmaCore.SvgItem {
> +elementId: "horizontal-line"

This is the line that separates the playback and recording applications.
Just notice that should not always be visible. Will fix.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72739.
gvgeo added a comment.


  Remove port combobox and put port selection back the hamburger menu.
  
Now port selection appears only if there is at least 2 available ports.
  
  Made devices/applications labels clickable.
  Removed RTL margin problems.
  Made playback/recording separation line smaller.(according to mockup)
  
  The lines added with svg, are not the same with mockup or current lines 
between playback devices.
  There are 2 pixels, and have lower opacity (with 1 pixel spacing?).

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72373&id=72739

BRANCH
  layout (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Filip Fila]

filipf added inline comments.

INLINE COMMENTS

> ListItemBase.qml:117
> +id: defaultButton
> +Layout.leftMargin: units.smallSpacing * 0.75
> +checked: PulseObject.default

Margins should but don't get mirrored when using a RTL layout. I haven't tested 
anything, but I think you need to add a rightMargin when layout mirroring is 
enabled.

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  Without device icons, the text needs to be improved a bit, or else I see this 
when I have multiple devices: F7851224: Screenshot_20191230_145819.png 


INLINE COMMENTS

> ListItemBase.qml:115
>  
> +PlasmaComponents3.RadioButton {
> +id: defaultButton

Give the radio button a `text:` property and remove the heading item; that way 
the text will be clickable

> main.qml:406
>  
> -Header {
> +PlasmaCore.SvgItem {
> +elementId: "horizontal-line"

I think there isn't a line here in the mockup, right?

> main.qml:471
>  
> -Header {
> -id: sourceViewHeader
> +PlasmaCore.SvgItem {
> +elementId: "horizontal-line"

ditto

> main.qml:525
> +
> +PlasmaCore.SvgItem {
> +elementId: "horizontal-line"

you can give it negative side margins to get it to touch the sides of the 
pop-up, as in the mockup

> main.qml:539
> +Layout.leftMargin: units.smallSpacing * 0.75 + 4
> +onClicked: 
> console.log(raiseMaximumVolumeCheckbox.Layout.rightMargin)
> +}

So this doesn't actually do anything yet? Obviously that needs to change before 
this can land. :) Also the checked state needs to be saved somewhere.

> main.qml:542
> +
> +PlasmaExtras.Heading {
> +Layout.fillWidth: true

Don't use a separate Heading here; give the Checkbox a `text:` property

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72373.
gvgeo added a comment.


  Brought back spacing, and corrected sizes.
  Centered icons, and align text.
  Reduced checkbox to 18 pixel.
  
  QQC2 checkbox is 16 pixels, but does not scale. Instead used 
PlasmaComponents3 which is 18.

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72345&id=72373

BRANCH
  UI (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Nathaniel Graham]

ngraham added a comment.


  Awesome work, @gvego. I'm really liking all of these patches of yours. :)
  
  The checkbox size issue is probably from the styling of the PlasmaComponents 
checkbox, as opposed to the basic QQC2 Checkbox. If I'm right, we'd need to fix 
the PC checkbox, or else just use the QQC2 one instead.
  
  It might also be worth figuring out how to implement the different 
window/view background colors as depicted in the mockup, because it's a common 
pattern on those mockups. Might require tweaking the scrollable list appearance 
in the Plasma style, maybe?

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  I was not able to change the checkbox size, nothing I tried worked.
  Checked other places with smaller checkbox(like notifications). But could not 
found how the size is controlled.
  
  Current view:
  F7849371: Screenshot_20191229_220847.png 

  
  F7849370: Screenshot_20191229_220809.png 


REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo updated this revision to Diff 72345.
gvgeo added a comment.


  Fix spacing
  Change plasmoid tooltip to mirror the port name
  Cleanup main.qml

REPOSITORY
  R115 Plasma Audio Volume Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26271?vs=72321&id=72345

BRANCH
  UI (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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

D26271: [Applet]Update layout based on T10470

[Manuel Jesús de la Fuente]

manueljlin added a comment.


  Looking at the screenshots, UI wise it's looking good already, only that the 
checkbox seems too big (should be the same size as the radio button, 16px) and 
there's a 5px sized padding after the divider that cuts off part of the 
Internal Microphone button and volume slider earlier than it's supposed to  
F7849101: image.png 

REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo added a comment.


  Looks weird right now.
  From what I can tell, themes add lines between list items.
  There must be some easy way to disable it.
  F7848489: Screenshot_20191229_114632.png 


REPOSITORY
  R115 Plasma Audio Volume Applet

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

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

D26271: [Applet]Update layout based on T10470

[George Vogiatzis]

gvgeo created this revision.
gvgeo added reviewers: VDG, Plasma, manueljlin, drosca.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
gvgeo requested review of this revision.

REVISION SUMMARY
  Change layout according to T10470  
mockups.
  
  Not included in this patch:
  100% line mark
  Size changes(Margins, fonts)
  Raise maximum volume D26256 
  Mute all icon

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  UI (branched from master)

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

AFFECTED FILES
  applet/contents/ui/DeviceListItem.qml
  applet/contents/ui/ListItemBase.qml
  applet/contents/ui/StreamListItem.qml
  applet/contents/ui/main.qml

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