Introduce setWindow and CloseWhenWindowActivated
Needs ReviewPublic

Authored by nicolasfella on May 3 2020, 8:10 PM.

Details

Reviewers
broulik
Group Reviewers
Frameworks
Summary

This is a replacement for CloseOnWidgetActivated that operates on a QWindow instead of QWidget.

Test Plan

PoC port of Yakuake: D29392

Diff Detail

Repository
R289 KNotifications
Branch
closewin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27019
Build 27037: arc lint + arc unit
nicolasfella created this revision.May 3 2020, 8:10 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 3 2020, 8:10 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.May 3 2020, 8:10 PM

Generally +1
The QWidget one has some additional features, like closing the notification when the window is destroyed (cf. its docs), which would be useful?
Not a fan of the timer.
Also, we might want to deprecate the QWidget one now?

Generally +1
The QWidget one has some additional features, like closing the notification when the window is destroyed (cf. its docs), which would be useful?

I'll take a look

Not a fan of the timer.

Me neither, but the widgets code has this too, so I kept it. I can remove it though if we decide we don't want it

Also, we might want to deprecate the QWidget one now?

That's my plan, but I still need to port some things

kossebau added inline comments.
src/knotification.h
598

And some remarks from a local API docs pedant :)

Please place the text not on the first line after "/**", but the second line only (for consistency at least). Also keep indent at one space in the following lines then again afte the asterisk.

Also wants a "@since" note, same with the getter :) Them being jealous on the one you gave to the flag.

I would also propose to place these methods for the new property before the two methods tagged with "@internal", to keep some ordering in the list of methods for the human reader of the code.

The QWidget one has some additional features, like closing the notification when the window is destroyed (cf. its docs), which would be useful?

lol, turns out that doesn't even work when using setWidget instead of the ctor with a widget because of 30b10492

  • Address APIdocs issues
  • Remove timer
nicolasfella edited the test plan for this revision. (Show Details)May 18 2020, 6:36 PM
anthonyfieroni added inline comments.
src/knotification.cpp
263

Flags can be changed, disconnect window, if present and activation flags is not set, connect in opposite.

585

Disconnect previously window to this, if present

586–590

Connect if flag present.

broulik added inline comments.May 19 2020, 6:48 AM
src/knotification.h
253

Should we deprecate this? Or just use this one instead. I don't see why we would have separate enum values for those