search

elementary / wingpanel-indicator-notifications

Wingpanel Notifications Indicator
GNU Lesser General Public License v2.1
24 stars 12 forks source link

Support buttons with action group #263

Closed leolost2605 closed 1 year ago

leolost2605 commented 1 year ago
leolost2605 commented 1 year ago

Sorry @Marukesu I thought I hadn't requested a review yet

Marukesu commented 1 year ago

I don't think this depend on the Session removal…

Thinking better, i take this back.

The session file store notifications by id, ids aren't meant to be persistent and are reused by the server after a restart, that makes the indicator unreliable to deal with removing/closing/activating a notifications because of colliding ids.

the fact this doesn't cause much problems currently is because the indicator don't report back notification closing, neither support action activation of any type (the current default action code is broken). but once this get merged theses issues will start to be recurrent.

leolost2605 commented 1 year ago

Notifications are now stored by an internal id consisting of timestamp.server_id. If we load a notification from the session file the server_id will be 0 and no signals will be emitted. If actions exist the buttons will be insensitive (I thought about not storing actions at all but i think this might be clearer as otherwise we might end up with a situation like "Do you want to allow this?" and you've got nothing to click on. Another solution might be to not store notifications with actions at all 🤷) Requires elementary/notifications#211

Marukesu commented 1 year ago

If actions exist the buttons will be insensitive (I thought about not storing actions at all but i think this might be clearer as otherwise we might end up with a situation like "Do you want to allow this?" and you've got nothing to click on.

i like the idea of the deactivated buttons, i think you could drop the actions but not the label from the session file, and simply don't try to validate anything when restoring. that way we don't risk ending re-enabling a button because the new notification for the same id has a matching action name.

leolost2605 commented 1 year ago

i like the idea of the deactivated buttons, i think you could drop the actions but not the label from the session file, and simply don't try to validate anything when restoring. that way we don't risk ending re-enabling a button because the new notification for the same id has a matching action name.

The server id will be 0 and therefore the action always invalid so AFAIK there is no way we could re-enable it by accident. If you want me to do it anyway because of a bit less space used I'd be happy to update it but it'd require quite a bit more code because without we only have to handle a single way buttons are given and I don't think it's really worth it 🤷

danirabbit commented 1 year ago

:tada: amazing love it