D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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,

D17975: [Notifications] Add padding to notifications

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 lik

D17975: [Notifications] Add padding to notifications

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 , committed by ngraham). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https:

D17975: [Notifications] Add padding to notifications

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, fili

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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&id=49230 BRANCH

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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&id=49209 BRANCH master REVISION DETAIL https://phabricator.kde.org/D1797

D17975: [Notifications] Add padding to notifications

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,

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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,

D17975: [Notifications] Add padding to notifications

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&i

D17975: [Notifications] Add padding to notifications

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 unders

D17975: [Notifications] Add padding to notifications

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. REPOSITOR

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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 ico

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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, th

D17975: [Notifications] Add padding to notifications

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&id=49132 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17975 AFFECTED FI

D17975: [Notifications] Add padding to notifications

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&id=49131 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17975 AFFECT

D17975: [Notifications] Add padding to notifications

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, abett

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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&id=49110 BRANCH

D17975: [Notifications] Add padding to notifications

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 >

D17975: [Notifications] Add padding to notifications

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 wo

D17975: [Notifications] Add padding to notifications

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 No

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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, fi

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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 an

D17975: [Notifications] Add padding to notifications

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.

D17975: [Notifications] Add padding to notifications

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 ter

D17975: [Notifications] Add padding to notifications

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.

D17975: [Notifications] Add padding to notifications

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 st

D17975: [Notifications] Add padding to notifications

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 ful

D17975: [Notifications] Add padding to notifications

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.

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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? RE

D17975: [Notifications] Add padding to notifications

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 Ad

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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

D17975: [Notifications] Add padding to notifications

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