Add top area to notifications
ClosedPublic

Authored by niccolove on Mar 11 2020, 12:57 PM.

Details

Summary

Added the new PlasmoidHeading component to notifications, see:

Diff Detail

Repository
R120 Plasma Workspace
Branch
notification_toparea
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24582
Build 24600: arc lint + arc unit
niccolove created this revision.Mar 11 2020, 12:57 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 11 2020, 12:57 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
niccolove requested review of this revision.Mar 11 2020, 12:57 PM
niccolove edited the summary of this revision. (Show Details)Mar 11 2020, 12:58 PM
niccolove added reviewers: broulik, ngraham.

Please also test with icons and configure button, I use https://tests.peter.sh/notification-generator/ which can generate a multitude of test notifications

applets/notifications/package/contents/ui/NotificationPopup.qml
168 ↗(On Diff #77406)

You know I'm not a fan of randomly dividing sizes

ngraham retitled this revision from Added top area to notifications to [WIP] Add top area to notifications.Mar 11 2020, 3:13 PM
ngraham edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Mar 11 2020, 3:24 PM

Please test to make sure that your patches work :)

applets/notifications/package/contents/ui/NotificationItem.qml
114–116

there's no such thing as PlasmaExtraComponents

119

there are spaces on this line

This revision now requires changes to proceed.Mar 11 2020, 3:24 PM

Please also test with icons and configure button, I use https://tests.peter.sh/notification-generator/ which can generate a multitude of test notifications

Yepp, I did test it with various notifications.

Please test to make sure that your patches work :)

I do! But since I was unable to add that class to my local framework, I tested it by putting the PlasmoidHeading in the same folder and using "PlasmoidHeading" instead of "PlasmaExtras.PlasmoidHeading". Then I replaced it to make the diff, but got the name wrong.

Anyway - the main thing missing in this task is how to avoid broke history - do you have any tip on how I could address it?

applets/notifications/package/contents/ui/NotificationItem.qml
119

I set up Kate to show trailing spaces now, so I should stop putting them accidentally.

applets/notifications/package/contents/ui/NotificationPopup.qml
168 ↗(On Diff #77406)

What should I use? 0 is too small, while the entire margin is too big.

Please also test with icons and configure button, I use https://tests.peter.sh/notification-generator/ which can generate a multitude of test notifications

Yepp, I did test it with various notifications.

Please test to make sure that your patches work :)

I do! But since I was unable to add that class to my local framework

If it doesn't work for you, that's a good bet it won't work for other people either, which means it can't actually be tested and merged. :) Whatever is preventing it from working when you integrate it in the correct way is a bug that needs to be fixed so that the patch is testable and merge-able, not something you can hack around.

niccolove updated this revision to Diff 79064.Apr 1 2020, 4:34 PM

Rewrote the whole thing

niccolove updated this revision to Diff 79065.Apr 1 2020, 4:36 PM

Remove unrelated change

niccolove marked 3 inline comments as done.Apr 1 2020, 4:36 PM
niccolove retitled this revision from [WIP] Add top area to notifications to Add top area to notifications.
niccolove edited the summary of this revision. (Show Details)

This looks amazing! I love it. I think the content area below the header needs an additional units.smallSpacing margin though. The text in that area comes really close to touching the header:

niccolove added a comment.EditedApr 2 2020, 9:24 AM

Weird, I do see a margin. Do you have any idea what could case this issue (us seeing different things)? I assumed that the margin was given by a qml Heading default topmargin

ngraham accepted this revision.Apr 2 2020, 3:08 PM

The padding looks much better with default font settings:

I'm using 11pt Ubuntu though, not 10pt Noto Sans. So now I'm thinking that there's some issue with making the padding dependent on the font. Still, that's clearly unrelated to this patch (it just exposes a pre-existing issue) so I say go for it! Bonus points for investigating the font padding issue in a follow-up patch. :)

This revision is now accepted and ready to land.Apr 2 2020, 3:08 PM
This revision was automatically updated to reflect the committed changes.

I will add the padding in the todo list :-)

So I think the padding issue isn't related to fonts after all. I noticed while doing a backup today that there's no spacing between the header and the content underneath it even when there's no text.

So I think the padding issue isn't related to fonts after all. I noticed while doing a backup today that there's no spacing between the header and the content underneath it even when there's no text.

I'm sorry, what do you mean with "no text"?

Sorry. With no header text.