The plain messages are the ones hown by:

Application.showMessage(msg);


$ git grep showMessage
src/mainWindow.js:            Application.notificationManager.showMessage(message);
src/mainWindow.js:            Application.notificationManager.showMessage(message);
src/mapView.js:                Application.notificationManager.showMessage(e.message);
src/mapView.js:            Application.notificationManager.showMessage(msg);
src/mapView.js:            Application.notificationManager.showMessage(msg);
src/notificationManager.js:    showMessage: function (msg) {
src/placeEntry.js:                Application.notificationManager.showMessage(msg);
src/routeService.js:                    Application.notificationManager.showMessage(_("No route found."));
src/routeService.js:                Application.notificationManager.showMessage(_("Route request failed."));
src/sendToDialog.js:                Application.notificationManager.showMessage(msg);

i have done some work on this issue, how do i make sure that it actually solve this 
problem ??

Created attachment 319293 [details] [review]
A patch to remove duplicate plain message.

In this patch i checked if a plain message already present on stack. then don't
add that one. else add that.

Review of attachment 319293 [details] [review]:

Thanks!

This is not an approach I think will work.
Check notificationManager.js. What is keeping custom notification from spamming? Like the locationServiceNotification?
Can you be inspired?

in showNotification function, you should probably make a special case of the instanceof notification is a Notification.Plain?

::: src/notification.js
@@ +80,3 @@
                                     label   : msg });
+        if ( _checkMsg(label) === false )
+            this._ui.body.add(label);

I do not understand, how is this supposed to work?

You are creating this object now, how could this ever be true?

@@ +86,3 @@
+        for each (let msg in  this._ui.body) {
+
+        if (msg === label)

label is a GtkLabel

I think we should take some time and think about how this bug should be solved. Maybe we could keep track of notifications and and if there is more than one show a 'details' buttons that when clicked shows a list of all messages?

Or maybe if the user hovers (or taps) on the notification body, it expands (like gnome-shell's notifications) to show all the undismissed notifications?

Created attachment 319602 [details] [review]
Updated patch.

Sorry for the last time i misunderstood bug.hope this patch will solve it.

Review of attachment 319602 [details] [review]:

Thanks!

::: src/notificationManager.js
@@ +25,3 @@
 const Notification = imports.notification;
 
+let plainMessage = [];

I do not like this approach. We already store all text somewhere, having an extra array does not feel correct.

@@ +48,3 @@
+        if(temp) {
+            this._overlay.add_overlay(notification);
+                break;

So text never gets removed from the array? What happens when the user dismiss a notification, we can never get the same notification again?

Please think the use-cases through! What do we want to accomplish?

I would rather have checks in addNotification, something special if the instanceof the notification is PlainMessage, maybe?

we only store plain message on the overlay container, and there is no method 
such that we can determine that a perticular message is right now on the manager
so i thought going with the storing a plain message may be right, we only have 
very few types of plain messages to show, so it will not increase space taken
by application. @jonasd if you find anything wrong in above statement please let 
me know.I went through all the docs related to Gtk.Overlay and Gtk container
but didnt find any way to transverse a overlay container.

(In reply to Prashant Tyagi from comment #8)
> we only store plain message on the overlay container, and there is no method 
> such that we can determine that a perticular message is right now on the
> manager
> so i thought going with the storing a plain message may be right, we only
> have 
> very few types of plain messages to show, so it will not increase space taken
> by application. @jonasd if you find anything wrong in above statement please
> let 
> me know.I went through all the docs related to Gtk.Overlay and Gtk container
> but didnt find any way to transverse a overlay container.

this._overlay.get_children()?

(In reply to Jonas Danielsson from comment #9)
> (In reply to Prashant Tyagi from comment #8)
> > we only store plain message on the overlay container, and there is no method 
> > such that we can determine that a perticular message is right now on the
> > manager
> > so i thought going with the storing a plain message may be right, we only
> > have 
> > very few types of plain messages to show, so it will not increase space taken
> > by application. @jonasd if you find anything wrong in above statement please
> > let 
> > me know.I went through all the docs related to Gtk.Overlay and Gtk container
> > but didnt find any way to transverse a overlay container.
> 
> this._overlay.get_children()?

thanks jonas i tried that, infact it is the first thing i tried.this._overlay.get_children() gives the list of type of widget attached at particular position . for a two same plain messages "No route found" it gives different value.

Created attachment 319766 [details] [review]
A patch to remove duplicate plain message.

A updated patch.

Review of attachment 319766 [details] [review]:

Thanks!

git diff --check origin/master should not report any problems

We do not have a module called plainMessage, please amend the commit message.

Also, should we really not add a plain message if the message is anywhere in the stack of notifications? Or should we just look at the latest?
Imagine we get a route not found, then a bunch of other messages, then a differant route not found. Should that appear or not?

::: src/notification.js
@@ +75,3 @@
     _init: function(msg) {
         this.parent();
+        this.msg = msg;

Either have this private and add a function:

this._label = new Gtk...

and

equals: function(notification) {
      return this._label.label === notification._label.label;
}

or have

this.label = new Gtk.Label ...

No need for a new msg property.

::: src/notificationManager.js
@@ +35,1 @@
     showMessage: function (msg) {

Add the code in showNotification instead.

@@ +47,3 @@
+            }
+       }
+        if(addMsg) {

Can we invert this later on so that we return if a function that checks the if the plain message exists, returns false

@@ +51,3 @@
+            let dismissId = notification.connect('dismissed', (function() {
+                this._overlay.remove(notification);
+                notification.disconnect(dismissId);

No need for this, the destroy function disconnects signals.

Created attachment 319798 [details] [review]
A patch to remove duplicate plain message.

A updated patch.

Review of attachment 319798 [details] [review]:

::: src/notificationManager.js
@@ +35,3 @@
     showMessage: function (msg) {
         let notification = new Notification.Plain(msg);
+        let existNotification = this._overlay.get_children();

I think this should probably be called something like "existingNotifications"

Review of attachment 319798 [details] [review]:

::: src/notificationManager.js
@@ +48,3 @@
+        if(!addMsg)
+            this._overlay.add_overlay(notification);
+        print(existNotification.length);

Left-over debug output here…

(In reply to Marcus Lundblad from comment #15)
> Review of attachment 319798 [details] [review] [review]:
> 
> ::: src/notificationManager.js
> @@ +48,3 @@
> +        if(!addMsg)
> +            this._overlay.add_overlay(notification);
> +        print(existNotification.length);
> 
> Left-over debug output here…

yeah that is a mistake, if anything conceptually needed please let me know.

I took the latest patch for a run, and here are some quick UX feedback.
I tried to go from China to Oslo, Norway and Maps gave me the message that the route was not found. I then tried to get a route from China to Stockholm.

* I got a notification about the China-Oslo failed route, but not for the China-Stockholm route. I had expected there to be a new message that the China-Stockholm route failure.
* If I do a failed route search and then tries to open a custom OSM layer, but runs into an error, the OSM layer error overlaps the previous notification error.
* Notifications never time out like they do after a couple of seconds in Nautilus. The file manager timeout is 6 seconds, and I think we should do the same.
* There are some style inconsistencies between the notifications in Maps and Files/Boxes etc. That is probably a separate bug though.

Let me know what of the above issues belong in this bug report and what should be filed as separate issues.

So what about this:

* Only show one notification at one time. If a new notification comes in, dismiss the old one.
* In that case, the old notification slides up, and the new one slides down. Like passing notes under a door.
* In order to avoid this happening too often, notifications needs to time out after 6 seconds unless closed first.

(In reply to Andreas Nilsson from comment #18)
> So what about this:
> 
> * Only show one notification at one time. If a new notification comes in,
> dismiss the old one.
> * In that case, the old notification slides up, and the new one slides down.
> Like passing notes under a door.
> * In order to avoid this happening too often, notifications needs to time
> out after 6 seconds unless closed first.

In order to time out a message you mean that message should be dismiss
after 6sec, is it??

(In reply to Jonas Danielsson from comment #12)
> Review of attachment 319766 [details] [review] [review]:
> 
> Thanks!
> 
> git diff --check origin/master should not report any problems
> 
> We do not have a module called plainMessage, please amend the commit message.
> 
> Also, should we really not add a plain message if the message is anywhere in
> the stack of notifications? Or should we just look at the latest?
> Imagine we get a route not found, then a bunch of other messages, then a
> differant route not found. Should that appear or not?
> 
> ::: src/notification.js
> @@ +75,3 @@
>      _init: function(msg) {
>          this.parent();
> +        this.msg = msg;
> 
> Either have this private and add a function:
> 
> this._label = new Gtk...
> 
> and
> 
> equals: function(notification) {
>       return this._label.label === notification._label.label;
> }
> 
> or have
> 
> this.label = new Gtk.Label ...
> 
> No need for a new msg property.
> 
> ::: src/notificationManager.js
> @@ +35,1 @@
>      showMessage: function (msg) {
> 
> Add the code in showNotification instead.
> 
> @@ +47,3 @@
> +            }
> +       }
> +        if(addMsg) {
> 
> Can we invert this later on so that we return if a function that checks the
> if the plain message exists, returns false
> 
> @@ +51,3 @@
> +            let dismissId = notification.connect('dismissed', (function() {
> +                this._overlay.remove(notification);
> +                notification.disconnect(dismissId);
> 
> No need for this, the destroy function disconnects signals.

jonasdn we are storing a locationservice notification beacause we need them in
order to keep track which of them are already displayed so we do not display them again ,but for plain message why there is even need to store a message once it is displayed we never need them.

(In reply to Prashant Tyagi from comment #19)
> (In reply to Andreas Nilsson from comment #18)

> In order to time out a message you mean that message should be dismiss
> after 6sec, is it??

Yes, have the message automatically go away after 6 seconds.

Created attachment 320154 [details] [review]
A patch to remove duplicate plain message.

Review of attachment 320154 [details] [review]:

We should never have to look for existing notifications anymore, right?
Since we now are changing the behavior of the in-app notifications so that only one notification is on "the stack" at a time.
So you can remove quite a bit of code. And just make sure that everytime a new notification is added, we remove the old and when that animation is done we add a new.
Along with that we can add a timeout on each added notification so that it will remove itself in _NOTIFICATION_TIMEOUT time.

::: src/notificationManager.js
@@ +23,3 @@
 const Lang = imports.lang;
 
+const Mainloop = imports.mainloop;

Format the imports like in other file and alphabetical.

@@ +43,2 @@
         this._overlay.add_overlay(notification);
+        this._destroyNotification(notification);

What is happening here?

@@ +49,3 @@
     showNotification: function(notification) {
         if(notification.get_parent() !== this._overlay) {
+            this._destroyOverlappingNotification();

Why is this needed?

@@ +64,3 @@
+    _destroyNotification: function(notification) {
+        let overlay = this._overlay;
+        Mainloop.timeout_add(6000,(function() {

Use constants for values like this, that helps explain them.
Also, you are missing a space here, right? And if you do not intend to bind the closure to this, you do not need the extra parenthesis before function.

@@ +66,3 @@
+        Mainloop.timeout_add(6000,(function() {
+                             overlay.remove(notification);
+                             notification.destroy.bind(notification);

What are you doing here?

@@ +70,3 @@
+    },
+
+                             overlay.remove(notification);

Is this function really needed anymore?

Review of attachment 320154 [details] [review]:

::: src/notificationManager.js
@@ +43,2 @@
         this._overlay.add_overlay(notification);
+        this._destroyNotification(notification);

this will make sure that notification will be timeout in 6sec after it is pushed on the stack.

@@ +49,3 @@
     showNotification: function(notification) {
         if(notification.get_parent() !== this._overlay) {
+            this._destroyOverlappingNotification();

when adding a new notification on the stack, i checked that whether there is any notification already attached on the stack so i removed that before adding a new one(that happens when two cosecutive notification tries to add within 6sec).

@@ +70,3 @@
+    },
+
+                             overlay.remove(notification);

this function is for  removing a notification from the stack if a new notification comes with a time less that 6sec.

Review of attachment 320154 [details] [review]:

::: src/notificationManager.js
@@ +49,3 @@
     showNotification: function(notification) {
         if(notification.get_parent() !== this._overlay) {
+            this._destroyOverlappingNotification();

Yes, but notification.get_parent() will only ever work on singleton notifications like locationServiceNotification, right?

@@ +70,3 @@
+    },
+
+                             overlay.remove(notification);

We have to be able to do this simpler, right? We ever only have 1 child, so we can store that child as this._currentNotification or something, right?

Looks good in general ui-wise. I like the timeout!

Does the "Failed to open layer" notification work differently from the "No route found" notification? With the first I can continue to right-click the map, but not with the second one.

Created attachment 320395 [details] [review]
Updated patch.

In this patch i use only one function for Plain as well as for locationservice
notification.

Review of attachment 320395 [details] [review]:

Thanks!

Please do not remove showMessage. Keep showMessage as its own function.

I think the formatting of the commit message can be better. Also the subject.

notificationManager: Simplify notification handling


maybe?

::: src/notificationManager.js
@@ +25,3 @@
 const Notification = imports.notification;
 
+const Mainloop = imports.mainloop;

Move this to other forign imports.

@@ +33,3 @@
         this._overlay = overlay;
         this._cache = {};
+        this._currentNotification = null;

No need to init this here, right?

@@ +34,3 @@
         this._cache = {};
+        this._currentNotification = null;
+        this._timeoutTime = 6000;

Have this as a contant intstead like other constants are treated in other modules.

@@ +35,3 @@
+        this._currentNotification = null;
+        this._timeoutTime = 6000;
+        this._timeOut = false;

what is this used for?

@@ +43,1 @@
+        if(this._currentNotification)

space after if: if (...

@@ +43,2 @@
+        if(this._currentNotification)
+            this._currentNotification.destroy();

We want to dismiss here I feel and only add the newone in the dismissed callback, so we get the transitions, right?

@@ -43,2 @@
-    showNotification: function(notification) {
-        if(notification.get_parent() !== this._overlay) {

Why remove this check? I see merit in keeping it and removing it, can you explain your thoughts?

@@ +49,3 @@
+        Mainloop.timeout_add(this._timeoutTime, function() {
+            notification.destroy();
+            return false;});

Have the }); on own line.

Review of attachment 320395 [details] [review]:

::: src/notificationManager.js
@@ -43,2 @@
-    showNotification: function(notification) {
-        if(notification.get_parent() !== this._overlay) {

previously we create a locationservice notification and use that for any call. so when two locationservice queries comes one after other, notiificationManager treated that as a same 
notification.but i did it beacause at every query of notification we want new notification that 
to be displayed, and also on previous locationservice notification destroy function will destroy 
that widget but we need it again.if you see a mainWindow.js file that i have updated you will
get hang of it.

Created attachment 320399 [details] [review]
Updated patch.

A updated patch.

Review of attachment 320399 [details] [review]:

::: src/notificationManager.js
@@ +26,2 @@
 const Notification = imports.notification;
+const timeOutTime = 6000;

No, format it like other constants and have a new line between the imports:


const _NOTIFICATION_TIMEOUT = 6000; /* ms */

@@ +27,3 @@
+const timeOutTime = 6000;
+
+let currentNotification = null;

No, keep it as 'this._notification' but you do not need to init it as null:

!this._notification catches undefined as well

@@ +43,3 @@
+        if (currentNotification) {
+            if (currentNotification.get_parent === this._overlay)
+                this._overlay.remove(currentnotification);

This can _never_ happend. We do not have any instance of Plain message as singleton and we will not have it either.

@@ +50,1 @@
         this._overlay.add_overlay(notification);

We should not add until the previous one has been dismissed.
But all this should be able to be in showNotification, right?

@@ +74,3 @@
+            notification.dismiss();
+            return false;
+        });

Will this not schedule an extra dismiss if the notification was already there?

"notification.get_parent() === this._overlay case?

@@ +76,3 @@
+        });
+
+

New newline.

Review of attachment 320399 [details] [review]:

::: src/notificationManager.js
@@ +43,3 @@
+        if (currentNotification) {
+            if (currentNotification.get_parent === this._overlay)
+                this._overlay.remove(currentnotification);

this is for when you already have locationservice notification attached to the window and any 
plain message comes, in that case you have to remove a locationservice notification from the 
overlay and if the current notification is a plain message you dismiss it that.

@@ +50,1 @@
         this._overlay.add_overlay(notification);

above if else statement make sure that previous notification dismiss.

@@ +73,3 @@
+        Mainloop.timeout_add(timeOutTime, function() {
+            notification.dismiss();
+            return false;

for every locationservice notification query(whether the locationservice notification is already attached or not) you reveal it, so it is like added a new notification so that needs to 
timeout after certain time.

Review of attachment 320399 [details] [review]:

::: src/notificationManager.js
@@ +43,3 @@
+        if (currentNotification) {
+            if (currentNotification.get_parent === this._overlay)
+                this._overlay.remove(currentnotification);

get_parent should be get_parent() I guess? Have this been tested? And how can currentNotification ever not have this._overlay as parent? This it is not currentNotification.

@@ +50,1 @@
         this._overlay.add_overlay(notification);

No you do not, there is animation involved. You do not wait for that animation to finish. And again you should be able to only add code in showNotification, right?

Created attachment 322627 [details] [review]
notificationManager: Only allow one notification

Do not stack notification on each other. Only allow one at a time.
Add the next one after the current one has transitioned away.

Also add a timeout of 5 seconds before auto-dismiss a notification.

(In reply to Jonas Danielsson from comment #34)
> Created attachment 322627 [details] [review] [review]
> notificationManager: Only allow one notification
> 
> Do not stack notification on each other. Only allow one at a time.
> Add the next one after the current one has transitioned away.
> 
> Also add a timeout of 5 seconds before auto-dismiss a notification.

How about something like this?

Attachment 322627 [details] pushed as b5c8ea2 - notificationManager: Only allow one notification