After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723996 - Add in app notifications
Add in app notifications
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-10 06:05 UTC by Mattias Bengtsson
Modified: 2014-04-03 18:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
MainWindow: remove unnecessary GtkGrid (2.28 KB, patch)
2014-02-10 06:05 UTC, Mattias Bengtsson
none Details | Review
MainWindow: move the zoomcontrol to here (2.48 KB, patch)
2014-02-10 06:05 UTC, Mattias Bengtsson
none Details | Review
Add in app notifications (7.83 KB, patch)
2014-02-10 06:05 UTC, Mattias Bengtsson
reviewed Details | Review
Notification test patch (1.37 KB, patch)
2014-02-10 06:05 UTC, Mattias Bengtsson
none Details | Review
Notifications: Add static notifications (2.19 KB, patch)
2014-02-15 06:16 UTC, Mattias Bengtsson
none Details | Review
Notifications: static example (2.25 KB, patch)
2014-02-15 06:16 UTC, Mattias Bengtsson
none Details | Review
Notification test patch (2.01 KB, patch)
2014-02-15 06:16 UTC, Mattias Bengtsson
none Details | Review
Add in app notifications (10.83 KB, patch)
2014-02-15 06:23 UTC, Mattias Bengtsson
none Details | Review
Notifications: Add static notifications (2.19 KB, patch)
2014-02-15 06:23 UTC, Mattias Bengtsson
none Details | Review
Notifications: static example (2.25 KB, patch)
2014-02-15 06:23 UTC, Mattias Bengtsson
none Details | Review
Notification test patch (2.01 KB, patch)
2014-02-15 06:23 UTC, Mattias Bengtsson
none Details | Review
MainWindow: remove unnecessary GtkGrid (1.81 KB, patch)
2014-02-15 07:30 UTC, Mattias Bengtsson
committed Details | Review
MainWindow: move the zoomcontrol to here (2.81 KB, patch)
2014-02-15 07:30 UTC, Mattias Bengtsson
committed Details | Review
Add in app notifications (10.71 KB, patch)
2014-02-15 07:31 UTC, Mattias Bengtsson
needs-work Details | Review
Notifications: Add static notifications (2.19 KB, patch)
2014-02-15 07:31 UTC, Mattias Bengtsson
needs-work Details | Review
Notifications: static example (2.25 KB, patch)
2014-02-15 07:31 UTC, Mattias Bengtsson
none Details | Review
Notification test patch (2.01 KB, patch)
2014-02-15 07:31 UTC, Mattias Bengtsson
none Details | Review
Add in app notifications (10.08 KB, patch)
2014-02-24 01:10 UTC, Mattias Bengtsson
committed Details | Review
Notifications: Add static notifications (1.94 KB, patch)
2014-02-24 01:11 UTC, Mattias Bengtsson
committed Details | Review
Notifications: static example (2.31 KB, patch)
2014-02-24 01:11 UTC, Mattias Bengtsson
none Details | Review
Notification test patch (2.02 KB, patch)
2014-02-24 01:11 UTC, Mattias Bengtsson
none Details | Review

Description Mattias Bengtsson 2014-02-10 06:05:43 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.
Comment 1 Mattias Bengtsson 2014-02-10 06:05:45 UTC
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.
Comment 2 Mattias Bengtsson 2014-02-10 06:05:49 UTC
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.
Comment 3 Mattias Bengtsson 2014-02-10 06:05:53 UTC
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.
Comment 4 Mattias Bengtsson 2014-02-10 06:05:56 UTC
Created attachment 268631 [details] [review]
Notification test patch

With this applied, try pushing the location button or changing base layer.
Comment 5 Jonas Danielsson 2014-02-10 11:02:36 UTC
Review of attachment 268629 [details] [review]:

Looks good otherwise.

::: src/mapView.js
@@ +91,2 @@
         this.setMapType(MapType.STREET);
 

One empty line to much, perhaps?
Comment 6 Jonas Danielsson 2014-02-10 11:24:00 UTC
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?
Comment 7 Mattias Bengtsson 2014-02-10 16:54:29 UTC
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.
Comment 8 Mattias Bengtsson 2014-02-10 17:07:35 UTC
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.
Comment 9 Mattias Bengtsson 2014-02-15 06:16:14 UTC
Created attachment 269167 [details] [review]
Notifications: Add static notifications

Add support for static notifications. These are pre-defined
notifications that are cached and reused.
Comment 10 Mattias Bengtsson 2014-02-15 06:16:20 UTC
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.
Comment 11 Mattias Bengtsson 2014-02-15 06:16:24 UTC
Created attachment 269169 [details] [review]
Notification test patch

With this applied, try pushing the location button or changing base
layer.
Comment 12 Mattias Bengtsson 2014-02-15 06:17:55 UTC
Gah, git-bz-fail. Disregard the three posts above.
Comment 13 Mattias Bengtsson 2014-02-15 06:23:13 UTC
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+
Comment 14 Mattias Bengtsson 2014-02-15 06:23:26 UTC
Created attachment 269171 [details] [review]
Notifications: Add static notifications

Add support for static notifications. These are pre-defined
notifications that are cached and reused.
Comment 15 Mattias Bengtsson 2014-02-15 06:23:32 UTC
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.
Comment 16 Mattias Bengtsson 2014-02-15 06:23:39 UTC
Created attachment 269173 [details] [review]
Notification test patch

With this applied, try pushing the location button or changing base
layer.
Comment 17 Mattias Bengtsson 2014-02-15 07:30:35 UTC
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+.
Comment 18 Mattias Bengtsson 2014-02-15 07:30:51 UTC
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.
Comment 19 Mattias Bengtsson 2014-02-15 07:30:59 UTC
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.
Comment 20 Mattias Bengtsson 2014-02-15 07:31:06 UTC
Created attachment 269178 [details] [review]
Add in app notifications

This patch adds in-app notifications. The code is derived from
gtk-widget-factory.
Comment 21 Mattias Bengtsson 2014-02-15 07:31:12 UTC
Created attachment 269179 [details] [review]
Notifications: Add static notifications

Add support for static notifications. These are pre-defined
notifications that are cached and reused.
Comment 22 Mattias Bengtsson 2014-02-15 07:31:18 UTC
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.
Comment 23 Mattias Bengtsson 2014-02-15 07:31:25 UTC
Created attachment 269181 [details] [review]
Notification test patch

With this applied, try pushing the location button or changing base
layer.
Comment 24 Jonas Danielsson 2014-02-15 11:50:18 UTC
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?
Comment 25 Jonas Danielsson 2014-02-15 11:50:55 UTC
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.
Comment 26 Jonas Danielsson 2014-02-15 13:09:03 UTC
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?
Comment 27 Jonas Danielsson 2014-02-15 13:15:15 UTC
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.
Comment 28 Zeeshan Ali 2014-02-15 15:33:06 UTC
(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.
Comment 29 Mattias Bengtsson 2014-02-16 22:18:32 UTC
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!
Comment 30 Mattias Bengtsson 2014-02-16 22:19:04 UTC
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.
Comment 31 Mattias Bengtsson 2014-02-16 22:34:27 UTC
(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".
Comment 32 Mattias Bengtsson 2014-02-16 23:31:14 UTC
Attachment 269176 [details] pushed as 08092f5 - MainWindow: remove unnecessary GtkGrid
Attachment 269177 [details] pushed as 84c5398 - MainWindow: move the zoomcontrol to here
Comment 33 Mattias Bengtsson 2014-02-17 00:56:38 UTC
(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.
Comment 34 Mattias Bengtsson 2014-02-17 01:15:39 UTC
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 }
  };
Comment 35 Mattias Bengtsson 2014-02-17 02:34:48 UTC
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!
Comment 36 Jonas Danielsson 2014-02-20 13:38:41 UTC
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. :)
Comment 37 Jonas Danielsson 2014-02-20 16:32:02 UTC
(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.
Comment 38 Jonas Danielsson 2014-02-20 16:38:11 UTC
(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?
Comment 39 Jonas Danielsson 2014-02-20 16:41:09 UTC
(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!
Comment 40 Mattias Bengtsson 2014-02-23 17:29:14 UTC
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
Comment 41 Mattias Bengtsson 2014-02-23 17:35:16 UTC
(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.
Comment 42 Mattias Bengtsson 2014-02-23 17:38:10 UTC
(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).
Comment 43 Mattias Bengtsson 2014-02-23 17:40:10 UTC
(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.
Comment 44 Zeeshan Ali 2014-02-23 18:18:40 UTC
(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).
Comment 45 Mattias Bengtsson 2014-02-23 19:25:35 UTC
(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!
Comment 46 Mattias Bengtsson 2014-02-24 01:10:55 UTC
Created attachment 270083 [details] [review]
Add in app notifications

This patch adds in-app notifications. The code is derived from
gtk-widget-factory.
Comment 47 Mattias Bengtsson 2014-02-24 01:11:29 UTC
Created attachment 270084 [details] [review]
Notifications: Add static notifications

Add support for static notifications. These are pre-defined
notifications that are cached and reused.
Comment 48 Mattias Bengtsson 2014-02-24 01:11:36 UTC
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.
Comment 49 Mattias Bengtsson 2014-02-24 01:11:45 UTC
Created attachment 270086 [details] [review]
Notification test patch

With this applied, try pushing the location button or changing base
layer.
Comment 50 Mattias Bengtsson 2014-02-24 01:13:21 UTC
These patches should fix all comments I think. :)
Comment 51 Jonas Danielsson 2014-02-25 11:52:17 UTC
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.
Comment 52 Jonas Danielsson 2014-02-25 11:53:15 UTC
Review of attachment 270084 [details] [review]:

::: src/notification.js
@@ +82,3 @@
 
+const Type = { };
+

Nit: the cache does not have a space between the curlies.
Comment 53 Jonas Danielsson 2014-02-25 11:55:12 UTC
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.
Comment 54 Jonas Danielsson 2014-04-01 20:39:24 UTC
Attachment 270083 [details] pushed as 531c441 - Add in app notifications
Attachment 270084 [details] pushed as ddba7aa - Notifications: Add static notifications
Comment 55 Mattias Bengtsson 2014-04-03 16:06:38 UTC
Jonas: Should we close this or do you want to keep it open for a potentially cleaner patch later?
Comment 56 Jonas Danielsson 2014-04-03 18:26:57 UTC
Let's close it.