create a built-in Notifications module for ashell #456
Conversation
… notification handling
…at, timestamps, and display limits
…tification handling
…aemon and local state
|
Notification dismiss/Clear buttons don’t actually remove entries from the notification service/daemon. Why did you disable the DBus signals??
still shows the last empty line 
|
Thanks! I hadn't noticed that before. I'm working on fixing those issues now. |
…ndling in notifications
MalpenZibo
left a comment
MalpenZibo
left a comment
There was a problem hiding this comment.
nice addition!! Thanks. There's some work to do, but it could be a great module.
I left a preliminary review with my main concern
| let _ = NotificationDaemon::close_notification_by_id(id).await; | ||
| }); | ||
| // Remove from local state immediately for responsive UI | ||
| self.notifications.remove(&id); |
There was a problem hiding this comment.
if we return a task to perform the async operation we could also place this code in the task "callback"
| Message::ClearNotifications => { | ||
| // Close each notification through the daemon (cleans up both daemon and global state) | ||
| let notification_ids: Vec<u32> = self.notifications.keys().copied().collect(); | ||
| tokio::spawn(async move { |
| } | ||
|
|
||
| pub fn menu_view<'a>(&'a self, _id: Id, theme: &'a AshellTheme) -> Element<'a, Message> { | ||
| let mut notifications_data: Vec<_> = self.notifications.values().cloned().collect(); |
There was a problem hiding this comment.
there isn't a way to avoid this clone? Maybe we could directly save in the state the vector
| let mut notifications_data: Vec<_> = self.notifications.values().cloned().collect(); | ||
|
|
||
| // Sort by timestamp (newest first) and limit | ||
| notifications_data.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)); |
There was a problem hiding this comment.
time consuming operation should go in the update method if it's possible
There was a problem hiding this comment.
moved to the update
| }; | ||
|
|
||
| // Global static for storing notifications - accessed by service layer | ||
| pub static NOTIFICATIONS: std::sync::OnceLock<std::sync::Mutex<HashMap<u32, Notification>>> = |
There was a problem hiding this comment.
Maybe we could avoid this static global object. I haven't checked all the code, but if I understand correctly, you should take inspiration from the tray service
There was a problem hiding this comment.
this has been replaced
…d theme parameters for improved flexibility
…or improved flexibility
…ving code clarity
|
Sorry, it's a big change, so I will not merge it into the next release. Anyway, the toast surface makes sense. The problem right now is that iced takes some time to create and correctly render a new surface (taking into consideration the HiDPI monitor). For this reason, the menu surface is always open. You are "correctly" creating a new surface to show a notification when necessary. I need to check if this solution will work. After the release, I will update the iced library to the latest version to see if this behavior has been improved. |
… improved clarity and efficiency
|
Ok here I am! Sorry for the delay. 
First, the layout, I'd like to have a bigger icon in the first column. Then it could be ok for now, but I'd like to have an X button on the top right to close the notification and reserve the click on notification for an optional call to action Scaling issue
Sadly, as I thought, we have the same scaling issue that I had with the menu. On a HiDPI monitor, the time between the first frame and the point where iced fixes the scaling of the surface is not trivial. I will try with the new version of iced to see if this issue is resolved:
|
|
Ok, great news. The new version of iced seems to fix this issue. This means that we can leave the surface creation logic as it is here and, in the future, resolve the menu surface problem. That's great news, we could also simplify that |
|
I will improve this |
|
There is an issue with the toast layer's dimensions: it's blocking mouse clicks on the underlying window. Implementing click-through functionality or resizable layer would solve this. |
|
ok, so let's block this PR until the update of iced is merged. So we can easily open and close the notification surface |
|
I had a look at the branch at the current state, I think it looks fine for a first merge, so people can use it and give feedback. |
|
Yes! Now that we've merged the iced update the main blocker here is gone and we can merge it. @Yoimiya-Naganohara are you able to fix the conflict? |
Yoimiya-Naganohara
force-pushed
the
notifications
branch
from
0b840aa to
6ba816f
Compare
|
I double-checked the code. There are a few things that I would like to change to make everything more in line with the rest of the codebase. |
|
Nevermind! There are some things that I prefer to fix by myself! I will merge this PR as soon as I can. |
|
I’ve been working on other things recently, so I’ve just resolved the conflicts. |
surely |
4 participants
This pull request adds a new notifications module to the application, integrating notifications into the app's configuration, UI, and event handling. The changes include new configuration options, menu integration, message handling, and icon support for notifications.
Notifications Module Integration:
Notificationsmodule, includingNotificationsModuleConfigwith customizable options such as format, timestamps, maximum notifications, body display, and view mode. (src/config.rs,src/modules/notifications.rs) [1] [2]Notificationsin the mainAppstruct, message enum, initialization, update, and event handling logic, ensuring the module is properly initialized and updated with configuration reloads. (src/app.rs) [1] [2] [3] [4] [5] [6] [7]src/menu.rs,src/app.rs,src/modules/mod.rs) [1] [2] [3]Configuration and Subscription Support:
Configstruct and default implementation to includenotificationsconfiguration, and updated module name parsing to recognizeNotifications. (src/config.rs) [1] [2] [3] [4]src/app.rs,src/modules/mod.rs) [1] [2]Icon Support:
BellandBellBadge) to be used in the UI. (src/components/icons.rs) [1] [2]