D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#389443 , @ngraham wrote:
  
  > This is no longer WIP, right?
  
  
  Oh so I can rename it? Sure!

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: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  This is no longer WIP, right?

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: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  In D17975#389141 , @abetts wrote:
  
  > I would be in favor of more consistency wherever possible. However if we 
can't get that, I would still vote for having these changes applied to 
notifications rather than not have them because of inconsistencies elsewhere.
  
  
  +1. FWIW, adding padding globally is probably not possible because of how 
specific layouts are constructed. We'd need to do it on an individual basis.

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: [WIP, Notifications] Add padding to notifications

[Andres Betts]

abetts added a comment.


  In D17975#388981 , @filipf wrote:
  
  > I have some concerns about this. Mostly it's about visual consistency and 
notifications having different padding than other plasmoids. Notice how there's 
currently more or less equal distance to the left of Audio Volume and to the 
left of the album cover icon?
  >
  > F6535430: Screenshot_20190108_133709.png 

  >
  > Wouldn't it be better to somehow add padding globally?
  >
  > Plus, some desktop themes already add padding, which would make their 
notifications look bad. Can you test with some other desktop themes?
  
  
  I would be in favor of more consistency wherever possible. However if we 
can't get that, I would still vote for having these changes applied to 
notifications rather than not have them because of inconsistencies elsewhere.

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: [WIP, Notifications] Add padding to notifications

[Filip Fila]

filipf added a comment.


  I have some concerns about this. Mostly it's about visual consistency and 
notifications having different padding than other plasmoids. Notice how there's 
currently more or less equal distance to the left of Audio Volume and to the 
left of the album cover icon?
  
  F6535430: Screenshot_20190108_133709.png 

  
  Wouldn't it be better to somehow add padding globally?
  
  Plus, some desktop themes already add padding, which would make their 
notifications look bad. Can you test with some other desktop themes?

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  Before and after would be perfect, yes!

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#388788 , @ngraham wrote:
  
  > Could you update the images in the Summary section instead of posting new 
ones in comments? The visual history of the patch is not really important to 
preserve, and having the images on top always be current makes reviewing it 
much easier. Thanks!
  
  
  haha sure thing, can i just do the before and after then? no more stream of 
consciousness?

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  Could you update the images in the Summary section instead of posting new 
ones in comments? The visual history of the patch is not really important to 
preserve, and having the images on top always be current makes reviewing it 
much easier. Thanks!

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  I removed "0.75 * units.smallSpacing" because it's not an integer - the 
result is that the margins are slightly uneven, but considering they weren't 
ever entirely even, and the difference between the margins gets more pronounced 
with increasing font size (and scaling factor), but isn't noticeable, maybe 
it's a nonissue?
  
  That being said, I've also added a line that raises the notification contents 
by units.smallSpacing if there is more than one line of text, to make the 
margins more even. 
  Before: 
  F6533113: before.png 
  After: 
  F6533115: after.png 

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

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


  Use integers/halves and line counts

REPOSITORY
  R120 Plasma Workspace

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

BRANCH
  master

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

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  In D17975#388728 , @rooty wrote:
  
  > Hey guys, I feel silly asking this but I'm using a different computer right 
now - how do I get this computer's arcanist to lock onto and update this diff 
(instead of creating an entirely new diff...)?
  
  
  
  
arc patch D17975 # Downloads the patch to a local branch

arc diff --update D17975 # forces arc to update this diff instead of 
creating a new one, on the off chance that it would try to

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  Hey guys, I feel silly asking this but I'm using a different computer right 
now - how do I get this computer's arcanist to lock onto and update this diff 
(instead of creating an entirely new diff...)?

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added a comment.


  > How are you scaling plasma? Are you setting PLASMA_USE_QT_SCALING=1 to make 
sure it's really actually using the scale factor you set in the KScreen KCM?
  
  You don't need to set that. Plasma does it's own thing with fonts, fonts will 
be changed regardless.
  
  > Technically Plasma doesn't support fractional scale factors yet
  
  It kinda does because it does it's own thing with the fonts...unless you set 
that env var above.

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  How are you scaling plasma? Are you setting `PLASMA_USE_QT_SCALING=1` to make 
sure it's really actually using the scale factor you set in the KScreen KCM?
  
  Technically Plasma doesn't support fractional scale factors yet (see 
https://bugs.kde.org/show_bug.cgi?id=356446) so if the only bugs you have occur 
there, maybe we can live with that?

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  @davidedmundson Is there anything I can put in the code to get the icons to 
scale properly? this is a screenshot of master (not this patch):
  
  F6529598: image.png 
  
  I've tried multiplying units.iconSizes.large by the scaling factor, and that 
works, but that's as ballpark a solution as they come...

INLINE COMMENTS

> davidedmundson wrote in NotificationItem.qml:175
> > Is there any way around this? Because if it's not 0.75 then it's shifted 
> > too far down (uneven padding)
> 
> If that's the case, adjust the other side, even adjust the window if you need 
> to.

Nate's right though, I should abandon the idea of totally even padding because 
it'll never hold up as you change fonts or scaling... then again it doesn't 
have to be perfect, right? Just close enough?

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> rooty wrote in NotificationItem.qml:175
> Is there any way around this? Because if it's not 0.75 then it's shifted too 
> far down (uneven padding)

> Is there any way around this? Because if it's not 0.75 then it's shifted too 
> far down (uneven padding)

If that's the case, adjust the other side, even adjust the window if you need 
to.

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty marked an inline comment as done.
rooty added a comment.


  In D17975#386685 , @ngraham wrote:
  
  > A bit more padding is good!
  >
  > 1. Making this pixel-perfect with every combination of font and font size 
is going to be impossible sadly. With that in mind, the most elegant design is 
one that makes this not a problem, because it doesn't have to be pixel-perfect 
and all the baselines are at least aligned.
  
  
  Yeah...
  
  > 2. No matter what you pick here, it will look wrong in some cases. :(  If 
you anchor the icon to the top left, it will look bad the moment there's a 
second line of text. But if you vertically center it, then it looks bad anytime 
the notification is tall. Judgment call I think, but personally I'm in favor of 
anchoring it to the top left.
  
  Top left sounds good. I'm on the fence to be honest, but I thought to myself 
hey this is a problem for another diff anyway so why not take care of the 
padding first 😆

INLINE COMMENTS

> hein wrote in NotificationItem.qml:175
> Fractions of smallSpacing can lead to subpixel positioning causing blurry 
> text.

Is there any way around this? Because if it's not 0.75 then it's shifted too 
far down (uneven padding)

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  A bit more padding is good!
  
  1. Making this pixel-perfect with every combination of font and font size is 
going to be impossible sadly. With that in mind, the most elegant design is one 
that makes this not a problem, because it doesn't have to be pixel-perfect and 
all the baselines are at least aligned.
  
  2. No matter what you pick here, it will look wrong in some cases. :(  If you 
anchor the icon to the top left, it will look bad the moment there's a second 
line of text. But if you vertically center it, then it looks bad anytime the 
notification is tall. Judgment call I think, but personally I'm in favor of 
anchoring it to the top left.

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

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


  Change shift in height of notification to an integer

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=48709&id=48711

BRANCH
  notification-padding (branched from master)

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

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  In D17975#386665 , @ngraham wrote:
  
  > Can you clarify what the ultimate visual goal here? Clearly defining that 
might help to tighten up your code and thought process.
  >
  > Also, what remaining issues make this WIP? I find that the Phabricator 
`NOTE:` markup works well for this. See 
https://secure.phabricator.com/book/phabricator/article/remarkup/
  
  
  The basic idea is to get padding all around the notification contents, 
without making the heading stick out too much above the notification item. 
Basically pushing everything slightly inward.
  
  There are two issues that are beyond the scope of this diff though:
  
  - 1) A technical concern: Will it scale properly? Will it handle different 
font sizes properly? At present the notification widget doesn't scale properly 
and I don't know how to fix that so that wasn't my primary concern:
  
  F6525885: image.png 
  
  NOTE: that is an issue present in master (this screenshot is of master, not 
my patch)
  
  
  
  - 2) A design concern: Should the notification icon be lowered in position 
based on the //amount// of content (text) in the notification like what happens 
in certain Windows 10 notifications:
  
  F6525887: image.png 

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Nathaniel Graham]

ngraham added a comment.


  Can you clarify what the ultimate visual goal here? Clearly defining that 
might help to tighten up your code and thought process.
  
  Also, what remaining issues make this WIP? I find that the Phabricator 
`NOTE:` markup works well for this. See 
https://secure.phabricator.com/book/phabricator/article/remarkup/

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> NotificationItem.qml:33
>  width: parent.width
> -implicitHeight: Math.max(appIconItem.valid || imageItem.nativeWidth > 0 
> ? units.iconSizes.large : 0, mainLayout.height)
> +implicitHeight: Math.max(appIconItem.valid || imageItem.nativeWidth > 0 
> ? units.iconSizes.large : 0, (mainLayout.height + 1.75 * units.smallSpacing)) 
> // Add 1.75 units.smallSpacing to correct for the topMargin of mainLayout
>  

Adding 1.75 pixels is just too arbitrary (and not an even number, and not 
scaled by device pixel ratio).

> NotificationItem.qml:175
>  top: parent.top
> +topMargin: units.smallSpacing * 0.75
>  left: appIconItem.valid || imageItem.nativeWidth > 0 ? 
> appIconItem.right : parent.left

Fractions of smallSpacing can lead to subpixel positioning causing blurry text.

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

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


  Add margins to the right of the buttons on the right border

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=48708&id=48709

BRANCH
  notification-padding (branched from master)

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

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  Okay, so I figured out how to add padding to the buttons as well, by adding a 
right margin to the mainLayout part (hence the deleted comment describing how I 
couldn't figure it out, my bad):
  
  F6525782: before-n-after.png 

REPOSITORY
  R120 Plasma Workspace

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

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

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty added a comment.


  Hey so I'd also like for the Open... (image three in each set) button to 
shift by one units.smallSpacing to the left.
  The close button does, if I add "rightMargin: units.smallSpacing" into 
ColumnLayout / mainLayout, but the Open... button doesn't.
  
  I just don't know how to go about it. Anyone have any idea how to do this?
  Is this worth changing or should the "Open..." (and other buttons like "Got 
it" etc.) stay where it is?

REPOSITORY
  R120 Plasma Workspace

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

To: rooty
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17975: [WIP, Notifications] Add padding to notifications

[Root]

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


  Correct spelling

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17975?vs=48707&id=48708

BRANCH
  notification-padding (branched from master)

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

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

To: rooty
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17975: [WIP, Notifications] Add padding to notifications

[Root]

rooty created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
rooty requested review of this revision.

REVISION SUMMARY
  This patch aims to add padding to the notification widget and fully remedy 
the issues that arise once it is applied.

TEST PLAN
  [Coming soon]

REPOSITORY
  R120 Plasma Workspace

BRANCH
  notification-padding (branched from master)

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

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

To: rooty
Cc: plasma-devel, kvanton, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart