Eclipse Community Forums: Mylyn » Mylyn Notifications: Sort newest first in popup notification

Home » Eclipse Projects » Mylyn » Mylyn Notifications: Sort newest first in popup notification(Working around a potential bug with heterogeneous AbstractNotification lists)

Show: Today's Messages :: Show Polls :: Message Navigator

Mylyn Notifications: Sort newest first in popup notification [message #1726175]

Thu, 10 March 2016 10:31

Andreas Sewe

Messages: 111
Registered: June 2013

Senior Member

Hi,

I am currently writing a plugin that wants to display notifications. As such, reusing Mylyn Notifications is quietly appealing. So far, most of it works great. Smile

Alas, there's one issue: If there are multiple notifications, they should be displayed "newest first" in the popup window. But as far as I can see AbstractNotification.compareTo fixes the sort order to "oldest first". And using a different implementation of compareTo in a subclass won't work IMHO, even though ServiceMessage does does just that:

public int compareTo(ServiceMessage o) {
	return -getId().compareTo(o.getId());
}

But AFAICT a sink may display notification's of different AbstractNotification subclasses at the same time. At least, PopupNotificationSink's currentlyNotifying list seems to alow this, which IMHO is buggy, as Collections.sort will potentially see AbstractNotifications with inconsistent notions of what compareTo means: ServiceMessage.compareTo(AbstractNofication) is not the inverse of AbstractNofication.compareTo(ServiceMessage).

Is this a bug? If so, how to work around it (ideally without implementing my own sink & popup)?

Best wishes,

Andreas

Report message to a moderator

Re: Mylyn Notifications: Sort newest first in popup notification [message #1726256 is a reply to message #1726175]

Thu, 10 March 2016 18:49

Sam Davis

Messages: 98
Registered: July 2012

Member

It looks like a bug that ServiceMessage.compareTo does not override the method in the super class, because the type of the paramter is ServiceMessage instead of AbstractNotification. As long as you correctly override that method in your subclass I don't see why it wouldn't work.

I think that you are right that if you have different types of notifications with different implementations of compareTo in the same sink, you will get inconsistent behaviour.

Report message to a moderator

Re: Mylyn Notifications: Sort newest first in popup notification [message #1726302 is a reply to message #1726256]

Fri, 11 March 2016 08:32

Andreas Sewe

Messages: 111
Registered: June 2013

Senior Member

Quote:

It looks like a bug that ServiceMessage.compareTo does not override the method in the super class, because the type of the paramter is ServiceMessage instead of AbstractNotification. As long as you correctly override that method in your subclass I don't see why it wouldn't work.

Good catch, ServiceMessage.compareTo doesn't even override AbstractNotification.compareTo. But [i]BuildsServiceNotification.compareTo[i] does...

Quote:

I think that you are right that if you have different types of notifications with different implementations of compareTo in the same sink, you will get inconsistent behaviour.

...so I think that this might become an problem in practice, as I have seen Collections.sort throw an IllegalArgumentException when the compareTo contract is violated (see ComparableTimSort's implementation).

I think there's two way's around this:

Require that all notifications are sorted by date, earliest first (effectively making AbstractNotification final.
Mandate that notifications from multiple NotificationSinkEvents are not
"blended together" by a NotificationSink.

To me, option 1 is clearly undesirable, as sorting is highly notification-specific. On the other hand, option 2 may even positively affect the UX, as a popup displaying both build results and new issues together might be confusing.

Anyway, should I move this conversation over to Bugzilla?

Report message to a moderator

Re: Mylyn Notifications: Sort newest first in popup notification [message #1726351 is a reply to message #1726302]

Fri, 11 March 2016 17:46

Sam Davis

Messages: 98
Registered: July 2012

Member

Sure. I haven't checked but I suspect different types are never used with the same sink, otherwise we would have seen this problem. But it could be a good idea to enforce that if we can do it without breaking API.

Report message to a moderator

Re: Mylyn Notifications: Sort newest first in popup notification [message #1726492 is a reply to message #1726351]

Mon, 14 March 2016 10:02

Andreas Sewe

Messages: 111
Registered: June 2013

Senior Member

Quote:

Sure. I haven't checked but I suspect different types are never used with the same sink, otherwise we would have seen this problem.

My impression was that different types of notifications can be routed to different sinks by design. In particular, the Notifiers checkbox list on the General > Notifications preference page suggest that users are allowed to route arbitrary types of notifications to arbitrary sinks, which thanks to the AbstractNotification interface should all be able to do "something sensible" with them (although some sinks may offer extra functionality for certain kinds of events).

Also, it would also be very cumbersome if everyone had to implement their own variant of the "Desktop Popup" sink for their notifications. Fade-in/fade-out in particular is a wheel you don't want to reinvent all the time.

Not only would the burden on implementers increase if everyone needs their own variant sinks, but user experience would also suffer from multiple "Desktop Popup" sinks: As these variant implementations don't know about each other, the user may end up with different sinks' popups being layered on top of each other.

Quote:

But it could be a good idea to enforce that if we can do it without breaking API.

I think the solution is to require of sinks not to mix notifications from NotificationSinkEvents (and hence NotificationService.notify calls).

I don't think this breaks API at all, although of course if changes the observable behavior of sinks. But as NotificationSink.notify doesn't mandate a particular behavior (it doesn't even have Javadoc), I don't think one can call this API breakage.

Report message to a moderator