Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

stan-janssen
Copy link

@stan-janssen stan-janssen commented Mar 29, 2023

We can move the notifications out of the way so that they no longer overlap with the indicator menus. (Fixes elementary/gala#91). This goes together with this PR: elementary/gala#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:

notifications-move-away.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:

notifications-offset-does-not-resize.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
Copy link
Member

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

@stan-janssen stan-janssen force-pushed the notifications-avoid-indicator-menus branch from 7bc16d8 to 70fb61d Compare March 29, 2023 10:03
lenemter

This comment was marked as outdated.

@stan-janssen
Copy link
Author

stan-janssen commented Mar 29, 2023

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?

@stan-janssen stan-janssen marked this pull request as draft March 29, 2023 10:15
@lenemter
Copy link
Member

@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
Copy link
Author

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

@lenemter
Copy link
Member

lenemter commented Mar 29, 2023

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

This comment was marked as outdated.

This avoids a deadlock when selecting the Log Out option.
@stan-janssen
Copy link
Author

stan-janssen commented Mar 29, 2023

@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
Copy link
Author

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 stan-janssen marked this pull request as ready for review May 27, 2023 20:46
@stan-janssen
Copy link
Author

@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
Copy link
Member

@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.

@lenemter lenemter mentioned this pull request Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move notifications when indicators are open
2 participants