Fix various bugs introduced with heading
ClosedPublic

Authored by niccolove on Apr 27 2020, 4:35 PM.

Details

Summary

This fixes:

  • Indented notifications line (it's back)
  • Heading being non-clickable
  • Buttons not being right-aligned in history
  • Keyboard navigation
  • Long app names

Diff Detail

Repository
R120 Plasma Workspace
Branch
fix_notifications (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26227
Build 26245: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Plasma. · View Herald TranscriptApr 27 2020, 4:35 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
niccolove requested review of this revision.Apr 27 2020, 4:35 PM
niccolove updated this revision to Diff 81365.Apr 27 2020, 4:37 PM

Remove unrelated

niccolove updated this revision to Diff 81366.Apr 27 2020, 4:37 PM

Unrelated pt2

ngraham added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
149

missing space before the curly brace

446

unrelated change

niccolove updated this revision to Diff 81367.Apr 27 2020, 4:38 PM

unrelated pt3

niccolove updated this revision to Diff 81368.Apr 27 2020, 4:39 PM

Missing space

niccolove marked 2 inline comments as done.Apr 27 2020, 4:39 PM
niccolove planned changes to this revision.Apr 27 2020, 4:48 PM

Ahh, this adds top margin in history. Let me fix.

niccolove updated this revision to Diff 81372.Apr 27 2020, 5:26 PM

Fix spacing

broulik requested changes to this revision.Apr 27 2020, 7:31 PM
broulik added inline comments.
applets/notifications/package/contents/ui/FullRepresentation.qml
459

Move the Svg somewhere outside the delegate, I don't want multiple instances of it

applets/notifications/package/contents/ui/NotificationItem.qml
121–126

Please use one of the State below for inGroup state

149

This MouseArea should not be necessary and it makes the close button unusable

This revision now requires changes to proceed.Apr 27 2020, 7:31 PM
niccolove added inline comments.Apr 27 2020, 10:21 PM
applets/notifications/package/contents/ui/NotificationItem.qml
121–126

I originally did this but I had a problem. It displays and works just as good, but it will throw a "you are using anchors in layouts" error in the console. Even though I disable the anchors in the propertychanges, it seems like the property is updated after throwing the error.

149

Why shouldn't it be necessary? PlasmoidHeading inherits from the Control element, which does not let click events through according to the documentation, so I thought I'd need to catch them on top of it.

Regarding the close button unusable, it's due to the order of the elements, I'll fix it immediately

niccolove updated this revision to Diff 81389.Apr 27 2020, 10:22 PM

Fix close button not clickable

broulik added inline comments.Apr 28 2020, 3:03 PM
applets/notifications/package/contents/ui/NotificationItem.qml
121–126

hm, ok

149

inherits from the Control element, which does not let click events through

urgh, really?! Then we can't use it here. I will not accept having two competing MouseAreas doing the same thing.

broulik requested changes to this revision.May 1 2020, 7:30 PM
This revision now requires changes to proceed.May 1 2020, 7:30 PM
niccolove updated this revision to Diff 81727.May 2 2020, 9:24 AM

Remove plasmoidHeading element as it's a control

I used the Svg instead of the plasmoidHeading to avoid using a Control. User-wise, it works correctly, but can I get your opinion code-wise?

applets/notifications/package/contents/ui/FullRepresentation.qml
459

I'm a bit confused here. The PlasmaCore.svg{id: lineSvg...} element was inside a different PlasmaCore.SvgItem that I removed. So what I did was essentially to move it from that SvgItem to this SvgItem. That one is the only instance of PlasmaCore.Svg.

applets/notifications/package/contents/ui/NotificationItem.qml
121–126

I moved it in a RowLayout to remove the necessity of checking altogether (otherwise, I should've put anchors.margins AND layout.margins for each case and it would've have been quite ugly imo)

niccolove updated this revision to Diff 81730.May 2 2020, 9:35 AM

Fix keyboard navigation

niccolove edited the summary of this revision. (Show Details)May 2 2020, 9:35 AM

Keyboard navigation is now working again; however, pressing tab to focus buttons seem to no longer be possible after this patch; looking at the code, I'm given the impression that it was true before porting notifications to page as well. Is that correct, or should I investigate tabbing?

niccolove planned changes to this revision.May 2 2020, 9:43 AM

Ah, there's a mistake on visibility, just a sec

niccolove updated this revision to Diff 81732.May 2 2020, 10:03 AM

Fix visibility

niccolove updated this revision to Diff 81733.May 2 2020, 10:03 AM

Move lineSvg to root element

broulik added inline comments.May 2 2020, 10:26 AM
applets/notifications/package/contents/ui/NotificationItem.qml
116

When inGroup the height will be zero so you effectively leak the item contents outside? I don't get what this is supposed to do.

128

This item is pointless since there's only the NotificationHeader inside

niccolove updated this revision to Diff 81748.May 2 2020, 5:37 PM

Address feedback

niccolove marked 11 inline comments as done.May 2 2020, 5:38 PM
niccolove edited the summary of this revision. (Show Details)
niccolove planned changes to this revision.May 2 2020, 5:43 PM
niccolove updated this revision to Diff 81750.May 2 2020, 5:46 PM

Fix very long notification titles

niccolove updated this revision to Diff 81791.May 3 2020, 11:43 AM

Fix very long application names

niccolove edited the summary of this revision. (Show Details)May 3 2020, 11:44 AM
broulik accepted this revision.May 3 2020, 12:06 PM
broulik added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
117

Put id at the top

121

Use "

122

You can group those

anchors {
    fill: parent
    topMargin: ...
    ...
}
174

Keep it where it was

This revision is now accepted and ready to land.May 3 2020, 12:06 PM
niccolove updated this revision to Diff 82060.May 6 2020, 8:20 AM

Address feedback and add inHistory bool

niccolove requested review of this revision.May 6 2020, 8:22 AM
niccolove marked 4 inline comments as done.

I added one last thing: I feel like it was not a good idea to show the heading in history for notifications that are not in group. I added a inHistory bool, false by default, that's set true in FullRepresentation for notifications that are in history. Heading is not shown for any notification in the history.

niccolove edited the summary of this revision. (Show Details)May 6 2020, 8:23 AM

I added one last thing

Can we please not mix behavior changes into a patch that fixes bugs and regressions.

Sorry, I'll make a different patch for that

niccolove edited the summary of this revision. (Show Details)May 6 2020, 8:24 AM
niccolove updated this revision to Diff 82062.May 6 2020, 8:24 AM

Remove inHistory change

broulik accepted this revision.May 6 2020, 8:42 AM
This revision is now accepted and ready to land.May 6 2020, 8:42 AM
This revision was automatically updated to reflect the committed changes.