GNOME Bugzilla – Bug 723996
Add in app notifications
Last modified: 2014-04-03 18:26:57 UTC
Add in app notifications that match the current mockups. The first two patches are cleanups that were found during creation of the notification patch. The third patch is where the magic happens. Fourth patch is just a test patch to see it all in action.
Created attachment 268628 [details] [review] MainWindow: remove unnecessary GtkGrid Make the window-content object be a GtkOverlay instead of a GtkGrid and hence push the creation of the overlay to the UI-file.
Created attachment 268629 [details] [review] MainWindow: move the zoomcontrol to here Initialize the zoom control outside of the mapView, just like the searchpopup. As a side effect this removes the circular dependency between the map overlay and the map view.
Created attachment 268630 [details] [review] Add in app notifications This patch adds in app notifications. The code is derived from gtk-widget-factory. The Notification widget inherits from GtkRevealer and has a showNotification()- and a dismiss() method.
Created attachment 268631 [details] [review] Notification test patch With this applied, try pushing the location button or changing base layer.
Review of attachment 268629 [details] [review]: Looks good otherwise. ::: src/mapView.js @@ +91,2 @@ this.setMapType(MapType.STREET); One empty line to much, perhaps?
Review of attachment 268630 [details] [review]: Nice job! Some comments below. * Are we sure that the only two use-cases for the notification is without button or with? Or do we want fancier layouts? * Should we remove the rounding of the corners, since we do not have transparency it doesn't look all that good with the grey corners. ::: src/mainWindow.js @@ +102,1 @@ }, Does it really need to be created and added as an overlay each time? The number of children of our overlay just goes up and up since they are never removed. How about just creating it once, add it as an overlay just once. And then have a separate reveal and revealWithButton functions (or better names). Then the user of the notification can decide what they need. The 'revealWithButton' can take the callback function as argument. Then the dismiss function can take care of disconnecting the clicked signal. ::: src/notification.ui @@ +11,3 @@ + <property name="margin-start">10</property> + <property name="margin-end">10</property> + <property name="spacing">20</property> You like to use GtkBox, I like to use GtkGrid. Maybe we should decide what to use Maps-wide? :) I vote GtkGrid :) From GtkBox docs: "Note Note that a single-row or single-column GtkGrid provides exactly the same functionality as GtkBox." From GtkBox versus GtkGrid (https://developer.gnome.org/gtk3/stable/ch28s02.html) "When adding a child to a GtkBox, there are two hard-to-remember parameters (child properties, more exactly) named expand and fill that determine how the child size behaves in the main direction of the box. [...] GtkGrid does not have any custom child properties for controlling size allocation to children. Instead, it fully supports the newly introduced "hexpand", "vexpand", "halign" and "valign" properties." From GtkGrid docs: "GtkGrid is a container which arranges its child widgets in rows and columns. It is a very similar to GtkTable and GtkBox, but it consistently uses GtkWidget's "margin" and "expand" properties instead of custom child properties, and it fully supports height-for-width geometry management." What say you?
Review of attachment 268630 [details] [review]: Thanks! I'm not sure if we will want fancier layouts. I guess what we would do in that case is rip out the code that handles the button and and just make it possible to add a child or many children to the notification. Yeah, we should probably be consistent and remove the roundings.
Review of attachment 268630 [details] [review]: More code comments. ::: src/mainWindow.js @@ +102,1 @@ }, The widget is destroyed when dismissed (look inside the dismissed method in notifcation.js). Maybe it should be destroyed out here instead, that's probably a more flexible solution. Anyways, my first solution was just like that, one single notification that was mutated. However, that means that the app can never send more than one notification. With this solution the notifications are stacked and you can interact with them, one at a time. This shouldn't come up much, but I can still see it happen. ::: src/notification.js @@ +60,3 @@ + this.set_reveal_child(false); + Mainloop.timeout_add(this.transition_duration, (function() { + this.destroy(); Here it's destroyed. I used to emit a "dismissed" signal here and destroy the widget in MainWindow.showNotification instead. That's probably clearer and more flexible. Also not sure about the destroy, maybe just removing it from the overlay is enough and let gjs garbage collect it. ::: src/notification.ui @@ +11,3 @@ + <property name="margin-start">10</property> + <property name="margin-end">10</property> + <property name="spacing">20</property> I'm totally ok with moving to GtkGrid. I've always thought that GtkBox was a simpler version of GtkGrid for when the complexity of a grid isn't needed, but there seems to be no advantages (but some disadvantages) to GtkBox so let's go with a grid.
Created attachment 269167 [details] [review] Notifications: Add static notifications Add support for static notifications. These are pre-defined notifications that are cached and reused.
Created attachment 269168 [details] [review] Notifications: static example Two stubs for example static (reusable) notifications. Not meant to be pushed as is, but as a demonstration on how to work with this.
Created attachment 269169 [details] [review] Notification test patch With this applied, try pushing the location button or changing base layer.
Gah, git-bz-fail. Disregard the three posts above.
Created attachment 269170 [details] [review] Add in app notifications This patch adds in-app notifications. The code is derived from gtk-widget-factory. This patch and the following below is a rework of the previous patches following discussions with Jonas on G+
Created attachment 269171 [details] [review] Notifications: Add static notifications Add support for static notifications. These are pre-defined notifications that are cached and reused.
Created attachment 269172 [details] [review] Notifications: static example Two stubs for example static (reusable) notifications. Not meant to be pushed as is, but as a demonstration on how to work with this.
Created attachment 269173 [details] [review] Notification test patch With this applied, try pushing the location button or changing base layer.
Had also forgotten to rebase against master… I guess I'm a bit tired. Anyhow these new patches are (as mentined above) a rework of the previous patches following discussions with Jonas on G+.
Created attachment 269176 [details] [review] MainWindow: remove unnecessary GtkGrid Make the window-content object be a GtkOverlay instead of a GtkGrid and hence push the creation of the overlay to the UI-file.
Created attachment 269177 [details] [review] MainWindow: move the zoomcontrol to here Initialize the zoom control outside of the mapView, just like the searchpopup. As a side effect this removes the circular dependency between the map overlay and the map view.
Created attachment 269178 [details] [review] Add in app notifications This patch adds in-app notifications. The code is derived from gtk-widget-factory.
Created attachment 269179 [details] [review] Notifications: Add static notifications Add support for static notifications. These are pre-defined notifications that are cached and reused.
Created attachment 269180 [details] [review] Notifications: static example Two stubs for example static (reusable) notifications. Not meant to be pushed as is, but as a demonstration on how to work with this.
Created attachment 269181 [details] [review] Notification test patch With this applied, try pushing the location button or changing base layer.
Review of attachment 269176 [details] [review]: Looks fine, but the grid assignment should be removed in this patch and not in the zoomControl patch, no?
Review of attachment 269177 [details] [review]: Looks fine otherwise. ::: src/mainWindow.js @@ -63,3 @@ 'search-entry', 'search-completion']); - let grid = ui.windowContent; This should be removed in the previous patch.
Review of attachment 269178 [details] [review]: How about adding the NotificationManager class to notification.js? So we would have Notification.Notification and Notification.NotificationManager or maybe just Notification.Manager()? The approach we take no makes it so that we cannot issue notifications in the _init function of MainWindow since we need to pass the overlay to the NotificationManager. Looking at how Documents does this, they have a grid in the NotificationManager that notifications gets put in. And that gets added to the overlay. With that approach the Manager would not need to know about our windowContent overlay. It would make a difference UI-wise I guess, with notifications stacking downwards. It would be nice to not be forced to pass the overlay to the NotificationManager. But maybe there is no way around it? ::: src/notification.js @@ +56,3 @@ + // We only want to send a dismissed / shown -signal + // if there is an actual change in revealed state. + if(state !== this.child_revealed) { if () ::: src/notificationManager.js @@ +23,3 @@ +const Mainloop = imports.mainloop; +const Lang = imports.lang; +const Utils = imports.utils; Nit: Utils not actually used, right?
Review of attachment 269179 [details] [review]: At the moment Boxes, Documents and Contacts all use Gd.Notification. Is there work going on to have a new GtkWidget for Notifications? Like moving this out of Gd? Is the caching really needed? What is your thinking there? A bit more justification would me nice. Did you look at how Documents does this? With custom Notification classes for stuff like Print? Maybe that is a better approach? showNotification can take a notification class directly. Like NetworkNotification and LocationNotification.
(In reply to comment #27) > Review of attachment 269179 [details] [review]: > > At the moment Boxes, Documents and Contacts all use Gd.Notification. > Is there work going on to have a new GtkWidget for Notifications? Like moving > this out of Gd? I don't think so. Its just a simple widget in the end so nobody bothers much. I plan to just create it manually in Boxes and drop use of Gd.Notification.
Review of attachment 269178 [details] [review]: ::: src/notification.js @@ +56,3 @@ + // We only want to send a dismissed / shown -signal + // if there is an actual change in revealed state. + if(state !== this.child_revealed) { Did you miss something here? :) ::: src/notificationManager.js @@ +23,3 @@ +const Mainloop = imports.mainloop; +const Lang = imports.lang; +const Utils = imports.utils; Ah true!
Review of attachment 269177 [details] [review]: ::: src/mainWindow.js @@ -63,3 @@ 'search-entry', 'search-completion']); - let grid = ui.windowContent; Yep. A rebase fail. :) Will fix before committing.
(In reply to comment #26) > Review of attachment 269178 [details] [review]: > > How about adding the NotificationManager class to notification.js? So we would > have Notification.Notification and Notification.NotificationManager or maybe > just Notification.Manager()? Sure! That sounds better actually. > The approach we take no makes it so that we cannot issue notifications in the > _init function of MainWindow since we need to pass the overlay to the > NotificationManager. > Looking at how Documents does this, they have a grid in the NotificationManager > that notifications gets put in. And that gets added to the overlay. With that > approach the Manager would not need to know about our windowContent overlay. It > would make a difference UI-wise I guess, with notifications stacking downwards. We can't really do that with a grid because of the problem with transparency over GtkClutterEmbed. I see two options: 1) keep a list of "waiting" notifications in notificationManager and add a setOverlay()-method that when called will show all those "waiting" notifications then. 2) Create the overlay in application.js and pass it to mainWindow. Of these two solutions I like #2 best. > It would be nice to not be forced to pass the overlay to the > NotificationManager. But maybe there is no way around it? It needs a reference to it some way or another. We could make a subclass of overlay called MapOverlay or so and let that have the notification stuff in it instead (and also the zoomcontrol). But the difference is not huge, you'll have a reference to the overlay either way, be it via "this" or via "this._overlay".
Attachment 269176 [details] pushed as 08092f5 - MainWindow: remove unnecessary GtkGrid Attachment 269177 [details] pushed as 84c5398 - MainWindow: move the zoomcontrol to here
(In reply to comment #27) > Review of attachment 269179 [details] [review]: > > At the moment Boxes, Documents and Contacts all use Gd.Notification. > Is there work going on to have a new GtkWidget for Notifications? Like moving > this out of Gd? What Zeeshan said and also rolling our "own" (read: copy-pasting the one Matthias Clasen did), makes us not depend on libgd which I think is a good thing. > Is the caching really needed? What is your thinking there? A bit more > justification would me nice. It isn't needed per se, but it brings two small benefits: 1) we don't need to spend cycles recreating notifications (very small benefit). 2) it makes it very easy to avoid duplicate notifications (this is pretty nice). > Did you look at how Documents does this? With custom Notification classes for > stuff like Print? Didn't look at it until now, but yeah the solutions are very similar. Take a look at https://bugzilla.gnome.org/attachment.cgi?id=269180 to see an example of custom notifications. There are stub implementations of NO_NETWORK- and NO_LOCATION notifications. > Maybe that is a better approach? showNotification can take a notification class > directly. Like NetworkNotification and LocationNotification. It kind of does. The Notification.Type is supposed to be a tuple {name, Class}, the name is needed to be able to put a key inside the _cache object. If we decided not to cache I think the name-field is unnecessary and we could let the Type-object be something like: Type = { NO_LOCATION: NoLocation, NO_NETWORK: NoNetwork } ...or just let people pass raw Notification class objects to showNotification, or even let them create the notifications themselves and let them pass in the instances. However... I think reason #2 for keeping caching is warranted.
Err. Like this instead: ...The Notification.Type is supposed to be a /bunch/ of tuples {name, Class}... Example from the code: const Type = { NO_NETWORK : { name: "NO_NETWORK", Class: NoNetwork }, NO_LOCATION : { name: "NO_LOCATION", Class: NoLocation } };
So my suggestion is that I just make the following changes: 1) move NotificationManager to notification.js and rename it Manager so you would do new Notification.Manager(overlay) 2) move the creation of the overlay to Application.vfunc_activate and pass that to Notification.Manager and MainWindow.MainWindow What do you think? I will move to working on the routing now and come back to this when I get more feedback. :) Thanks for the reviews again!
Review of attachment 269178 [details] [review]: ::: src/notification.js @@ +56,3 @@ + // We only want to send a dismissed / shown -signal + // if there is an actual change in revealed state. + if(state !== this.child_revealed) { Did I? I just wanted a space added between the if and the parenthesis. :)
(In reply to comment #31) > We can't really do that with a grid because of the problem with transparency > over GtkClutterEmbed. > I see two options: > 1) keep a list of "waiting" notifications in notificationManager and add a > setOverlay()-method that when called will show all those "waiting" > notifications then. > 2) Create the overlay in application.js and pass it to mainWindow. > > Of these two solutions I like #2 best. > Well, it could be done, if the grid is above the GtkClutterEmbed, under the searchBar and drops down over it, it need not transparency then if we have no rounded corners. But then the stacking might be an issue. We could look into it later if we feel like it. > > It would be nice to not be forced to pass the overlay to the > > NotificationManager. But maybe there is no way around it? > > It needs a reference to it some way or another. We could make a subclass of > overlay called MapOverlay or so and let that have the notification stuff in it > instead (and also the zoomcontrol). > But the difference is not huge, you'll have a reference to the overlay either > way, be it via "this" or via "this._overlay". Yeah, agreed.
(In reply to comment #33) . > > It kind of does. The Notification.Type is supposed to be a tuple {name, Class}, > the name is needed to be able to put a key inside the _cache object. If we > decided not to cache I think the name-field is unnecessary and we could let the > Type-object be something like: > > Type = { > NO_LOCATION: NoLocation, > NO_NETWORK: NoNetwork > } > > ...or just let people pass raw Notification class objects to showNotification, > or even let them create the notifications themselves and let them pass in the > instances. > > However... I think reason #2 for keeping caching is warranted. Ok, fair enough, so, just so I'm following. What makes it avoid duplicate notifications is that it's the same object added to the overlay? If we call showNotification twice with same argument?
(In reply to comment #35) > So my suggestion is that I just make the following changes: > 1) move NotificationManager to notification.js and rename it Manager so you > would do new Notification.Manager(overlay) > 2) move the creation of the overlay to Application.vfunc_activate and pass that > to Notification.Manager and MainWindow.MainWindow > > What do you think? > 1) I like it, but I wonder if it's the way to do it. Or if the convention is to name it Notification.NotificationManager? 2) Yeah, sounds fine. > I will move to working on the routing now and come back to this when I get more > feedback. :) > Sounds nice! Look forward to the routing! > Thanks for the reviews again! Thanks for the patches!
Review of attachment 269178 [details] [review]: ::: src/notification.js @@ +56,3 @@ + // We only want to send a dismissed / shown -signal + // if there is an actual change in revealed state. + if(state !== this.child_revealed) { Oh ok! :D
(In reply to comment #37) > (In reply to comment #31) > > Well, it could be done, if the grid is above the GtkClutterEmbed, under the > searchBar and drops down over it, it need not transparency then if we have no > rounded corners. But then the stacking might be an issue. We could look into it > later if we feel like it. I'm not sure I'm following, we should have a chat on G+, IRC or over the phone about this I think. But are you ok with revisiting this later? > > > It would be nice to not be forced to pass the overlay to the > > > NotificationManager. But maybe there is no way around it? > > > > It needs a reference to it some way or another. We could make a subclass of > > overlay called MapOverlay or so and let that have the notification stuff in it > > instead (and also the zoomcontrol). > > But the difference is not huge, you'll have a reference to the overlay either > > way, be it via "this" or via "this._overlay". > > Yeah, agreed. Cool.
(In reply to comment #38) > (In reply to comment #33) > . > > > > It kind of does. The Notification.Type is supposed to be a tuple {name, Class}, > > the name is needed to be able to put a key inside the _cache object. If we > > decided not to cache I think the name-field is unnecessary and we could let the > > Type-object be something like: > > > > Type = { > > NO_LOCATION: NoLocation, > > NO_NETWORK: NoNetwork > > } > > > > ...or just let people pass raw Notification class objects to showNotification, > > or even let them create the notifications themselves and let them pass in the > > instances. > > > > However... I think reason #2 for keeping caching is warranted. > > Ok, fair enough, so, just so I'm following. What makes it avoid duplicate > notifications is that it's the same object added to the overlay? If we call > showNotification twice with same argument? Yeah exactly. You'd call showNotification with some member of Notification.Type telling what notification to show. And if that notification has a parent widget (ie is currently shown) we just don't add it (it would fail with an error if we did).
(In reply to comment #39) > (In reply to comment #35) > > So my suggestion is that I just make the following changes: > > 1) move NotificationManager to notification.js and rename it Manager so you > > would do new Notification.Manager(overlay) > > 2) move the creation of the overlay to Application.vfunc_activate and pass that > > to Notification.Manager and MainWindow.MainWindow > > > > What do you think? > > > > 1) I like it, but I wonder if it's the way to do it. Or if the convention is to > name it Notification.NotificationManager? Zeeshan, any thoughts here? Notification.Manager, Notification.NotificationManager or NotificationManager.NotificationManager? > 2) Yeah, sounds fine. Cool. Nice, I'll put this together tonight.
(In reply to comment #43) > (In reply to comment #39) > > (In reply to comment #35) > > > So my suggestion is that I just make the following changes: > > > 1) move NotificationManager to notification.js and rename it Manager so you > > > would do new Notification.Manager(overlay) > > > 2) move the creation of the overlay to Application.vfunc_activate and pass that > > > to Notification.Manager and MainWindow.MainWindow > > > > > > What do you think? > > > > > > > 1) I like it, but I wonder if it's the way to do it. Or if the convention is to > > name it Notification.NotificationManager? > > Zeeshan, any thoughts here? Notification.Manager, > Notification.NotificationManager or NotificationManager.NotificationManager? No, lets just get this done ASAP as we are already in freeze. :) BTW, please don't push these changes before getting permission from r-t (and i think from docs team as well).
(In reply to comment #44) > No, lets just get this done ASAP as we are already in freeze. :) Ok :) > BTW, please don't push these changes before getting permission from r-t (and i > think from docs team as well). Sure!
Created attachment 270083 [details] [review] Add in app notifications This patch adds in-app notifications. The code is derived from gtk-widget-factory.
Created attachment 270084 [details] [review] Notifications: Add static notifications Add support for static notifications. These are pre-defined notifications that are cached and reused.
Created attachment 270085 [details] [review] Notifications: static example Two stubs for example static (reusable) notifications. Not meant to be pushed as is, but as a demonstration on how to work with this.
Created attachment 270086 [details] [review] Notification test patch With this applied, try pushing the location button or changing base layer.
These patches should fix all comments I think. :)
Review of attachment 270083 [details] [review]: Looks fine otherwise. ::: src/notification.js @@ +56,3 @@ + // We only want to send a dismissed / shown -signal + // if there is an actual change in revealed state. + if(state !== this.child_revealed) { I still want this as if (state ..., i.e. add a space after if.
Review of attachment 270084 [details] [review]: ::: src/notification.js @@ +82,3 @@ +const Type = { }; + Nit: the cache does not have a space between the curlies.
We still need a freeze exception if we want this. Could you write one? I know we have discussed a "better" solution for this, but right now it's the exception that counts. If you post an improved series I'll review it quickly. And also, this series is not important without the routing series, right? Since atm we do not have anything to notify in app.
Attachment 270083 [details] pushed as 531c441 - Add in app notifications Attachment 270084 [details] pushed as ddba7aa - Notifications: Add static notifications
Jonas: Should we close this or do you want to keep it open for a potentially cleaner patch later?
Let's close it.