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

This revision was automatically updated to reflect the committed changes. Closed by commit R120:23f3345a2287: [notifications] Lift up notification content if one line of body text droops (authored by rooty). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

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

rooty updated this revision to Diff 53523. rooty added a comment. Rebase if needed REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=52409=53523 BRANCH arcpatch-D19036 REVISION DETAIL https://phabricator.kde.org/D19036 AFFECTED FILES

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

filipf added a comment. In D19036#427849 , @rooty wrote: > In D19036#427845 , @cfeck wrote: > > > The status of this says 'Accepted', so developers might not check it. > > > > If you want to wait

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

rooty added a comment. In D19036#427845 , @cfeck wrote: > 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

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

cfeck added a comment. 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'. REPOSITORY R120 Plasma Workspace BRANCH arcpatch-D19036 REVISION DETAIL

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

ngraham accepted this revision. ngraham added a comment. @broulik, does this look good to you? REPOSITORY R120 Plasma Workspace BRANCH arcpatch-D19036 REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty, #plasma, #vdg, broulik, ngraham, filipf Cc: anthonyfieroni, filipf,

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

rooty updated this revision to Diff 52409. rooty added a comment. Missed a comma REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=52408=52409 BRANCH arcpatch-D19036 REVISION DETAIL https://phabricator.kde.org/D19036 AFFECTED FILES

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

rooty updated this revision to Diff 52408. rooty added a comment. Tidy up comments REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=52088=52408 BRANCH arcpatch-D19036 REVISION DETAIL https://phabricator.kde.org/D19036 AFFECTED FILES

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

filipf accepted this revision. filipf added a comment. This revision is now accepted and ready to land. Tested it and it looks good to me. REPOSITORY R120 Plasma Workspace BRANCH lift-up-noto (branched from master) REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty,

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

rooty updated this revision to Diff 52088. rooty added a comment. Edit comments REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=52087=52088 BRANCH lift-up-noto (branched from master) REVISION DETAIL https://phabricator.kde.org/D19036

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

rooty added a comment. 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

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

rooty updated this revision to Diff 52087. rooty added a comment. Even padding with bodyText.lineCount > 1 (account for close button) REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51858=52087 BRANCH lift-up-noto (branched from master)

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

rooty updated this revision to Diff 51858. rooty added a comment. Improve indentation REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51857=51858 BRANCH lift-up-noto (branched from master) REVISION DETAIL

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

rooty updated this revision to Diff 51857. rooty added a comment. Remove first two else branches / simplify REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51823=51857 BRANCH lift-up-noto (branched from master) REVISION DETAIL

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

anthonyfieroni added inline comments. INLINE COMMENTS > NotificationItem.qml:34-47 > +if (bodyText.lineCount > 1) { > +return mainLayout.height > +} else { > +if (appIconItem.valid || imageItem.nativeWidth > 0) { > +return

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

rooty added a comment. Okay, it works now and it uses if then else syntax (and a teeny tiny unintrusive ternary operator :D) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty, #plasma, #vdg, broulik, ngraham Cc: filipf, ngraham, abetts,

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

rooty updated this revision to Diff 51823. rooty added a comment. Fix bug, simplify REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51777=51823 BRANCH lift-up-noto (branched from master) REVISION DETAIL

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

rooty requested review of this revision. rooty added a comment. There's a bug here that arises if I use the if-then-else syntax F6617956: image.png If I use the ternary operator this doesn't happen F6617962: image.png

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

rooty added a comment. Sure thing. Hopefully it'll also close the bug now that I've adjusted my settings haha REPOSITORY R120 Plasma Workspace BRANCH lift-up-noto (branched from master) REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty, #plasma, #vdg, broulik, ngraham

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

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. 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

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

rooty added a comment. In D19036#412936 , @rooty wrote: > Actually use if-then-else instead of ternary operator My apologies guys I forgot to copy the file to the working copy REPOSITORY R120 Plasma Workspace REVISION DETAIL

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

rooty updated this revision to Diff 51777. rooty added a comment. Actually use if-then-else instead of ternary operator REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51775=51777 BRANCH lift-up-noto (branched from master) REVISION

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

rooty updated this revision to Diff 51775. rooty added a comment. Use if-then-else instead of ternary operator for implicitHeight REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19036?vs=51756=51775 BRANCH lift-up-noto (branched from master)

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

ngraham added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL

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

rooty added a comment. @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) REPOSITORY R120 Plasma Workspace REVISION DETAIL

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

rooty added a comment. In D19036#412890 , @ngraham wrote: > Here, like this: > > function adjustHeight() > { > var layoutHeight = mainLayout.height > if (bodyText.lineCount > 1) { > layoutHeight =

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

ngraham added a comment. Here, like this: function adjustHeight() { var layoutHeight = mainLayout.height if (bodyText.lineCount > 1) { layoutHeight = mainLayout.height } else { if (appIconItem.valid || imageItem.nativeWidth > 0) {

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

rooty added a comment. In D19036#412876 , @ngraham wrote: > The lines below an `if` or `else` are always indented. function adjustHeight() { var layoutHeight = mainLayout.height if (bodyText.lineCount > 1) {

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

filipf added a comment. Nice, this patch makes it look better for me. Before: F6616985: image.png After: F6616988: image.png REPOSITORY R120 Plasma Workspace REVISION DETAIL

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

rooty added a comment. In D19036#412855 , @filipf wrote: > Nice, this patch makes it look better for me. > > Before: > F6616985: image.png > > After: > F6616988: image.png

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

ngraham added a comment. In D19036#412851 , @rooty wrote: > How's this? > > function adjustHeight() > { > var layoutHeight = mainLayout.height > if (bodyText.lineCount > 1) { > layoutHeight =

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

rooty added a comment. In D19036#412830 , @ngraham wrote: > Can you use the typical KDE coding style for if/else blocks? How's this? function adjustHeight() { var layoutHeight = mainLayout.height if

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

ngraham added a comment. In D19036#412809 , @rooty wrote: > I hope I didn't mess up the syntax... Do you prefer this to the ternary operator? > > function adjustHeight() > { > var layoutHeight = mainLayout.height > if

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

rooty marked 3 inline comments as done. rooty added a comment. I hope I didn't mess up the syntax... function adjustHeight() { var layoutHeight = mainLayout.height if (bodyText.lineCount > 1) { layoutHeight = mainLayout.height } else if

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

abetts added a comment. +1 REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty, #plasma, #vdg Cc: abetts, broulik, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

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

rooty marked an inline comment as done. rooty added inline comments. INLINE COMMENTS > broulik wrote in NotificationItem.qml:175 > Is `smallSpacing` always even? That's the idea yes REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19036 To: rooty, #plasma,

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

broulik added inline comments. INLINE COMMENTS > NotificationItem.qml:33 > width: parent.width > -implicitHeight: bodyText.lineCount > 1 ? mainLayout.height : > (appIconItem.valid || imageItem.nativeWidth > 0 ? > (Math.max((mainLayout.height + 2 *

D19036: [notifications] Lift up notification content if one line of body text droops (improve padding)

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 lifts up the text content of notifications in which the combination of a single line of body text and its heading are