D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove updated this revision to Diff 79740.
niccolove added a comment.


  Removed merged look, no longer needed, but kept rounded corners change

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28372?vs=78742=79740

BRANCH
  merged-plasmoidheading (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

To: niccolove, #plasma
Cc: manueljlin, ahiemstra, ndavis, ngraham, mart, davidedmundson, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Nathaniel Graham]

ngraham added a comment.


  The second image definitely looks better than the first. However I wonder if 
we could even combine them into just one row. For toolbars with nothing but 
icons-only buttons, they kind of feel like they're screaming out to be 
integrated into the header.
  
  For toolbars with controls that have labels, yeah, those should probably be 
on a second row.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: manueljlin, ahiemstra, ndavis, ngraham, mart, davidedmundson, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove added a subscriber: manueljlin.
niccolove added a comment.


  Generally speaking, I'd still like for all applets to have a row of 
plasmoidheading on the bottom of system tray's own because having just system 
tray plasmoidheading, especially with the new small font, does not feel 
"enough" to me; if you look at every @manueljlin mockups, you'll see they have 
two rows of heading. I think it's because there's a visual minimum 
(plasmoidheading area / normal background area) ratio to look pretty. To 
clarify, I think this is not ideal:
  F8207914: image.png 
  While I think this looks much better:
  F8207917: image.png 
  YMMV. What do you think?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: manueljlin, ahiemstra, ndavis, ngraham, mart, davidedmundson, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Nathaniel Graham]

ngraham added a comment.


  Awesome, might be something to explore!
  
  I think this patch should probably go in too, since even if we have 
headerbar-style headers, there are still going to be cases where we'll want a 
second row to put more controls in. But yeah, for the common case of just a few 
toolbuttons, putting them in the same row as the title would be really nice 
IMO. Feel free to polish up and submit that proof-of-concept patch you were 
mentioning in the other thread!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ahiemstra, ndavis, ngraham, mart, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Arjen Hiemstra]

ahiemstra added subscribers: ndavis, ahiemstra.
ahiemstra added a comment.


  The same thing was actually proposed by I think @ndavis a while back, see 
https://phabricator.kde.org/T10470#219994 .
  
  To summarize, it's perfectly technically possible and I actually rather like 
it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ahiemstra, ndavis, ngraham, mart, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Nathaniel Graham]

ngraham added a comment.


  So I have a heretical thought here: maybe we should consider using a 
CSD-style headerbar for system tray plasmoids with toolbar-style controls and 
put the controls on the same row as the header.
  
  No wait, hear me out! This pop-up doesn't suffer from most of the 
disadvantages of CSD headerbars 
 since they aren't 
movable or resizable and we have total control over the presentation. And using 
this style would allow us to achieve visual consistency with Kirigami-style 
toolbars, which have the title on the left and a toolbar with controls on the 
right. We might need to increase the width of the pop-ups to accommodate long 
translations, and the space for toolbar content would be very limited so we'd 
need to use icons-only buttons for everything; no labels. The drawbacks might 
still outweigh the advantages. Anyway I just wanted to bring it up as an 
option. :) I'm not at all opposed to this idea here, I just wanted to present a 
possible alternative.

INLINE COMMENTS

> PlasmoidHeading.qml:58
> + * If false, the plasmoidheading will display a separator. If true, it 
> will be considered merged
> + * with another plasmoidheading and the separator won't be showed. False 
> by default.
> + */

Might be worth saying, "...merged with another plasmoidheading underneath this 
one..." for clarity

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ngraham, mart, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> niccolove wrote in PlasmoidHeading.qml:77
> I'd go with no SVG, to make sure that the theme creators will see that it's 
> broken and add a merged- look. Keep in mind that the plasmoidheading svg was 
> added barely one month ago, and no big Plasma release had it yet, so I find 
> it very hard to believe that any third party theme already has it.

agree with nicolove

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: mart, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove marked an inline comment as done.
niccolove added inline comments.

INLINE COMMENTS

> davidedmundson wrote in PlasmoidHeading.qml:77
> What should happen if this is set to merged and someone uses a theme that 
> doesn't have a merged- prefix?
> 
> The old SVG or no SVG at all?

I'd go with no SVG, to make sure that the theme creators will see that it's 
broken and add a merged- look. Keep in mind that the plasmoidheading svg was 
added barely one month ago, and no big Plasma release had it yet, so I find it 
very hard to believe that any third party theme already has it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove updated this revision to Diff 78742.
niccolove added a comment.


  Specified false by default

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28372?vs=78721=78742

BRANCH
  merged-plasmoidheading (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> PlasmoidHeading.qml:58
> + * If false, the plasmoidheading will display a separator. If true, it 
> will be considered merged
> + * with another plasmoidheading and the separator won't be showed.
> + */

Worth writing "the default value is false" in the docs, as it's what most 
people look for.

> PlasmoidHeading.qml:77
>  imagePath: "widgets/plasmoidheading"
> -prefix: location == PlasmoidHeading.Location.Header? 'header' : 
> 'footer'
> -enabledBorders: {
> -var borders = new Array()
> -borders |= PlasmaCore.FrameSvg.LeftBorder
> -borders |= PlasmaCore.FrameSvg.RightBorder
> -if (plasmoid.location !== PlasmaCore.Types.TopEdge || location 
> != PlasmoidHeading.Location.Header) {
> -borders |= PlasmaCore.FrameSvg.TopBorder
> -}
> -if (plasmoid.location !== PlasmaCore.Types.BottomEdge || 
> location != PlasmoidHeading.Location.Footer) {
> -borders |= PlasmaCore.FrameSvg.BottomBorder
> -}
> -return borders
> -}
> +prefix: (merged ? 'merged-' : '') + (location == 
> PlasmoidHeading.Location.Header? 'header' : 'footer')
>  }

What should happen if this is set to merged and someone uses a theme that 
doesn't have a merged- prefix?

The old SVG or no SVG at all?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove added a reviewer: Plasma.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove retitled this revision from "Added a merged look to the 
plasmoidheading" to "Added a merged look to the plasmoidheading and remove 
roundedborders".
niccolove edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading

[Niccolò Venerandi]

niccolove updated this revision to Diff 78721.
niccolove added a comment.


  Removed rounded borders and thus the enabledBorders check

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28372?vs=78719=78721

BRANCH
  merged-plasmoidheading (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

To: niccolove
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading

[Niccolò Venerandi]

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


  The rounding in the svgs is not necessary.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading

[Niccolò Venerandi]

niccolove created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
niccolove requested review of this revision.

REVISION SUMMARY
  Added a merged look (without a separator) so that multiple plasmoidheadings 
can be visually merged (e.g.: in system tray).

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  merged-plasmoidheading (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

To: niccolove
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns