Community
Participate
Working Groups
The TasksView has a nice GUI for displaying service messages. There is also some fairly generic code for handling such messages. The BuildsView should have a similar mechanism for displaying service messages.
Blocking 317890 as we want this feature for telling the user that a new Hudson server has been discovered.
Could someone assign this bug to me? I'm already on it.
I think I'm pretty much done with this one. I'll do some more tweaking and cleaning up. The problem is that the patch is at 1071 lines. I think I can get below the 250 lines limit if I patch one and one file though. Any tips?
In that case we'll need to put it through IP review. Should quick though if it's just a 1000 lines. Can you attach it here?
Created attachment 179441 [details] Patch to fix bug 325079 This patch implements a mechanism for handling service messages and also support invoking a Mylyn repository wizard when a link in the service message has been clicked.
Created attachment 179442 [details] mylyn/context/zip
It looks there is quite a bit of code duplication with tasks.ui. I would prefer to move the service message control code into the commons to make it available for reuse. How about I take a first pass at refactoring that before we proceed so the builds framework can consume the implementation from commons.ui?
(In reply to comment #7) > It looks there is quite a bit of code duplication with tasks.ui. I would prefer > to move the service message control code into the commons to make it available > for reuse. How about I take a first pass at refactoring that before we proceed > so the builds framework can consume the implementation from commons.ui? Yes I was wanting to do that, was not sure if it was OK. I think at least the control is a candidate. Not sure about the rest though.
Created attachment 179618 [details] Patch to fix bug 325079 This patch places a common message control in org.eclipse.mylyn.commons.ui for reused.
Created attachment 179625 [details] Updated patch The view layout was broken in the previous patch. Fixed.
Thanks for the patch. To make progress on this I would suggest to tackle bug
... bug 326736 first which is basically a subset of the patch. I am not sure I understand why the complexity in BuildServerMessageManager and BuildServiceMessageEvent is needed. The motivation in the notifications framework was mostly to allow for future extensibility in a binary backwards compatible manner. Wouldn't it suffice here to have an API interface that is directly implemented by the Builds view without a need for a manager that supports queuing of messages etc? IBuildServiceMessageHandler { handle(BuildServiceMessage message) } BuildServiceMessage would also need to be API but could be marked as @noextend.
Well I thought a bit about that. There could be more than one Hudson server on the local network and all should normally be discovered at the same time so to gather for that... There could also be service messages from other sources in the future. I'll split up the patch for bug 326736.
Created attachment 180048 [details] Updated patch Should contain only the relevant parts.
Created attachment 180049 [details] mylyn/context/zip
(In reply to comment #13) > Well I thought a bit about that. There could be more than one Hudson server on > the local network and all should normally be discovered at the same time so to > gather for that... There could also be service messages from other sources in > the future. I agree. The o.e.m.commons.notifications framework was intended to provide that kind of global event bus. Instead of duplicating the implementation why don't we extend it to support this use case and make the Builds view a notification sink that displays these messages? I would also like to keep the blank view with the configuration link if no servers are configured. The service message could be shown below that. I know that we use the service control in the Task List to guide through the initial setup but that is questionable as well and we have turned this off in some cases since the gradient is visually "loud".
(In reply to comment #16) > (In reply to comment #13) > > Well I thought a bit about that. There could be more than one Hudson server on > > the local network and all should normally be discovered at the same time so to > > gather for that... There could also be service messages from other sources in > > the future. > > I agree. The o.e.m.commons.notifications framework was intended to provide that > kind of global event bus. Instead of duplicating the implementation why don't we > extend it to support this use case and make the Builds view a notification sink > that displays these messages? Ok... I will take a look. > > I would also like to keep the blank view with the configuration link if no > servers are configured. The service message could be shown below that. I know > that we use the service control in the Task List to guide through the initial > setup but that is questionable as well and we have turned this off in some cases > since the gradient is visually "loud". I agree. Will put back the link.
Created attachment 180156 [details] Updated patch This patch implements service notifications using o.e.m.c.notifications instead of some private mechanism.
Created attachment 180157 [details] mylyn/context/zip
Forgot to put back the link. New patch coming right up.
So I've been playing around a bit and became unsure about the link. UI-wise it's a bit awkward as we are using two different means of prompting the user to create a new repository (link and service message). Also this method of telling the user to create something is not used much elsewhere in Eclipse (AFAIK only in the Task List). There is already a local toolbar button to create a new repository. Do we really need the link?
(In reply to comment #21) > So I've been playing around a bit and became unsure about the link. UI-wise it's > a bit awkward as we are using two different means of prompting the user to > create a new repository (link and service message). Also this method of telling > the user to create something is not used much elsewhere in Eclipse (AFAIK only > in the Task List). There is already a local toolbar button to create a new > repository. Do we really need the link? It is not uncommon for Eclipse views to help the user with the initial setup. The Search view and Synchronize view use the same mechanism for instance. My sense was that the service message was to add a specific server whereas the link is for general setup. I'll try it here to get a sense of how it feels. Thanks for the patch. That looks very good, I only have two minor nits: * BuildServiceMessageControl has an e.printStackTrace(). * The synchronization in notify() won't guard against concurrent access to messages since the asyncExec() will run on a separate thread.
(In reply to comment #22) > It is not uncommon for Eclipse views to help the user with the initial setup. > The Search view and Synchronize view use the same mechanism for instance. > > My sense was that the service message was to add a specific server whereas the > link is for general setup. I'll try it here to get a sense of how it feels. Ok... I'm leaning towards using the service message framework for all messages directly related to that specific view. Although I do agree with you that the service message component is a bit loud. Maybe we could tone it down a bit? > > Thanks for the patch. That looks very good, I only have two minor nits: > * BuildServiceMessageControl has an e.printStackTrace(). > * The synchronization in notify() won't guard against concurrent access to > messages since the asyncExec() will run on a separate thread. Thanks for pointing this out. I will take another look.
Created attachment 180244 [details] Improvements to logging and concurrency
Thanks for the updated patch. I have applied it with some minor modifications. I have kept the original view presentation that shows a link instead of the service message when no servers are configured. I really don't like the look of the empty table when the view is first opened and the service message has a slightly different purpose than the configuration prompt. I'll put this up for UI review for the next meeting soi we can get some input from other committers on that.
Sounds good. Thanks.
