General purpose notification service

[pkozlowski-opensource]

Should address #67

Made it a bit more general, so we could push any type of objects. A notification will be removed based on a category to which it was pushed.

This could be further extended (for example to evict notifications using the $timeout service) and probably coded in the way that is more generic but since we don't have any uses-cases for this right now the current version should do.

@petebacondarwin please review / comment / merge.

[petebacondarwin]

I like this. I'll merge it tonight.
Can I suggest a change to the API? We should streamline for the most common use cases. I believe these would be creating a simple info message with a string, clearing a notification and clearing all messages.

pushFixed(message, type='info')
pushCurrent(message, type='info')
pushNext(message, type='info')
all()
clear(notification | message)

The add... methods return a notification object you can use to remove.
The all method returns a list of notification objects.
The clear would remove the notification that matches the given object or the message string or everything if no parameter is passed.

More controversially, each notification object could have a clear function on it that you could use to remove itself from the service. This would be good in the UI because you could simply pass it a reference to the all() method and then it could clear messages as they were read off.

What do you think?

Pete

[pkozlowski-opensource]

I see from where you are coming, had similar thoughts. To answer your questions:

• I wouldn't like to remove the 'Route' part from the push methods - the API will become too cryptic
• +1 for some form of shortcut for string messages, this will be our main use case. But in this case I would either have an explicit method for this (pushFixedMsg) or simply get rid of the general purpose interface and focus on supporting messages only.
• I'm not huge fan of all-purpose-methods doing different things based on their arguments, but I'm also not against them :-) If we go this way I would prefer to name them get and clear respectively.
• Having a clearing method on an instance is not bad, was almost adding it :-) We can add it, will be nice in controllers. I would just name it $clear to be consistent with naming in the $resource.

Would be cool if we could get it to the repo today as it would unblock several other issues (I'm working on the i18n messages service atm). So I will do the changes we have discussed here and will push another commit. Then you will either merge it or push another commit with corrections. How does it sound?

[petebacondarwin]

Ok. I will be online in 2 hours

Pete
...from my mobile.
On Oct 20, 2012 5:59 PM, "Pawel Kozlowski" notifications@github.com wrote:

I see from where you are coming, had similar thoughts. To answer your
questions:

• I wouldn't like to remove the 'Route' part from the push methods -
  the API will become too cryptic
• +1 for some form of shortcut for string messages, this will be our
  main use case. But in this case I would either have an explicit method for
  this (pushFixedMsg) or simply get rid of the general purpose interface
  and focus on supporting messages only.
• I'm not huge fan of all-purpose-methods doing different things based
  on their arguments, but I'm also not against them :-) If we go this way I
  would prefer to name them get and clear respectively.
• Having a clearing method on an instance is not bad, was almost
  adding it :-) We can add it, will be nice in controllers. I would just name
  it $clear to be consistent with naming in the $resource.

Would be cool if we could get it to the repo today as it would unblock
several other issues (I'm working on the i18n messages service atm). So I
will do the changes we have discussed here and will push another commit.
Then you will either merge it or push another commit with corrections. How
does it sound?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/68#issuecomment-9633473.

[pkozlowski-opensource]

@petebacondarwin Pushed a new commit wit some changes, please have a look, review and change if needed.

As soon as we've got both notifications and i18n service we will be able to progress a lot on the app, making its UI more 'reactive' and i18n ready!

[petebacondarwin]

Hi Pawel
Sorry! I thought you had moved on to work on the I18N stuff so I went
ahead and refactored the notification service. I should have checked
first. I went to push it and then saw your push.
My changes are significantly different that I think it is worth discussing,
so I rebased and pushed them.
Pete

On 20 October 2012 22:01, Pawel Kozlowski notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin Pushed a new commit
wit some changes, please have a look, review and change if needed.

As soon as we've got both notifications and i18n service we will be able
to progress a lot on the app, making its UI more 'reactive' and i18n ready!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/68#issuecomment-9635966.

[pkozlowski-opensource]

@petebacondarwin Sorry, was to tired to respond yesterday. And I'm not too worried about the pushing 2 versions, at least we are able to discuss with the code examples!

I just had a fresh look at your proposal and I (mostly) like your API, provided that we do focus on string messages only for now (which should be enough). I will add some more comments to the code.

I'm a bit concerned with the implementation thought... It looks "cleaner" but got over 2x bigger compared to the initial one... without bringing new flexibility (I'm not even sure we want / need more flexibility at this point...). The bottom line for me is that impl got harder to understand (arguably) without bringing much more flexibility (but I might have missed things!).

Going to comment more in the code itself.

[petebacondarwin]

I am happy to go with $remove. It fits with the Angular way.

[petebacondarwin]

Happy to lose the current/all and just go with one method. current or all?
I was confused by all because it doesn't actually return all the notifications.

[petebacondarwin]

I am not keen on using dependency style injections. It feels too magical to me.
I find injecting $injector and then calling get on it less surprising and more self explanatory.
I've noticed it appearing more often in the angular tests too.

[petebacondarwin]

Two benefits of this implementation is that we could turn it into a provider/service model where people could configure their own notification lists in addition to the standard ones very easily. Also I meant to expose each queue on the service with a method call - e.g. stickyNotifications(), currentRouteNotifications() and nextRouteNotifications(), which would allow direct access to those nice methods on the list directly for more fine grained work.

I agree it does appear more complex to the casual observer.

Let me know what you would prefer to do. I am fairly relaxed about it.

Pete

[pkozlowski-opensource]

Thnx for all the comments / responses to my comments!

For me the most important part is that we've got the interface right (after removing all and renaming current to sth like get, and also renaming remove -> $remove on the instance level.

For the implementation itself - I don't mind either way. Don't want to spend too much time on it right now as somehow this is rather basic service. So going to do the changes we've discussed and merge it. We will see if we need any additional flexibility as we go and refactor accordingly.

[pkozlowski-opensource]

Landed as 6507541