elementary / wingpanel

Stylish top panel that holds indicators and spawns an application launcher
https://elementary.io
GNU General Public License v3.0
137 stars 46 forks source link

Make the notifications move out of the way when menus are opened #481

Open stan-janssen opened 1 year ago

stan-janssen commented 1 year ago

We can move the notifications out of the way so that they no longer overlap with the indicator menus. (Fixes https://github.com/elementary/gala/issues/91). This goes together with this PR: https://github.com/elementary/gala/pull/1594.

Sometimes, actions in the indicator menus will trigger a notiication (like when (dis)connecting a network interface), and then the notification gets in the way.

This change allows the following behavior:

https://user-images.githubusercontent.com/12174852/228489641-111061bf-9f23-41b1-b8c9-ee9f512ac73b.mp4

A remaining detail that I'd like some guidance on is: the notifications stack is not moved when the size of the indicator changes after it was opened. This can happen with the Network indicator when it finds additional networks:

https://user-images.githubusercontent.com/12174852/228490330-bd3ef22c-6827-4ea6-8d8f-862697b965cc.mp4

I'd like to attach a signal listener or event listener to someting like an 'on_size_change' event that the widget might emit, but I couldn't find a clear way to do that. Perhaps someone has a good suggestion?

lenemter commented 1 year ago

@stan-janssen You can install https://github.com/vala-lang/vala-lint to check lint locally

stan-janssen commented 1 year ago

Yes. I also noticed on my computer that the Log Out hangs when selected from the menu (in works fine when using ctrl + alt + del). It encounters a timeout for the DBus message, not sure why.

Any ideas?

lenemter commented 1 year ago

@stan-janssen I was able to workaround Log Out bug by wrapping offset_notifications in Idle like this:

Idle.add (() => {
    offset_notifications ();

    return Source.REMOVE;
});
stan-janssen commented 1 year ago

What is the underlying problem and why does wrapping it in Idle () work? What does that do?

lenemter commented 1 year ago

What is the underlying problem

I don't know ¯\_(ツ)_/¯

What does that do?

Idle.add delays calling a function. From Valadoc: Adds a function to be called whenever there are no higher priority events pending.

lenemter commented 1 year ago

There is an issue: notifications are offset when applications menu is opened, which looks weird.

image

I found a 'lazy' solution for this. You can check a type of an opened indication using current_indicator.base_indicator.code_name. So you can check if indicator is app launcher or a datetime indicator: current_indicator.base_indicator.code_name != Indicator.APP_LAUNCHER && current_indicator.base_indicator.code_name != Indicator.DATETIME

@danirabbit How do we want to handle this?

stan-janssen commented 1 year ago

@stan-janssen I was able to workaround Log Out bug by wrapping offset_notifications in Idle like this:

Idle.add (() => {
    offset_notifications ();

    return Source.REMOVE;
});

I solved it in a new commit by making the function async and calling it with begin () and end (), which seems to be a more common pattern (at least from the elementary code that I encountered up to now).

stan-janssen commented 1 year ago

I found a 'lazy' solution for this. You can check a type of an opened indication using current_indicator.base_indicator.code_name. So you can check if indicator is app launcher or a datetime indicator: current_indicator.base_indicator.code_name != Indicator.APP_LAUNCHER && current_indicator.base_indicator.code_name != Indicator.DATETIME

@danirabbit How do we want to handle this?

I like this, at least for now, because it correctly mirrors the complexity of the situation we are dealing with, and nothing more. We are explicitly excluding these two popovers from consideration, so it makes sense to name them explicitly in an if clause.

I just added a commit that implements this.

I think that tackles the outright bugs, let’s wait and see what @danirabbit thinks and if we need more design iterations or anything.

Thanks so much for your constructive feedback! I've only just started contributing the elementary this week and it feels good.

stan-janssen commented 1 year ago

@danirabbit I'd love to get your opinion on this. If you like it, I can rebase it onto the current branch and we can ship it. I myself like the feature a lot, and would make working in that top-right corner of the OS more delightful.

I just encountered another great use case: attempting to do something in a Wingpanel menu shortly after changing the speaker volume.

Anyway, if you have a moment to take a look, I'd appreciate it. Thanks.

lenemter commented 1 year ago

@stan-janssen I looked into this and found out you can connect to popover.size_allocate signal to listen to size changes and then call offset_notification when it happens.