D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#391560 , @ngraham wrote:
  
  > I believe you'll need to file a sysadmin ticket to get that changed.
  
  
  Will do! Thanks!

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  I believe you'll need to file a sysadmin ticket to get that changed.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#391371 , @davidedmundson 
wrote:
  
  > @rooty if you want to re-apply for a commit account, I'll approve it.
  
  
  Thank you you guys! One problem though - I used an alias (Pete Cho) on 
identity.kde.org, but i'd like to use my real name, is that possible?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:6a625cd61d29: [Notifications] Add padding to 
notifications (authored by Krešimir Čohar kco...@gmail.com, committed 
by ngraham).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49230=49252

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  @rooty if you want to re-apply for a commit account, I'll approve it.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  yep

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  @davidedmundson, does this look good to you?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

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


  Lovely, works perfectly now. :)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49230.
rooty added a comment.


  Use units.smallSpacing for padding consistently except when 
bodyText.lineCount > 1, spruce things up

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49209=49230

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  You were right! Thank you so much, it was so much easier when I started 
thinking about it as a math problem rather than a design problem. I'm going to 
upload another diff, I hope this one stands up to scrutiny.
  
  I apologize for the very long "implicitHeight" but it had to cover six 
different configurations:
  
  - no icon + heading only
  - no icon + heading + one line of body text
  - no icon + heading + many lines of body text
  - icon + heading only
  - icon + heading + one line of body text
  - icon + heading + many lines of body text.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  You have 4 possible states:
  
  Icon is taller and text is one line
  Icon is taller and text is multi-line
  Text is taller and text is one line
  Text is taller and text is multi-line
  
  Changing font sizes is moving you between them, but that's just a side effect.
  
  
  
  You seem to be trying different things and then testing them. From this ever 
lasting history that's not working.
  
  Making margins match isn't a visual problem, it's a maths problem.
  
  When I reviewed this last time, I didn't run it, I just did it all on paper. 
(genuinely, here's a photo 
https://home.davidedmundson.co.uk/index.php/s/W8ZtPMZNyBAXRZz)
  
  Take a step back, do the calculations of the window height based on what 
things should be, and then it'll just work.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49209.
rooty added a comment.


  Fix lower border cutoff for fonts > 10 pt

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49208=49209

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  Last diff is much better in general, but Noto Sans 11pt seems to cause 
problems
  
  F6539442: image.png 

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  
  
  >> How do we get the icon size to increase in proportion to increasing font 
size
  > 
  > See units.cpp in plasma-framework. It's complex. I don't think that's a 
relevant topic you need to go down
  
  Yeah, also I'm not sure it's the right approach - icons might turn out to be 
HUGE...
  
  In D17975#391025 , @filipf wrote:
  
  > Last diff is much better in general, but Noto Sans 11pt seems to cause 
problems
  >
  > F6539442: image.png 
  
  
  I have an idea how to fix that.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  > Is there a way to give the icon itself padding? I've been using anchors
  
  Yes, but if you're padding the icon by moving it then you need to set the 
window height to
  
  Math.max(sizeOfIcon, sizeOfText)
  
  needs to be
  
  Math.max(sizeOfIcon + 2*padding, sizeOfText)
  
  in order to keep even padding.
  
  
  
  > How do we get the icon size to increase in proportion to increasing font 
size
  
  See units.cpp in plasma-framework. It's complex. I don't think that's a 
relevant topic you need to go down

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49208.
rooty added a comment.


  Correct padding with just icon + heading + no text, make icon padding even 
for 10 pt sized fonts other than Noto Sans

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49132=49208

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#391008 , @filipf wrote:
  
  > ^ Yes, it's important to note that additional (and unwanted) padding 
underneath the icon can happen with master, while this patch can add the same 
above the icon. From my layman's understanding, there is nothing solid and 
absolute values are based on.
  
  
  There are effectively two problems here:
  
  - How do we get even padding around the **icon itself** without having to 
resort to manipulating the rest of the notification?
  - How do we get the icon size to increase in proportion to increasing font 
size (and/or scaling factor) and, once bigger/smaller, how do we get it to stay 
that very same size regardless of how many lines of text there are?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  ^ Yes, it's important to note that additional (and unwanted) padding 
underneath the icon can happen with master, while this patch can add the same 
above the icon. From my layman's understanding, there is nothing quite solid 
and absolute values are based on.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#390875 , @ngraham wrote:
  
  > With this patch: F6539252: With patch.png 

  
  
  TLDR: Yup, but that's only **the tip of the iceberg** (of the problems that 
plague //both master and this patch//)
  
  It's the problem I described two comments ago:
  
  > However, if another font is used, the amount of padding below the icon 
changes (Cabin - 5 px, Lato - 3 px etc.). It's because I've been using 
bottomPart or the implicitHeight to make the notification "taller" so as to 
give the icon padding.
  >  Is there a way to give the icon itself padding? I've been using anchors 
but there's nothing I can think of to anchor it to? If I anchor it to the 
notification text in any way, I can add a margin, sure, but it gets //stretched 
out beyond belief// if there's too much text.
  
  I've been using fonts to illustrate the problem but your screenshots //do a 
better job.//
  In addition, changing the height of the **icon** results in several binding 
loops (gross), or in an icon that's not a square (album art), or the situation 
in your second screenshot. Hardly ideal.
  
  I just don't know how to add bottom padding //to the notification icon 
itself// and have the height of the notification adapt accordingly.
  
  P.S. This did, however, shed some light on things that have been bothering me 
about the current master.
  
  > Status quo: F6539251: Without the patch.png 

  
  Should this happen? There's no description yet there's an empty space. The 
icon doesn't resize accordingly. This is what happens in master with fonts 
>10pt and scaling that's a non-integer that's not a multiple of 0.5 (1.1, 1.2, 
1.3 etc.), resulting in padding underneath the icon (not the notification 
text!) and none above. For example:
  F6539300: image.png 
  The plot thickens - another screenshot of **master** (not this patch)
  F6539302: image.png 
  I'm assuming this second one is because of the close button. This doesn't 
happen with this patch but I'm assuming I merely obscured the problem rather 
than solved it.
  
  I'm at a loss as to how to proceed. The padding problems result from:
  
  - in master, because the icon is defined as "units.iconSizes.largeSpacing" 
without any regard for the user's font (or scaling factor actually, except when 
it's 1.5, 2, 2.5, 3)
  - in this patch, because of the units.iconSizes.largeSpacing thing and my 
lack of proficiency in QML and inability to give the icon padding without 
manipulating either its height or that of the entire notification (doing the 
former results in **several binding loops** OR an icon that's **distorted in 
size** and isn't a square... doing the latter results in the problem depicted 
in your screenshots)

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

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


  If I use `notify-send` to create a simple notification, it's unfortunately a 
regression compared to what we have right now. Checkout the uneven top and 
bottom padding on the icon.
  
  Status quo: F6539251: Without the patch.png 

  
  With this patch: F6539252: With patch.png 


REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#390336 , @rooty wrote:
  
  > Is there a way to give the icon itself padding? I've been using anchors but 
there's nothing I can think of to anchor it to? If I anchor it to the 
notification text in any way, I can add a margin, sure, but it gets //stretched 
out beyond belief// if there's too much text.
  
  
  AFter playing around with the code (I'm still very new to this), I came up 
with this:
  
PlasmaCore.IconItem {
id: appIconItem

width: (bodyText.lineCount > 1) ? units.iconSizes.large : 
(notificationItem.height - 2 * units.smallSpacing)
height: (bodyText.lineCount > 1) ? units.iconSizes.large : 
(notificationItem.height - 2 * units.smallSpacing) 
  
  And a dozen binding loops later (wow!) the padding is indeed even with one 
heading + one line of description, regardless of what font is used (and the 
icon is still a square). And it looks fine with 1x, 1.5x, 2x, 2.5x scaling set 
up, but when it's 1.1, 1.2 etc. or the font is >10 pt in size, the icon is 
larger when the lineCount is 1 than when the line count is greater than 1.
  
  This solution seems very strange/crude to me though, and it's not without its 
downsides.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  There is one small problem, with almost every version I've submitted so far.
  The padding above the icon and below the icon is the same (7 px with 1x 
scaling) if Noto Sans is used as the default font, which is basically perfect.
  
  However, if another font is used, the amount of padding below the icon 
changes (Cabin - 5 px, Lato - 3 px etc.).
  It's because I've been using bottomPart or the implicitHeight to make the 
notification "taller" so as to give the icon padding.
  
  Is there a way to give the icon itself padding? I've been using anchors but 
there's nothing I can think of to anchor it to? If I anchor it to the 
notification text in any way, I can add a margin, sure, but it gets //stretched 
out beyond belief//.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49132.
rooty added a comment.


  Remove unnecessary comments

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49131=49132

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49131.
rooty added a comment.


  Use margin in bottomPart instead

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=49110=49131

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  > The end result is the same though.
  
  If the code's neater, lets see it.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  Hey @davidedmundson I know I'm a little late but should I use your suggestion 
instead? I just figured out how to do it using Layout.bottomMargin :D

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: rooty, #vdg, #plasma, ngraham, davidedmundson
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty updated this revision to Diff 49110.
rooty added a comment.


  Change certain integers to decimals for more even padding; fix padding in 
heading-only notifications

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=48943=49110

BRANCH
  master

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  @broulik If the consensus is that the notifications should stay "tight" (no 
padding), I'd very much prefer to add padding to the left and right of the icon 
(it's really stuck-on right now), and to push the notification icon down a bit 
if there's a bodyText.lineCount > 1 to prevent the icon from towering over the 
heading.
  
  F6537400: image.png 

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  In D17975#389769 , @rooty wrote:
  
  > In D17975#389760 , @filipf wrote:
  >
  > > FWIW if I backport the spacings and margins prior to D3560 
 it works great for me. The symmetry around 
the icon is better than with this diff. (only NotificationItem.qml is modified)
  > >
  > > This diff:
  > >  F6537366: Screenshot_20190109_000448.png 

  > >
  > > Old:
  > >  F6537364: image.png 
  >
  >
  > Try rolling back NotificationPopup.qml too. They should look very similar 
to the ones in this patch. (The idea is that the notification in the second 
screenshot has padding only because arc-kde provides it.)
  
  
  Still better with the old values:
  
  F6537395: Screenshot_20190109_124600.png 

  
  F6537396: Screenshot_20190109_124634.png 


REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389760 , @filipf wrote:
  
  > FWIW if I backport the spacings and margins prior to D3560 
 it works great for me. The symmetry around 
the icon is better than with this diff. (only NotificationItem.qml is modified)
  >
  > This diff:
  >  F6537366: Screenshot_20190109_000448.png 

  >
  > Old:
  >  F6537364: image.png 
  
  
  Try rolling back NotificationPopup.qml too. They should look very similar to 
the ones in this patch.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389695 , @broulik wrote:
  
  > We explicitly removed the padding in Plasma 5.9 to reduce popup sizes, this 
effectively reverts this decision…
  
  
  Sort of. This is basically what they looked like:
  F6537353: image.png 
  And the result of D3560  were a slew of 
custom themes that added the padding back. The main justification was:
  
  > Our notification popups are huge compared to other platforms
  
  and that's just not the case. At least not anymore:
  
  F6537359: image.png 
  F6537361: image.png 
  
  I grant that notifications without padding do save room. But it's really not 
that much room we're saving? Especially considering that a notification is 
something a user should be paying attention to.
  As far as text on the buttons is concerned, I'm confident we can make it work 
and that the labels need not be too short or too long, or in other words, that 
this is an issue completely separate from the padding issue.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  FWIW if I backport the spacings and margins prior to D3560 
 it works great for me. The symmetry around 
the icon is better than with this diff.
  
  This diff:
  F6537366: Screenshot_20190109_000448.png 

  
  Old:
  F6537364: image.png 

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Kai Uwe Broulik]

broulik added a comment.


  We explicitly removed the padding in Plasma 5.9 to reduce popup sizes, this 
effectively reverts this decision…

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: broulik, Codezela, abetts, filipf, davidedmundson, hein, ndavis, 
plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389613 , @davidedmundson 
wrote:
  
  > BTW, if you want a neater solution: (though consider this optional)
  >  Instead, use Layout.topMargin on the titleBar and Layout.bottomMargin on 
the lowerPart.
  
  
  I love it. It's just that the icon doesn't seem to get the message :/ That's 
why I've been manipulating the height this entire time. I'd like to implement 
this but I don't know how to add padding to IconItem... (I can't think of 
anything to anchor it to - anchoring it to parent.bottom and adding a margin 
stretches the icon out / distorts it)
  
  > There's a logic error. I'll rewrite it so you can see it.
  
  I'm sorry I still don't get it 
  If greater than 1 then return 0.5, otherwise (it's less than or equal to 1 = 
either 0 or 1?) if 0 then 0, otherwise (not zero but 1) return 2.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  BTW, if you want a neater solution: (though consider this optional)
  
  At the moment you're moving mainLayout about and then doing maths on the 
window height to make sure we get the same padding underneath.
  
  Instead, use Layout.topMargin on the titleBar and Layout.bottomMargin on the 
lowerPart.
  
  Then QtLayouts can do all the hard work, and you're spreading this logic 
about to the relevant places next to where you want the padding.
  
  (you'll have to bump "import QtQuick.Layouts 1.1" to 1.2)

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  @filipf 
  The media player situation was very different. This won't break anything.
  
  @rooty
  
  ((bodyText.lineCount > 1) ? 0.5 : ((bodyText.lineCount == 0) ? 0 : 2) * 
units.smallSpacing)))
  
  There's a logic error. I'll rewrite it so you can see it.
  
if (lineCount > 1) {
return 0.5;
} else {
   if (lineCount == 0) {   
  return 0;
   } else {
 return 2;
   }
}

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  > I wouldn't want the attitude of "breaking other themes is unfortunate, 
but.." becoming a pattern because openness to customization is our main selling 
point.
  
  You've made your point. But allow me to flesh it out  - it seems we've 
reached an impasse; in simple terms, you're either going to maintain the themes 
and restrict Plasma's growth or you're going to remedy the problems this widget 
has exhibited and step on a few toes.
  
  You can, in fact, modify Breeze to add padding, sure. But if you do so, you 
can't move the notification text up (or around, if you should want to). I'm 
kind of belaboring the point here - that means you can't get padding that 
stretches and contracts naturally depending on how much notification content 
there is. (I never wanted **just** padding.)
  
  You can also manipulate the notification icon to achieve the same effect that 
I achieved by lowering the value of topMargin of the notification heading. 
However even then, padding that's arrived at through theme manipulation is 
still always going to remain the same.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  @davidedmundson I've tried using
  
implicitHeight: Math.max(appIconItem.valid || imageItem.nativeWidth > 0 ? 
units.iconSizes.large : 0, (mainLayout.height + ((bodyText.lineCount > 1) ? 0.5 
: ((bodyText.lineCount == 0) ? 0 : 2) * units.smallSpacing))) // Add 
units.smallSpacing to correct for the topMargin of mainLayout according to 
number of lines
  
  (I know, it looks ugly...) but it doesn't seem to do anything

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  In D17975#389584 , @ngraham wrote:
  
  > So what's going on here? Other themes add more padding? If so, I don't see 
how that's something we can possibly account for, and that shouldn't affect our 
ability to improve our own stuff.
  
  
  Yes, as you can see in the pics it doesn't just add extra padding but may 
also result in worse symmetry before.
  
  I'm simply saying we try to investigate if there might be a solution that 
improves our stuff, but doesn't regress other stuff.
  
  People do use other themes and they do notice when things get messed up. 
Example of the last time this happened:
  
  
https://www.reddit.com/r/kde/comments/8rj3k7/wrong_media_player_icon_after_513_update/
  
https://www.reddit.com/r/ManjaroLinux/comments/90drav/media_player_tray_icon_missing_kde/
  
https://www.reddit.com/r/kde/comments/93thby/issues_with_the_new_media_player_icons/
  
https://forum.manjaro.org/t/manjaro-18-kde-media-player-no-panel-icon-using-breath-theme-set/64010
  https://forum.manjaro.org/t/kde-breath-theme-missing-icons/51644
  https://forum.manjaro.org/t/kde-media-player-indicator-slot-empty/64199
  
https://forum.manjaro.org/t/manjaro-18-kde-media-player-no-panel-icon-using-breath-theme-set/64010
  
https://forum.manjaro.org/t/smplayer-missing-tray-icon-in-kde-when-playing-movie/58021
  
  I wouldn't want the attitude of "breaking other themes is unfortunate, but.." 
becoming a pattern because openness to customization is our main selling point.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389580 , @filipf wrote
  
  > I'd much rather if there was some padding added in Breeze and then you only 
fix the wrong spacing to right of the notification. Even if that wouldn't work, 
I don't get the sense we've fully explored other solutions yet.
  
  
  There are other problems though besides the right margin of the icon. The 
heading can't be lifted to stop the icon from towering over it because the 
theme provides its own padding and, in essence, //locks it into place//.
  
  In other words, the padding here **varies depending on the amount of 
notification content**, which I don't think is something a theme can account 
for.
  
  Modifying Breeze to add padding would restrict its ability to manipulate the 
notification content fully.
  
  In D17975#389584 , @ngraham wrote:
  
  > So what's going on here? Other themes add more padding? If so, I don't see 
how that's something we can possibly account for, and that shouldn't affect our 
ability to improve our own stuff.
  
  
  We may be able to account for it, but our hands are tied here I'm afraid 
because of the technical difficulties of making the Breeze theme doing what I 
want the notifications to do in this patch (vary padding by notification 
content).

INLINE COMMENTS

> davidedmundson wrote in NotificationItem.qml:33
> Much better.
> 
> Only part I don't understand, then ship it! from me.
> 
> Assuming there's no icon
> 
> For multiline
> 
> - 1 small space above 1 small space under.
> 
> For one line we effectively get:
> 
> - 0 margin on top, 0.5 under
> 
> Why the 0.5? Surely that would be inconsistent?
> 
> (In all your examples the icon is bigger so we don't see it)

You're right! I never tested it with just the heading, There's an extra 0.5 * 
units.smallSpacing of padding there. But I needed that 0.5 units to provide 
even padding to notifications that are "Heading + One line of description".

Is there any way I can put in a condition that if it's just a heading that it 
not put that 0.5 * units.smallSpacing there?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  So what's going on here? Other themes add more padding? If so, I don't see 
how that's something we can possibly account for, and that shouldn't affect our 
ability to improve our own stuff.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  In D17975#389575 , @rooty wrote:
  
  > Breaking other desktop themes is unfortunate ...
  
  
  It's a bit more than unfortunate. Similar things have happened recently e.g. 
with the new media player icons and with the network widget. This causes 
tangible annoyance among the user base. The best solution for Breeze should be 
the priority, yes, but not if there are solutions that work well for all 
parties involved. I'd much rather if there was some padding added in Breeze and 
then you only fix the wrong spacing to right of the notification. Even if that 
wouldn't work, I don't get the sense we've fully explored other solutions yet.

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Hazem Salem]

Codezela added a comment.


  In D17975#389576 , @rooty wrote:
  
  > In D17975#389567 , @Codezela 
wrote:
  >
  > > why every notification width is too wide
  > >  why its not resize depends on the content on it with some little padding
  >
  >
  > what do you mean? the width of the entire notification?
  
  
  i mean the space between the close button and the text vertically

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389567 , @Codezela wrote:
  
  > why every notification width is too wide
  >  why its not resize depends on the content on it with some little padding
  
  
  what do you mean? the width of the entire notification?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Root]

rooty added a comment.


  > Wouldn't it be better to somehow add padding globally?
  
  Not really feasible because you don't necessarily want padding everywhere 
(for example, Adapta does this and i think it looks unsightly)
  F6536644: adapta-popup.png 
  Adding padding **//everywhere//** might involve modifying every heading and 
every icon, that's just not what we want to do (you don't, for instance, want 
padding around system settings sidebar icons which might be a side effect of 
what you're suggesting)
  
  If you limit yourself to only text/headings, then adding padding may make 
your GUI elements look disproportionate (the text gets padding, the icon 
doesn't and gets pushed up and to the left (provided the theme doesn't provide 
you with padding), which is what happened in some of my mockups)
  
  Breaking other desktop themes is unfortunate, but our primary responsibility 
is in my opinion to Breeze - maybe we can find a way to modify Breeze to add 
more padding/cushioning, but then two problems arise:
  
  (1) This would prevent the text from being shifted upward with a line count 
greater than 1 and cause **//even greater inconsistency//** in the margins 
(they would become very asymmetrical) - you could work around this but these 
workaround are probably going to be very //inelegant//
  (1) These modifications would have to be implemented in Breeze Dark, Breeze 
LIght and possibly even Breath (the main theme Manjaro Linux uses), and any 
other theme that wants to follow suit
  (3) This would also open up a couple of other very interesting questions - 
should other Plasma elements be modified, or should the widgets that call for 
them be modified? Should we rely on desktop themes to add padding to "Audio 
Volume" or "Status & Notifications"?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Hazem Salem]

Codezela added a comment.


  why every notification width is too wide
  why its not resize depends on the content on it with some little padding

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: Codezela, abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, 
kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  This is what I get when I test on 2 of my machines:
  
  F6536633: Screenshot_20190109_001429.png 

  
  F6536634: Screenshot_20190109_001555.png 

  
  F6536638: Screenshot_20190109_000551.png 

  
  F6536637: Screenshot_20190109_000448.png 


REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, kvanton, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D17975: [Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> hein wrote in NotificationItem.qml:33
> Adding 1.75 pixels is just too arbitrary (and not an even number, and not 
> scaled by device pixel ratio).

Much better.

Only part I don't understand, then ship it! from me.

Assuming there's no icon

For multiline

- 1 small space above 1 small space under.

For one line we effectively get:

- 0 margin on top, 0.5 under

Why the 0.5? Surely that would be inconsistent?

(In all your examples the icon is bigger so we don't see it)

REPOSITORY
  R120 Plasma Workspace

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

To: rooty, #vdg, #plasma, ngraham
Cc: abetts, filipf, davidedmundson, hein, ndavis, plasma-devel, kvanton, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart