GNOME Bugzilla – Bug 763876
notificationbar: Remove old notifications
Last modified: 2016-09-20 08:15:55 UTC
See patches.
Created attachment 324281 [details] [review] notificationbar: Remove old notifications Right now, the notification tray can hold an unlimited number of notifications which leads to a very crowded user interface. When trying to use a broken URL it throws too many warnings if Enter or Continue button is pressed repeatedly. In order to fix this limit the maximum number of warnings shown in a tray by dismissing the old ones.
Review of attachment 324281 [details] [review]: The patch is specific to warning/error notifications while the shortlog and description here seems to suggest it's not. I think for errors/warnings, this is likely a good idea so I guess it's good that it's specific to error messages but commit log should be clear about that. ::: src/notificationbar.vala @@ +4,3 @@ private class Boxes.Notificationbar: Gtk.Grid { public const int DEFAULT_TIMEOUT = 6; + private const int MAX_NUMBER_OF_NOTIFICATIONS = 2; * This should have ERROR in the name. * While I prefer descriptive names, brevity is also a goal so you want to choose he shortest name that would be clear enough. In this case I'd go for: MAX_ERROR_NOTIFICATIONS. As a side-note, "num" is a well-known shortcut for 'number_of' in progromming world. @@ +74,2 @@ public Gd.Notification display_error (string message, int timeout = DEFAULT_TIMEOUT) { + uint notifications_length; No need for separate declarations in Vala. Just use 'var' to declare it when you initialize below. Although since it's not used more than once, I'd just not use a variable here at all. @@ +75,3 @@ + uint notifications_length; + + notifications_length = active_notifications.length(); Coding-style nitpick: Space before (). @@ +77,3 @@ + notifications_length = active_notifications.length(); + if (notifications_length > MAX_NUMBER_OF_NOTIFICATIONS) { + Widget notification_to_remove; * Same comment about forward declaration. In this case you'd want something like: var notification = active_notifications.nth_data (notifications_length - 1) as Gd.Notification. * Just 'notification' would suffice as name here. @@ +80,3 @@ + + notification_to_remove = active_notifications.nth_data (notifications_length - 1); + ((Gd.Notification) notification_to_remove).dismiss (); Prefer 'as' keyword for casting.
(In reply to Zeeshan Ali (Khattak) from comment #2) > Review of attachment 324281 [details] [review] [review]: > > The patch is specific to warning/error notifications while the shortlog and > description here seems to suggest it's not. I think for errors/warnings, > this is likely a good idea so I guess it's good that it's specific to error > messages but commit log should be clear about that. > > ::: src/notificationbar.vala > @@ +4,3 @@ > private class Boxes.Notificationbar: Gtk.Grid { > public const int DEFAULT_TIMEOUT = 6; > + private const int MAX_NUMBER_OF_NOTIFICATIONS = 2; > > * This should have ERROR in the name. > > * While I prefer descriptive names, brevity is also a goal so you want to > choose he shortest name that would be clear enough. In this case I'd go for: > MAX_ERROR_NOTIFICATIONS. As a side-note, "num" is a well-known shortcut for > 'number_of' in progromming world. > > @@ +74,2 @@ > public Gd.Notification display_error (string message, int timeout = > DEFAULT_TIMEOUT) { > + uint notifications_length; > > No need for separate declarations in Vala. Just use 'var' to declare it when > you initialize below. Although since it's not used more than once, I'd just > not use a variable here at all. > > @@ +75,3 @@ > + uint notifications_length; > + > + notifications_length = active_notifications.length(); > > Coding-style nitpick: Space before (). > > @@ +77,3 @@ > + notifications_length = active_notifications.length(); > + if (notifications_length > MAX_NUMBER_OF_NOTIFICATIONS) { > + Widget notification_to_remove; > > * Same comment about forward declaration. In this case you'd want something > like: > > var notification = active_notifications.nth_data (notifications_length - 1) > as Gd.Notification. > > * Just 'notification' would suffice as name here. > > @@ +80,3 @@ > + > + notification_to_remove = active_notifications.nth_data > (notifications_length - 1); > + ((Gd.Notification) notification_to_remove).dismiss (); > > Prefer 'as' keyword for casting. > + uint notifications_length; > + > + notifications_length = active_notifications.length(); > I used a variable for the length of the notifications list because Vala resembles C# from my point of view and I know that when you use a function/procedure in a loop it is called again every time and I wanted to avoid that. So the main reason was the time complexity. Should I give up on it?
(In reply to Radu Stochitoiu from comment #3) > (In reply to Zeeshan Ali (Khattak) from comment #2) > > Review of attachment 324281 [details] [review] [review] [review]: > > > > The patch is specific to warning/error notifications while the shortlog and > > description here seems to suggest it's not. I think for errors/warnings, > > this is likely a good idea so I guess it's good that it's specific to error > > messages but commit log should be clear about that. > > > > ::: src/notificationbar.vala > > @@ +4,3 @@ > > private class Boxes.Notificationbar: Gtk.Grid { > > public const int DEFAULT_TIMEOUT = 6; > > + private const int MAX_NUMBER_OF_NOTIFICATIONS = 2; > > > > * This should have ERROR in the name. > > > > * While I prefer descriptive names, brevity is also a goal so you want to > > choose he shortest name that would be clear enough. In this case I'd go for: > > MAX_ERROR_NOTIFICATIONS. As a side-note, "num" is a well-known shortcut for > > 'number_of' in progromming world. > > > > @@ +74,2 @@ > > public Gd.Notification display_error (string message, int timeout = > > DEFAULT_TIMEOUT) { > > + uint notifications_length; > > > > No need for separate declarations in Vala. Just use 'var' to declare it when > > you initialize below. Although since it's not used more than once, I'd just > > not use a variable here at all. > > > > @@ +75,3 @@ > > + uint notifications_length; > > + > > + notifications_length = active_notifications.length(); > > > > Coding-style nitpick: Space before (). > > > > @@ +77,3 @@ > > + notifications_length = active_notifications.length(); > > + if (notifications_length > MAX_NUMBER_OF_NOTIFICATIONS) { > > + Widget notification_to_remove; > > > > * Same comment about forward declaration. In this case you'd want something > > like: > > > > var notification = active_notifications.nth_data (notifications_length - 1) > > as Gd.Notification. > > > > * Just 'notification' would suffice as name here. > > > > @@ +80,3 @@ > > + > > + notification_to_remove = active_notifications.nth_data > > (notifications_length - 1); > > + ((Gd.Notification) notification_to_remove).dismiss (); > > > > Prefer 'as' keyword for casting. > > > + uint notifications_length; > > + > > + notifications_length = active_notifications.length(); > > > > I used a variable for the length of the notifications list because Vala > resembles C# from my point of view and I know that when you use a > function/procedure in a loop it is called again every time and I wanted to > avoid that. So the main reason was the time complexity. Should I give up on > it? Yeah, I use it only once. Changed the first behaviour so I forgot about it. Thanks!
(In reply to Radu Stochitoiu from comment #3) > (In reply to Zeeshan Ali (Khattak) from comment #2) > > Review of attachment 324281 [details] [review] [review] [review]: > > > > + uint notifications_length; > > + > > + notifications_length = active_notifications.length(); > > > > I used a variable for the length of the notifications list because Vala > resembles C# from my point of view and I know that when you use a > function/procedure in a loop it is called again every time and I wanted to > avoid that. So the main reason was the time complexity. Should I give up on > it? Hmm.. currently in your patch, it's not used in a loop but just a one-off 'if' condition so that point doesn't apply.
Created attachment 324499 [details] [review] notificationbar: Limit num. of simultaneous notifications Right now the warning/error notification tray can hold an unlimited number of notifications which leads to a very crowded user interface. When trying to use a broken URL it throws too many warnings if Enter or Continue button are pressed repeatedly. In order to fix this limit the maximum number of warnings/errors shown in a tray by dismissing the old ones.
Comment on attachment 324281 [details] [review] notificationbar: Remove old notifications In future, please remember to obsolete patches when (or after) submitting re-worked version of them. If you use git-bz, you'll find -e option of `git-bz attach` very useful in this regard. If the subject/summary line of patch remains the same, `git-bz attach` will automatically do this for you but many times after reviews you have to change the summary.
Review of attachment 324499 [details] [review]: Looks good otherwise and I was almost about to push it with minor changes but I realized a rather big issue. ::: src/notificationbar.vala @@ +74,2 @@ public Gd.Notification display_error (string message, int timeout = DEFAULT_TIMEOUT) { + if (active_notifications.length () > MAX_ERROR_NOTIFICATIONS) { * With check for '>', the max notifications displayed at once are going to be actually 3 so the constant's name and value aren't consistent. I'd rather define the constant to be '3' and check for >=. * while the patch is intented for only error messages, we're still checking for number of ALL notifications and dismissing them w/o checking if they are error notifications or not. Thinking about it, i don't see a clean and easy way to implement this only for error notifications. I know this goes against what I previously said but how about we limit all notifications (and hence do it from display() method instead) and have a bigger limit, e.g 5 or 6. @@ +77,3 @@ + + notification.dismiss (); + active_notifications.remove (notification); If you check the code in display() method, it ensures that notification is removed from list on being dismissed.
(In reply to Zeeshan Ali (Khattak) from comment #8) > > If you check the code in display() method, it ensures that notification is > removed from list on being dismissed. I modified only the display function and it's working properly without forcefully removing the notifications, with only one exception. If Enter is pressed too fast repeatedly the dismissed signal is sent slower than the input so the notification tray still increases. This would be the only change to the display function: if(active_notifications.length () > MAX_NOTIFICATIONS_NUM) { var notif = active_notif.nth_data (active_notifications.length () - 1) as Gd.Notification; notification2.dismiss (); // active_notifications.remove (notification2); } Forcefully removing it by uncommenting that line works but I know it's not how you want it to be.
(In reply to Radu Stochitoiu from comment #9) > if(active_notifications.length () > MAX_NOTIFICATIONS_NUM) { if(active_notifications.length () >= MAX_NOTIFICATIONS_NUM) { actually.
I know it's a bit difficult to get use to it, but please try your best to always use the 'Review' link to reply to inline comments. (In reply to Radu Stochitoiu from comment #9) > (In reply to Zeeshan Ali (Khattak) from comment #8) > > > > If you check the code in display() method, it ensures that notification is > > removed from list on being dismissed. > > > I modified only the display function and it's working properly without > forcefully removing the notifications, with only one exception. If Enter is > pressed too fast repeatedly the dismissed signal is sent slower than the > input so the notification tray still increases. 1. I don't see how you won't have the same issue if you only modify the non-generic method. 2. It's not that big a deal if this happens and it's very unlikely to happen any way (why would user repeatedly hit enter very fast?).
Created attachment 327895 [details] [review] Limit num. of simultaneous notifications Right now the notification tray can hold an unlimited number of notifications which leads to a very crowded user interface. When trying to use a broken URL it throws too many warnings if Enter or Continue are pressed repeatedly. In order to fix this make a limit of maximum number of notifications at once in the tray and dismiss the old ones.
Review of attachment 327895 [details] [review]: ::: src/notificationbar.vala @@ -4,3 @@ private class Boxes.Notificationbar: Gtk.Grid { public const int DEFAULT_TIMEOUT = 6; - private const int MAX_ERROR_NOTIFICATIONS = 2; This is obviously a patch on top of the patch, that was supposed to be obsoleted by this patch. Please squash them before uploading.
Created attachment 328467 [details] [review] notificationbar: Limit number of notifications Right now the notification tray can hold an unlimited number of notifications which leads to a very crowded user interface. When trying to use a broken URL it throws too many warnings if Enter or Continue are pressed repeatedly. In order to fix this make a limit of maximum number of notifications at once in the tray and dismiss the old ones.
Review of attachment 328467 [details] [review]: ::: src/notificationbar.vala @@ +105,3 @@ + var last_notification = active_notifications.nth_data (active_notifications.length () - 1) as Gd.Notification; + + last_notification.dismiss (); Same thing with this patch. This is assuming the previous patch, while it's supposed to include that. Squash them please.
How is it assuming the previous patch? I've commited a patch written from scratch, not using an old one.
(In reply to Radu Stochitoiu from comment #16) > How is it assuming the previous patch? I've commited a patch written from > scratch, not using an old one. Ah, my bad. Applies cleanly on git master.
(In reply to Zeeshan Ali (Khattak) from comment #17) > (In reply to Radu Stochitoiu from comment #16) > > How is it assuming the previous patch? I've commited a patch written from > > scratch, not using an old one. > > Ah, my bad. Applies cleanly on git master. So is it necessary to attach it again?
Review of attachment 328467 [details] [review]: I'll fix while pushing. ::: src/notificationbar.vala @@ +103,3 @@ + if (active_notifications.length () >= MAX_NOTIFICATIONS) { + var last_notification = active_notifications.nth_data (active_notifications.length () - 1) as Gd.Notification; Exceeds 120 chars.
Created attachment 330222 [details] [review] notificationbar: Limit concurrent notifications to 5 Notification tray can currently hold an unlimited number of notifications at once, which can lead to a very crowded UI. E.g when trying to formulate a URL in URL entry but ending up entering wrong URLs repeatedly and very quickly. Let's limit the number of concurrent notifications to 5. --- Besides commit log and fitting a line to 80 chars, I also moved the dismissal of old notification to, before prepending new one instead of after.
Created attachment 330223 [details] [review] notificationbar: Limit concurrent notifications to 5 Actually I realized how to really limit notifications to 5, no matter how fast you can press Enter. :)
(In reply to Zeeshan Ali (Khattak) from comment #22) > Created attachment 330223 [details] [review] [review] > notificationbar: Limit concurrent notifications to 5 > > Actually I realized how to really limit notifications to 5, no matter how > fast you can press Enter. :) How?
(In reply to Radu Stochitoiu from comment #23) > (In reply to Zeeshan Ali (Khattak) from comment #22) > > Created attachment 330223 [details] [review] [review] [review] > > notificationbar: Limit concurrent notifications to 5 > > > > Actually I realized how to really limit notifications to 5, no matter how > > fast you can press Enter. :) > > How? See the patch? :)
(In reply to Zeeshan Ali (Khattak) from comment #24) > > See the patch? :) Cool! xD Even if there were more than one left behind, I never thought that there could be more than one left behind. :))