Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Eclipse Projects » Mylyn » Mylyn Notifications: Sort newest first in popup notification(Working around a potential bug with heterogeneous AbstractNotification lists)
Mylyn Notifications: Sort newest first in popup notification [message #1726175] Thu, 10 March 2016 10:31 Go to next message
Andreas Sewe is currently offline Andreas SeweFriend
Messages: 109
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
Re: Mylyn Notifications: Sort newest first in popup notification [message #1726256 is a reply to message #1726175] Thu, 10 March 2016 18:49 Go to previous messageGo to next message
Sam Davis is currently offline Sam DavisFriend
Messages: 92
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.
Re: Mylyn Notifications: Sort newest first in popup notification [message #1726302 is a reply to message #1726256] Fri, 11 March 2016 08:32 Go to previous messageGo to next message
Andreas Sewe is currently offline Andreas SeweFriend
Messages: 109
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:

  1. Require that all notifications are sorted by date, earliest first (effectively making AbstractNotification final.
  2. 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?
Re: Mylyn Notifications: Sort newest first in popup notification [message #1726351 is a reply to message #1726302] Fri, 11 March 2016 17:46 Go to previous messageGo to next message
Sam Davis is currently offline Sam DavisFriend
Messages: 92
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.
Re: Mylyn Notifications: Sort newest first in popup notification [message #1726492 is a reply to message #1726351] Mon, 14 March 2016 10:02 Go to previous messageGo to next message
Andreas Sewe is currently offline Andreas SeweFriend
Messages: 109
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.
Re: Mylyn Notifications: Sort newest first in popup notification [message #1726495 is a reply to message #1726492] Mon, 14 March 2016 10:10 Go to previous message
Andreas Sewe is currently offline Andreas SeweFriend
Messages: 109
Registered: June 2013
Senior Member
FYI, I've opened Bug 489525 with an attached sample project demonstrating the IllegalArgumentException I talked about above.
Previous Topic:Mylyn Connectors for Java Helios
Next Topic:Mylyn 3.19 is now available
Goto Forum:
  


Current Time: Sun Nov 19 21:31:18 GMT 2017

Powered by FUDForum. Page generated in 0.02304 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software