Community
Participate
Working Groups
Currently the editors all submit changed tasks in their own unique way. We need to unify this to eliminate duplicate code and make submission of changes possible from outside of the editor. For example, we need to be able to submit priority changes made within the tasklist (bug#124224).
As part of this I want to eliminate the need for setting task data to null before submission. Note to self: ensure that by fixing this we don't loose the check for overwrite upon synching tasks with outgoing.
That would also help to submit activate/deactivate events...
Do you mean marking complete/incomplete?
No. It is JIRA feature (sort of time tracking thing). We have request to link it with Mylar's task activity. 144620: setting task as active should be reflected in JIRA https://bugs.eclipse.org/bugs/show_bug.cgi?id=144620
Update: I've pulled up the necessary parts of task submission up into AbstractRepositoryTaskEditor. There is now a postTaskData method on the ITaskDataHandler interface (formerly IOfflineTaskHandler). This method is used by AbstractRepositoryTaskEditor.submitToRepository so needs to be implemented for connectors with rich editors. In addition attachContext has been moved up and other task submission related code so we should be able to use this rather than implement for each new editor. These changes also take care of the "setting task data to null HACK" and improves task submission/synchronization handling. Also note that where RepositoryAttachment once held onto instances of TaskRepository, it now just hold the data necessary to get the attachment, repositoryUrl, type, taskid (for trac), and attachment id. Note that I left the other connector editors as is, overriding submitToRepository.
The new API is great. It reduces the Trac task editor to less than 60 lines of code! I discovered a few problems though while trying to implement it: * RepositoryTaskData.isNew() always returns false therefore it is not possible to distinguish new from existing tasks in postTaskData(). If at all possible I would not want to extend RepositoryTaskData (as Bugzilla does) just to overwrite that single method. * The status object in the CoreException handled by submitToRepository() is expected to have the severity set to IStatus.OK and use the plug-in specific code attribute to indicate an error. I think that is counter-intuitive, IStatus.ERROR should be used instead and the error handling could be done by submitJob() instead of the job listener. * handleSubmitError() should not use MylarStatusHandler to display the error since the CoreExceptions are not caused by programming errors but intermittent network failure etc (see attached screenshot).
Created attachment 54943 [details] Error when creating task without being connected
Steffen, I agree with your points from comment#6. If possible I would like to defer these changes so I don't destablise this close to 1.0. I gather this will mean you will have to stick with your current overriding of submitToRepository?
My goal was to fully support the Mylar API in the Trac connector. I will submit an implementation for postTaskData() but not use it in the editor and document why I didn't. We should make fixing this a high priority once the release is out.
Yes, I really wanted this cleaned up for 1.0 too (we're SO close!) but it's just getting too close to the release. As a policy I'm giving P2 tasks attention right after the release (this will be first in line).
bug#169453 is now resolved...there are no children of RepositoryTaskData and the class has a setNew(boolean) method. isNew() by default returns false. You were right about error dialog not being displayed in 3.3 when the job returns an ERROR status but apparently this is a bug (bug#168720).
Further to our discussion on the last conference call... Steffen, you were suggested implementing a Status with constants similar to TeamStatus and ITeamStatus (correct me if I'm wrong ). The constants can reflect how the error is displayed such as in a web dialog, Info, warning, etc..
Update: submitToRepository refactored and Error handling being worked on. Will likely commit tomorrow. Made an ITaskStatus interface that contains: /** * Operation failed with error from repository */ public final static int REPOSITORY_ERROR = IStatus.CANCEL << 1; /** * Operation failed with html error from repository */ public final static int REPOSITORY_ERROR_HTML = IStatus.CANCEL << 4; /** * Operation failed due to network error */ public final static int NETWORK_ERROR = IStatus.CANCEL << 5;
That is: public final static int REPOSITORY_ERROR = IStatus.CANCEL << 1; public final static int REPOSITORY_ERROR_HTML = IStatus.CANCEL << 2; public final static int NETWORK_ERROR = IStatus.CANCEL << 3; In the task editor, upon submitting a task, specifying a status code of REPOSITORY_ERROR_HTML on the error status results in the WebBrowserDialog displaying the content of the status.getException().getMessage(). REPOSITORY_ERROR and NETWORK_ERROR result in MessageDialog.openError displaying status.getMessage().
Great Rob, that's exactly what I had in mind. Why did you shift IStatus.CANCEL instead of just assigning distinct values to the error constants? Which class will be responsible for displaying the different error dialogs? MylarStatusHandler? Other constants that would be helpful: LOGIN_ERROR, REPOSITORY_NOT_FOUND, INTERNAL_ERROR .
(In reply to comment #15) > Why did you shift IStatus.CANCEL > instead of just assigning distinct values to the error constants? This was to continue the bitmask progression already in IStatus. CANCEL is the highest mask in Status (and is final). > Which class > will be responsible for displaying the different error dialogs? > MylarStatusHandler? Yes this seems like a reasonable place to me. > Other constants that would be helpful: LOGIN_ERROR, REPOSITORY_NOT_FOUND, > INTERNAL_ERROR . My thinking is that these are context specific instances of either REPOSITORY_ERROR or NETWORK_ERROR. My goal is not to duplicate what the exception hierarchy already provides. I'm not quite sure yet where the balance lies. Could you give an example of how these more specific error codes would be used?
(In reply to comment #15) > Which class will be responsible for displaying the different error dialogs? Actually I'll place the error display handling code in TasksUiUtil since MylarStatusHandler is in context core and we need access to the UI.
I guess one example of the use of more specific error codes might be if we had a constant like NEW_COMMENT_REQUIRED, then upon getting this error the AbstractRepositoryTaskEditor could display the error dialog but then follow up by setting focus to the new comment field.
Okay bad example...this type of error should be handled by the editor.
(In reply to comment #16) > (In reply to comment #15) > > Why did you shift IStatus.CANCEL > > instead of just assigning distinct values to the error constants? > This was to continue the bitmask progression already in IStatus. CANCEL is the > highest mask in Status (and is final). I thought these codes were intended to be used as the plug-in specific error code that is generally unrelated to the severity. Regarding the problem with the error dialog that is displayed by the platform when a job returns an error status: Could we completely move the submit job listener code to the submit job itself or is there a particular reason to use a listener (we could put it in a protected method to allow connectors to override)? > > Other constants that would be helpful: LOGIN_ERROR, REPOSITORY_NOT_FOUND, > > INTERNAL_ERROR . > My thinking is that these are context specific instances of either > REPOSITORY_ERROR or NETWORK_ERROR. > My goal is not to duplicate what the exception hierarchy already provides. Would that force connectors to set the exception field to a LoginException in case of a failed login? I think that is problematic since the underlying communication library could use a totally different mechanism to indicate a failed login. I would prefer a more abstract / generic error indication mechanism for Mylar with a single place (ITaskStatus) that provides all information for creating IStatus objects. So far I have always had to check the error handling code to figure out how exceptions are actually handled. BTW, JavaModelStatus.getMessage() also derives the message from the error code. I think we should do the same for Mylar if the message is set to null.
> Could you give an example of how > these more specific error codes would be used? In case of a login error the repository properties could be displayed.
(In reply to comment #20) > I thought these codes were intended to be used as the plug-in specific error > code that is generally unrelated to the severity. I see... and plugins will override getMessage() on their own Status objects. > Regarding the problem with the error dialog that is displayed by the platform > when a job returns an error status: Could we completely move the submit job > listener code to the submit job itself or is there a particular reason to use a > listener (we could put it in a protected method to allow connectors to > override)? Done. > I would prefer a more abstract / generic error indication > mechanism for Mylar with a single place (ITaskStatus) that provides all > information for creating IStatus objects. Sounds like a good idea.
Should we have a MylarStatus object (or TaskStatus) that overrides getMessage() and builds the necessary dialog message from a .properties file (to JavaModelStatus) based on the specified code? Then it would just be a matter of knowing what string to pass to the MylarStatus object under various error conditions.
The api thus far is like so: For example, upon failure to authenticate with the repository... throw new CoreException(new MylarStatus(Status.ERROR, BugzillaCorePlugin.PLUGIN_ID, IMylarStatusConstants.REPOSITORY_LOGIN_ERROR, repositoryUrl.toString())); The corresponding entry in MylarMessages.properties is repository_login_failure = Unable to login to {0}. \n\nPlease validate credentials via Task Repositories view. And this results in the passed in repositoryUrl being inserted in this string and displayed in the Message dialog (in this case .openError)
Note to self: The correct way to produce the dialogs may be through improving RepositoryAwareStatusHandler or adding an additional status handler.
Yes, that looks exactly right. Does it makes sense to have a setRepositoryUrl() method to make it more obvious how to pass the url? I guess most of the errors will have an associated repository.
Yes, perhaps we should also have a taskId field as well....
Just running out the door here... committed what I have so far. I'm using MylarStatus in Bugzilla connector now. Steffen, perhaps you can take a pass through the Trac connector and see how the new MylarStatus works for you? You should now be able to implement postTaskData and stop overriding submitToRepository in the editor etc as well. :)
Eugene, note that these changes to error handling have been committed and all tests pass but little manual testing has been done on Web and JIRA connectors.
Rob, sorry, I missed most of the discussion. Is it about submitting new tasks, new comments or attachments? What features should I pay attention to or retest?
Steffen and I have been hashing out how to improve error status construction and display in the ui. I'll write something up on the wiki and send an email to the dev list Monday/Tuesday. Just look for error handling oddities of any sort stemming from Bugzilla operations. We should now get proper dialogs upon submit errors / authentication errors for the bugzilla connector (vs. the ugly mylar status dialog we have been using lately). Steffen is going to do a pass through Trac making use of MylarStatus and I'll make any changes necessary based on his feedback. Then we'll have something solid for you to apply to the Web connector's error reporting (and iterate). I still have some work to do like move dialog ui code into the RepositoryAwareStatusHandler etc but the main parts are all there (see MylarStatus, IMylarStatusConstants, MylarMessages.properties if interested).
See bug 170388, not sure if it caused by your refactorings
Likely. I'll have a look...
I have committed a partial implementation for the task submission support for Trac (which basically means I have removed most of the code from the Trac editor implementations). I am still trying to figure out how to make the API of the MylarStatus object simpler and less breakable without loosing the benefit of unified error messages and handling. I have created a TracStatus object for now to play with API a bit.
Okay great!
Moved error displaying code to ReposiitoryAwareStatusHMylarStatusHandler. Error status now displayed via MylarStatusHandler.displayStatus(<DIALOG TITLE>, <MylarStatus>);
I think we can close this bug as an interface for task submission has been defined in ITaskDataHandler and implemented for Bugzilla and Trac. I have created bug 170536 to discuss improvements to error handling.
I agree. Marking resolved.
Created attachment 56932 [details] mylar/context/zip
