Bugzilla – Full Text Bug Listing
| Summary: | "editor has new changes" warning should provide hyperlink to update editor | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Eugene Kuleshov <ekuleshov> | ||||||||||
| Component: | Mylyn | Assignee: | Eugene Kuleshov <ekuleshov> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | minor | ||||||||||||
| Priority: | P4 | CC: | mik.kersten, robert.elves, shawn.minto | ||||||||||
| Version: | unspecified | Keywords: | helpwanted, noteworthy | ||||||||||
| Target Milestone: | 2.3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Eugene Kuleshov
Marking for bugday since this could be a fun and straightforward contribution for the next Bug Day. See also related bug 183229. Apparently is may not be as easy as it seemed. I took a quick look at the forms api but couldn't find how to set a message with the hyperlink on the form header. The only hyperlink they have in API is used to show a drop down with detail messages linked to the form fields. So, I am not sure what to do about this. Possible options: a) add another action to the form editor toolbar - update editor; b) change "Synchronize" action to not actually run synchronize if there are incoming changes, though I am not sure how to check if task have incoming changes (Rob, any clue?) c) add some custom component to the form header that will be able to show custom hyperlinks Created attachment 83127 [details]
mylyn/context/zip
Eugene: thanks for the exploration, that's valuable information. The current UI seems sufficient to me, since the common action that needs to be taken by the user (Synchronize) is immediately to the right of the message. This also has the effect of teaching the user how to use that button. The hyperlink would be nice but seems like very minor sugar, so reducing priority. (In reply to comment #5) > The current UI seems sufficient to me, You probably on a very fast connection to your issue trackers. I am seeing very annoying delay in 3..4 seconds or longer for slow repositories, because "refresh" button does server round trip (totally unnecessary in my opinion and hence suggestion b) above) before it brings new changes to the editor. So it is nothing like a sugar, but quite significant UI responsiveness issue. Created attachment 84696 [details]
added hyperlink
Apparently I was wrong and hyperlink is in fact can be added. So, here is very simple patch that adds it.
P Patch applied and verified. Thanks Eugene. Good to see that forms have built in support for this. Apparently there is a weird side effect. After clicking on newly introduced hyperlink changes are picked up, but they are not highlighted. Eugene: will you be investigating this or should I ask someone else to? Rob: apologies for not having sufficiently verified the patch, I thought that the right method was being called but did not inspect carefully enough. After testing it more thoughtfully id does work fine on a first click and new comments are highlighted fine. But if you leave editor open and and new change come trough, and you click refresh, the whole comment area is collapsed and new comments aren't highlighted. The patch added call to AbstractRepositoryTaskEditor.refreshEditor() when hyperlink is clicked after task change been detected. Rob, is that the right api to use? I also noticed couple more issues: - after AbstractRepositoryTaskEditor.refreshEditor() is called, incoming decoration is not cleared from the task in Task List - if I synchronize entire task list using toolbar button and task is only present in some category (not query), that task gets incoming decoration, but editor don't get notification about incoming changes. If you select and synchronize specific task, then editor is notified. Why don't you call synchronizeEditorAction.run() instead? That should have the same effect as pressing the synchronize button in the toolbar or is that broken as well? I think SynchronizeEditorAction has the logic that is required to merge changes into the task data object before refreshing the editor input. Task data is already available locally, but synchronizeEditorAction will take another unnecessary roundrip to the server. I wanted to avoid that. Sorry, please disregard comment 13. My brain was running out of sugar. The current refresh is much better and way faster. Shawn points out that after an additional hyperlink listener is added each time the message for an incoming notifications is displayed. That means after the first refresh there are two listeners so the editor is refreshed twice. The second refresh wipes out the incoming decorations. Good catch!
I guess we'll have to set hyperlink upon editor creation, but then it would appear for all the other messages that could be set. Here are messages that being set by AbstractRepositoryTaskEditor:
** 414: parentEditor.setMessage("Task has incoming changes, synchronize to view", IMessageProvider.WARNING);
601: parentEditor.setMessage("Task data not available. Press synchronize button (right) to retrieve latest data.", IMessageProvider.WARNING);
2,571: parentEditor.setMessage(job.getStatus().getMessage(), IMessageProvider.ERROR);
3,509: parentEditor.setMessage("Task data not available. Press synchronize button (right) to retrieve latest data.", IMessageProvider.WARNING);
3,593: parentEditor.setMessage(ERROR_NOCONNECTIVITY, IMessageProvider.ERROR);
If you guys think it ok to have link that would refresh editor from all of those messages, I'll can submit another patch or you can move the following code
parentEditor.getTopForm().addMessageHyperlinkListener(new HyperlinkAdapter() {
public void linkActivated(HyperlinkEvent e) {
refreshEditor();
}
});
into AbstractRepositoryTaskEditor.addHeaderControls() method.
If refreshing on all those messages is not a good idea, then there are couple of other options:
- replace parentEditor.setMessage() calls with something that would set or unset corresponding link listener (to handle scenario when second message appear while some other message is shown).
- add logic to hyperlink listener to invoke appropriate actions, i.e. refresh vs. full sync. Maybe we should make "Task data not available", a error, then we can call refresh() on all warnings and sync on all errors (otherwise we'll have to do comparison on text message). I am leaning towards this option.
Also, it seem like these messages need to be updated (i.e. to take hyperlink into the account).
- replace parentEditor.setMessage() calls with something that would set or unset corresponding link listener (to handle scenario when second message appear while some other message is shown). +1 lets make the method private if possible but that might become API in the future (In reply to comment #18) > +1 lets make the method private if possible but that might become API in the > future It won't be pretty and also nothing stop anyone to call parentEditor.getTopForm().setMessage() or set hyperlink directly. Created attachment 87731 [details]
fix
I have committed this fix for tonight's weekly build. Eugene, please review.
Created attachment 87732 [details]
mylyn/context/zip
(In reply to comment #20) > I have committed this fix for tonight's weekly build. Eugene, please review. That might work. One thing I am not sure about is what if two TaskEditor.setMessage() calls would come at the same time, would it lose listener instance? Also, listener probably need to be unregistered in dispose() method. (In reply to comment #22) > That might work. One thing I am not sure about is what if two > TaskEditor.setMessage() calls would come at the same time, would it lose > listener instance? What to you mean by the same time? All calls have to be made in the UI thread and are therefore sequential. The later call would overwrite the previous message and listener. > Also, listener probably need to be unregistered in dispose() method. Do you have an example of platform code where listeners are unregistered in dispose()? I would assume that it lies in the responsibility of the listener to properly remove itself. The lifetime of the existing listeners is scoped by the lifetime of the task editor so the garbage collection will pick everything up once the editor is disposed. On a second thought that code is maybe running in the UI thread. (In reply to comment #23) > > Also, listener probably need to be unregistered in dispose() method. > > Do you have an example of platform code where listeners are unregistered in > dispose()? A plenty. For instance AntEditorContentOutlinePage.dispose() > I would assume that it lies in the responsibility of the listener to > properly remove itself. The lifetime of the existing listeners is scoped by the > lifetime of the task editor so the garbage collection will pick everything up > once the editor is disposed. The problem is not with the dispose or listener itself but with GC. Listener instance could prevent objects from being garbage collected and lead to the memory leaks. I don't know if it is the case here, but I believe it is a good practice to clean up listeners on disposal. > The problem is not with the dispose or listener itself but with GC. Listener
> instance could prevent objects from being garbage collected and lead to the
> memory leaks. I don't know if it is the case here, but I believe it is a good
> practice to clean up listeners on disposal.
In most cases references to listeners will only be held by the instance that manages the life cycle of the UI widget and in those cases I am against adding superfluous remove calls in dispose which add additional code that needs to be maintained. In the case of the editor removing the listener might be a good idea though since it is not obvious to the caller how to remove the listener and we can't make any guarantees about the lifetime of the listener.
Marking as resolved. |