[notifications] Lift up notification content if one line of body text droops
ClosedPublic

Authored by rooty on Feb 15 2019, 2:00 PM.

Details

Summary

This patch lifts up the text content of notifications in which the combination of a single line of body text and its heading are taller than their icon and thereby prevents the notification content from drooping too far down and having too much of a bottom margin if it should become much larger than the icon. Seeing as the icon and its margins are never going to be taller than the content in question and the icon is always going to have 1 units.smallSpacing above it, this patch adjusts the height accordingly (0.5 units of smallSpacing above + 0.5 for the spacing below the heading + 0.5 below the content, accommodating the text) to achieve more even padding.
This patch should also improve padding with fonts greater than 10 pt in size.

BUG: 404407
FIXED-IN: 5.15.3

Test Plan

Before (left) and after (right):

Diff Detail

Repository
R120 Plasma Workspace
Branch
lift-up-noto (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8344
Build 8362: arc lint + arc unit
rooty created this revision.Feb 15 2019, 2:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 15 2019, 2:00 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rooty requested review of this revision.Feb 15 2019, 2:00 PM
rooty edited the test plan for this revision. (Show Details)Feb 15 2019, 2:01 PM
rooty added reviewers: Plasma, VDG.
rooty retitled this revision from [notifications] Lift up notification content if one line of body text droops (improve padding) to [notifications] Lift up notification content if one line of body text droops.
rooty edited the summary of this revision. (Show Details)Feb 15 2019, 2:08 PM
rooty edited the summary of this revision. (Show Details)
broulik added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
32–33

Can you please untangle this into a proper if statement

197

Is smallSpacing always even?

rooty marked an inline comment as done.Feb 15 2019, 3:20 PM
rooty added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
197

That's the idea yes

rooty edited the summary of this revision. (Show Details)Feb 15 2019, 3:20 PM
rooty marked 3 inline comments as done.EditedFeb 15 2019, 3:38 PM

I hope I didn't mess up the syntax... Do you prefer this to the ternary operator?

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) layoutHeight = mainLayout.height
      else if (appIconItem.valid || imageItem.nativeWidth > 0) {
        layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else if (bottomPart.height != 0) {
            if (mainLayout.height > units.iconSizes.large) {
              layoutHeight = mainLayout.height + 1.5 * units.smallspacing }
                else layoutHeight = mainLayout.height + 2 * units.smallSpacing }
                  else layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
    return layoutHeight
}

implicitHeight: adjustHeight()

+1

Thanks! :D

rooty edited the summary of this revision. (Show Details)Feb 15 2019, 3:42 PM

I hope I didn't mess up the syntax... Do you prefer this to the ternary operator?

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) layoutHeight = mainLayout.height
      else if (appIconItem.valid || imageItem.nativeWidth > 0) {
        layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else if (bottomPart.height != 0) {
            if (mainLayout.height > units.iconSizes.large) {
              layoutHeight = mainLayout.height + 1.5 * units.smallspacing }
                else layoutHeight = mainLayout.height + 2 * units.smallSpacing }
                  else layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
    return layoutHeight
}

implicitHeight: adjustHeight()

Can you use the typical KDE coding style for if/else blocks?

if (thing) {
    //do stuff
} else {
    if (other_thing) {
        // do other stuff
    } else {
        // lots of stuff
    }
}
rooty added a comment.EditedFeb 15 2019, 4:13 PM

Can you use the typical KDE coding style for if/else blocks?

How's this?

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) {
        layoutHeight = mainLayout.height
    } else {
        if (appIconItem.valid || imageItem.nativeWidth > 0) {
        layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else {
            if (bottomPart.height != 0) {
                if (mainLayout.height > units.iconSizes.large) {
                    layoutHeight = mainLayout.height + 1.5 * units.smallspacing   
                } else {
                    layoutHeight = mainLayout.height + 2 * units.smallSpacing 
                    }
                }
                    else {
                        layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
                    }
            }
        }
    return layoutHeight
}

implicitHeight: adjustHeight()
filipf added a subscriber: filipf.Feb 15 2019, 4:23 PM

Nice, this patch makes it look better for me.

Before:

After:

rooty added a comment.Feb 15 2019, 4:25 PM

Nice, this patch makes it look better for me.

Before:

After:

Excellent!

How's this?

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) {
        layoutHeight = mainLayout.height
    } else {
        if (appIconItem.valid || imageItem.nativeWidth > 0) {
        layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else {
            if (bottomPart.height != 0) {
                if (mainLayout.height > units.iconSizes.large) {
                    layoutHeight = mainLayout.height + 1.5 * units.smallspacing   
                } else {
                    layoutHeight = mainLayout.height + 2 * units.smallSpacing 
                    }
                }
                    else {
                        layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
                    }
            }
        }
    return layoutHeight
}

implicitHeight: adjustHeight()

The lines below an if or else are always indented.

rooty added a comment.Feb 15 2019, 4:30 PM

The lines below an if or else are always indented.

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) {
        layoutHeight = mainLayout.height
    } else {
        if (appIconItem.valid || imageItem.nativeWidth > 0) {
            layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else {
            if (bottomPart.height != 0) {
                if (mainLayout.height > units.iconSizes.large) {
                    layoutHeight = mainLayout.height + 1.5 * units.smallspacing   
                } else {
                    layoutHeight = mainLayout.height + 2 * units.smallSpacing 
                    }
            }
                    else {
                        layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
                    }
            }
        }
    return layoutHeight
}

implicitHeight: adjustHeight()

Here, like this:

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) {
        layoutHeight = mainLayout.height
    } else {
        if (appIconItem.valid || imageItem.nativeWidth > 0) {
            layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else {
            if (bottomPart.height != 0) {
                if (mainLayout.height > units.iconSizes.large) {
                    layoutHeight = mainLayout.height + 1.5 * units.smallspacing   
                } else {
                    layoutHeight = mainLayout.height + 2 * units.smallSpacing 
                }
            } else {
                layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
            }
        }
    }
    return layoutHeight
}
rooty added a comment.Feb 15 2019, 4:44 PM

Here, like this:

function adjustHeight()
{
    var layoutHeight = mainLayout.height
    if (bodyText.lineCount > 1) {
        layoutHeight = mainLayout.height
    } else {
        if (appIconItem.valid || imageItem.nativeWidth > 0) {
            layoutHeight = Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
        } else {
            if (bottomPart.height != 0) {
                if (mainLayout.height > units.iconSizes.large) {
                    layoutHeight = mainLayout.height + 1.5 * units.smallspacing   
                } else {
                    layoutHeight = mainLayout.height + 2 * units.smallSpacing 
                }
            } else {
                layoutHeight = mainLayout.height + units.smallSpacing // because of the close button
            }
        }
    }
    return layoutHeight
}

Ooh that does look nicer...

rooty added a comment.Feb 15 2019, 4:51 PM

@ngraham I don't understand, should I use that instead of the ternary operator in the diff qml?
It looks a lot tidier. But it's actually 23 lines (versus just one really long one :D)

Yes, clarity is always more important than conciseness with code. Code will be read many many more times than written, so it's always of paramount importance that it can be understood.

rooty updated this revision to Diff 51775.Feb 15 2019, 5:12 PM

Use if-then-else instead of ternary operator for implicitHeight

rooty updated this revision to Diff 51777.Feb 15 2019, 5:17 PM

Actually use if-then-else instead of ternary operator

rooty added a comment.Feb 15 2019, 5:18 PM

Actually use if-then-else instead of ternary operator

My apologies guys I forgot to copy the file to the working copy

rooty edited the summary of this revision. (Show Details)Feb 15 2019, 5:42 PM
ngraham accepted this revision.Feb 15 2019, 9:06 PM

Code and behavior look good to me but I'd like @broulik or another Plasma person's review before landing this, please.

Also, if and when this lands, it should probably go in the Plasma/5.15 branch and then merge to master.

This revision is now accepted and ready to land.Feb 15 2019, 9:06 PM

Sure thing.
Hopefully it'll also close the bug now that I've adjusted my settings haha

rooty requested review of this revision.EditedFeb 16 2019, 12:49 AM

There's a bug here that arises if I use the if-then-else version

If I use the ternary operator this doesn't happen

Can anyone please help me figure out why?
This is my first function ever so :D There were bound to be some kinks...

rooty updated this revision to Diff 51823.Feb 16 2019, 2:33 AM

Fix bug, simplify

rooty added a comment.Feb 16 2019, 2:39 AM

Okay, it works now and it uses if then else syntax (and a teeny tiny unintrusive ternary operator :D)

anthonyfieroni added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
34–47

When you have return you don't need else branch

if (bodyText.lineCount > 1) {
    return mainLayout.height
}
if (appIconItem.valid || imageItem.nativeWidth > 0) {
    return Math.max((mainLayout.height + 1.5 * units.smallSpacing),(units.iconSizes.large + 2 * units.smallSpacing))
}
and so on
rooty updated this revision to Diff 51857.Feb 16 2019, 1:29 PM

Remove first two else branches / simplify

rooty updated this revision to Diff 51858.Feb 16 2019, 1:31 PM

Improve indentation

rooty updated this revision to Diff 52087.Feb 19 2019, 5:29 PM

Even padding with bodyText.lineCount > 1 (account for close button)

rooty added a comment.Feb 19 2019, 5:32 PM

I just realized that there was more padding above than below with bodyText.lineCount > 1 because the closeButton takes up another 0.5 units above the text that I didn't take into account in my previous padding patch.
So, why not add it here :D

Sorry, you were right Kai, the padding was uneven but it should be fine now

rooty updated this revision to Diff 52088.Feb 19 2019, 5:33 PM

Edit comments

filipf accepted this revision.Feb 23 2019, 9:02 PM

Tested it and it looks good to me.

This revision is now accepted and ready to land.Feb 23 2019, 9:02 PM
rooty updated this revision to Diff 52408.Feb 23 2019, 9:20 PM

Tidy up comments

rooty updated this revision to Diff 52409.Feb 23 2019, 9:22 PM

Missed a comma

ngraham accepted this revision.Feb 24 2019, 8:50 PM

@broulik, does this look good to you?

rooty marked an inline comment as done.Mar 3 2019, 12:33 AM
cfeck added a subscriber: cfeck.Mar 9 2019, 3:38 PM

The status of this says 'Accepted', so developers might not check it.

If you want to wait for a developer's review, I suggest to keep the status 'Needs Review'.

rooty added a comment.Mar 9 2019, 3:41 PM

The status of this says 'Accepted', so developers might not check it.

If you want to wait for a developer's review, I suggest to keep the status 'Needs Review'.

I could also land it though? :D

filipf added a comment.Mar 9 2019, 4:00 PM

The status of this says 'Accepted', so developers might not check it.

If you want to wait for a developer's review, I suggest to keep the status 'Needs Review'.

I could also land it though? :D

I'm under the impression that if people had anything more to add they already would have done so. 5.15.3 comes out on Tuesday so it would be good to get it in by then.

rooty edited the summary of this revision. (Show Details)Mar 9 2019, 4:02 PM
rooty updated this revision to Diff 53523.Mar 9 2019, 4:05 PM

Rebase if needed

This revision was automatically updated to reflect the committed changes.