Support CriticalNotification type and place it in a CriticalNotificationLayer
ClosedPublic

Authored by broulik on Apr 17 2019, 8:38 AM.

Details

Test Plan

Depends on D20627 D20628 D20631

Can place critical notification windows now, they're shown on top of fullscreen windows and are animated like normal notification windows

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Apr 17 2019, 8:38 AM
Restricted Application added a project: KWin. · View Herald TranscriptApr 17 2019, 8:38 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
broulik requested review of this revision.Apr 17 2019, 8:38 AM
zzag added a subscriber: zzag.Sun, Apr 21, 10:25 PM

In general, the new window type is reasonable, though I'd prefer solution where KWin doesn't have to worry about notification urgency levels. plasmashell has already enough information about full screen clients (both on Wayland and X11), so depending on user preferences it could avoid showing notifications when required.

libkwineffects/kwineffects.h
1887

In KWin we do

/**
 *
 **/
2269

In KWin we do

/**
 *
 **/

so depending on user preferences it could avoid showing notifications when required.

So then then I have to keep track of what full screen window is on what screen and which notification got covered by a full screen window and re-show it once the full screen window goes away or is hidden or minimized or I enter showing desktop or..?

zzag added a comment.EditedWed, Apr 24, 2:43 PM

If you'd like to re-show suppressed notifications, then you would have to keep track of fullscreen clients in either case.

If you'd like re-show suppressed notifications, then you would have to keep track of fullscreen clients in either case.

Critical notifications stay on screen indefinitely. When KWin just raises them on top of full screen when that type is set I don't have to change any logic on my side. Otherwise I need to filter out based on screen and what not making a huge mess on my side...

zzag added a comment.Wed, Apr 24, 2:49 PM

and re-show it once the full screen window goes away

How does the proposed solution(i.e. in this patch) address this problem?

In D20629#455442, @zzag wrote:

How does the proposed solution(i.e. in this patch) address this problem?

When we change notifications to always show on top of full screen windows I need to keep track of that myself and hide and show notification popups depending on whether there's a fullscreen window here or not.

zzag added a comment.EditedWed, Apr 24, 3:00 PM

Alright, it seems like we have different views. Let's sort this out first.

Consider the following scenario:

  • A client enters fullscreen mode;
  • Notification A is shown;
  • Notification B is shown;
  • Notification A is hidden;
  • Notification B is hidden;
  • The client leaves fullscreen mode.

If both notification A and B are critical, they will be shown all the time (unless user dismisses them) above the client.

If both notifications have normal urgency, they will be below the client. So, after the client leaves fullscreen mode(i.e. after the last step), they both have to be re-shown, right? How does the proposed solution achieve that (without tracking fullscreen state)?

So, after the client leaves fullscreen mode(i.e. after the last step), they both have to be re-shown, right?

They will be in the history with a little unread notifications counter:

zzag added a comment.EditedWed, Apr 24, 3:11 PM

I proposed to swap Notifications and Active layers, and let plasmashell be in charge what notifications have to be displayed and when, i.e.

@@ -80,8 +80,8 @@ enum Layer {
     NormalLayer,
     DockLayer,
     AboveLayer,
-    NotificationLayer, // layer for windows of type notification
     ActiveLayer, // active fullscreen, or active dialog
+    NotificationLayer, // layer for windows of type notification
     OnScreenDisplayLayer, // layer for On Screen Display windows such as volume feedback
     UnmanagedLayer, // layer for override redirect windows.
     NumLayers // number of layers, must be last

I'm going to act as translator as you two are going in circles.

A client enters fullscreen mode;
Notification A is shown
Notification A expires
client leaves fullscreen

If we want to show notification A, we would have to track full screen.
But AFAIK we don't want that.

I assume Kai meant this case:

A client enters fullscreen mode;
Notification A is shown and it hasn't expired;
client leaves fullscreen

If we want to now show notifcation A we would now need to track fullscreen changes and show/hide as appropriate, whereas we don't with this approach.

I proposed to swap Notifications and Active layers, and let plasmashell be in charge what notifications have to be displayed and when, i.e.

That only works if you can assume no-one (ab)uses _NET_WM_WINDOW_TYPE_NOTIFICATION.

A quick grep of my source dir shows Chokoq and Amarok do.

So, how do we proceed here now? I still think it's KWin's task to manage windows (it's a window manager, after all, lol) and not have plasmashell care about what windows are on screen.

I don't think Vlad's idea is a bad idea, but given the issue with other clients - I think it's unworkable. Sorry.

If the concern is about introducing more layers, that's avoidable.
Whilst we need the new window type, CriticialNotification window types can be part of the OnScreenDisplayLayer.

That simplifies the newly added special casing somewhat.

zzag added a comment.Fri, Apr 26, 1:00 PM

I don't quite like that KWin needs to know about different notification urgency levels, that's pretty much it. Though I guess that's one of downsides of having desktop shell and compositor live in different processes, there have to be good communication between two.

In either case, new window type seems to be a sensible solution, so we can go with it.

broulik added a comment.EditedFri, Apr 26, 1:01 PM

I don't quite like that KWin needs to know about different notification urgency levels,

That's why I originally proposed using Notification + keepAbove.

zzag added a comment.Fri, Apr 26, 1:13 PM

Could you please address my inline comments?

broulik updated this revision to Diff 57038.Fri, Apr 26, 1:22 PM
  • Fixup doxygen comment
zzag accepted this revision.Fri, Apr 26, 2:03 PM
This revision is now accepted and ready to land.Fri, Apr 26, 2:03 PM
This revision was automatically updated to reflect the committed changes.