Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 331424

Summary: [commons][api] When declaring a notification sink it should be possible to specify which event types to display
Product: z_Archived Reporter: Torkild Resheim <torkildr>
Component: MylynAssignee: Torkild Resheim <torkildr>
Status: RESOLVED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: steffen.pingel
Version: unspecifiedKeywords: contributed
Target Milestone: 3.5   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 330064    
Bug Blocks: 333072    
Attachments:
Description Flags
Patch to fix the issue
none
mylyn/context/zip
none
adds support for "eventMapping" extension steffen.pingel: iplog+

Description Torkild Resheim CLA 2010-11-30 08:12:52 EST
Using the build service messages as example we have two types of events and two corresponding sinks. Normally we would want the build service events to show up in the builds view and the build plan events in the notification pop-ups. Because of ongoing improvements to the notification API it is possible for the user to specify which events to display where. By default all events are displayed everywhere as we currently have no means to do otherwise.

Hence it should be possible to specify which event types to display by default when declaring a sink. Allowing a wild-card would also be useful. If we for instance want all builds notifications in the pop-up we could declare it something like this:

    <extension point="org.eclipse.mylyn.commons.notifications.notifications">
        <sink class="org.eclipse.mylyn.internal.commons.ui.notifications.popup.PopupNotificationSink"
            id="org.eclipse.mylyn.commons.notifications.sink.Popup"
            label="Desktop Popup">
            <include>
                <event match="org.eclipse.mylyn.builds.ui.events.*"/>
            </include>
        </sink>
    </extension>

Or if we just want the build plan events:

    <include>
        <event match="org.eclipse.mylyn.builds.ui.events.PlanStatusChanged"/>
    </include>

It should be possible to use regular expressions in the "match" clause.
Comment 1 Torkild Resheim CLA 2010-12-22 04:39:54 EST
Considering that one would typically have only a few notification sinks and maybe just reuse what is already defined; it makes more sense that one on an event declaration would be able to specify which default sinks to use. After all it is more likely that new events are added than new sinks. Changed subject accordingly.
Comment 2 Torkild Resheim CLA 2010-12-22 06:19:50 EST
Created attachment 185692 [details]
Patch to fix the issue

Default sinks to use for a particular event can now be specified in the event type declaration. For instance:

 <extension
       point="org.eclipse.mylyn.commons.notifications.notifications">
    <event
          categoryId="org.eclipse.mylyn.builds.ui.category.Builds"
          id="org.eclipse.mylyn.builds.ui.events.PlanStatusChanged"
          label="Plan Status Changed">
       <description/>
       <defaultHandler
             sink="org.eclipse.mylyn.commons.notifications.sink.Popup">
       </defaultHandler>
    </event>
Comment 3 Torkild Resheim CLA 2010-12-22 06:19:53 EST
Created attachment 185693 [details]
mylyn/context/zip
Comment 4 Torkild Resheim CLA 2010-12-22 06:26:31 EST
(In reply to comment #2)
> Default sinks to use for a particular event can now be specified in the event
> type declaration. For instance:
Note that if no default sinks are specified, all will be used. So this will not break existing code.
Comment 5 Steffen Pingel CLA 2010-12-22 18:54:09 EST
Sounds like a good idea to me and a regular expression should work well. To keep it simple I would suggest that we add an optional filter tag under the sink extensions:  <filter match="..."/>. In theory we could use the core expression extension schema to allow complex expressions but as a start I think the proposed approach would work well.
Comment 6 Torkild Resheim CLA 2010-12-23 04:07:29 EST
Ok... So for each "sink" you want to be able to specify which "event" it will per default handle? That was my initial idea. But I later on (when I actually started implementing this) I found that it may not work so good. It would mean that we would probably have to change the declaration of the current notification popup (org.eclipse.mylyn.commons.notifications.sink.Popup) for each new event added that will use it. For API consumers that want to use notifications that could be a problem. Which why I ended up with the implementation described in comment #1 and comment #2.

Now we could implement the initial proposal with the changes in comment #5 but I don't see the need for it unless there is a reason why the patch won't work.
Comment 7 Steffen Pingel CLA 2010-12-27 10:34:41 EST
Good point. We could solve this through an extension point that maps event types to sinks, e.g. 

 <eventMapping 
sinkId="org.eclipse.mylyn.commons.notifications.sink.Popup"
eventIds="org.eclipse.mylyn.builds.ui.events.*"
</eventMapping>

If no mapping is specified for a particular event it would be handled by all sinks.
Comment 8 Torkild Resheim CLA 2010-12-28 03:48:19 EST
Ok, I can add this. I think we should keep the mapping/handler mechanism described in comment #2 though. Except adjust it a bit so the attribute names are the same.
Comment 9 Torkild Resheim CLA 2010-12-30 07:33:27 EST
Created attachment 185907 [details]
adds support for "eventMapping" extension

In addition to the optional "defaultHandler" element in "event" it is now possible to add an "eventMapping" extension as described in comment #7. The extension point document explains use and gives an example.
Comment 10 Steffen Pingel CLA 2011-01-04 21:19:11 EST
Thanks! I have applied the patch.
Comment 11 Steffen Pingel CLA 2011-01-04 21:19:56 EST
Reopening to fix assignment.
Comment 12 Steffen Pingel CLA 2011-01-04 21:20:28 EST
Closing.