Bugzilla – Full Text Bug Listing
| Summary: | [api] add additional IO support to import/export of tasks | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Remy Suen <remy.suen> | ||||||||
| Component: | Mylyn | Assignee: | Remy Suen <remy.suen> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P2 | CC: | caniszczyk, robert.elves, steffen.pingel | ||||||||
| Version: | dev | Keywords: | helpwanted | ||||||||
| Target Milestone: | 2.3 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 213743 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Remy Suen
Hmm. I've been complaining that this api should not be File-centric and somehow thought it is fixed already. To put some context into this request, the new API addition would make the implementation of bug 195737 cleaner. It'd be nice if some of the other java.io.File methods like TaskListWriter's readRepositories(File) and InteractionContextManager's loadContext(String, File) and loadContext(String, File, InteractionContextScaling) were changed too. I can file separate bugs for those (since they're not necessarily dealing with "tasks" per se), but I'll see how far this api request flies first. Yup, a stream-based API would be more flexible. Remy: are you interested in contributing this? If so I can give you pointers on how to proceed on making this stream based. InteractionContextManager changes are also possible, but will be a bit tricker. Please file a single new bug report for making InteractionContextManager's I/O stream-based. Created attachment 82592 [details] Patch to add two new methods to TaskListWriter that takes an OutputStream. (In reply to comment #4) > Remy: are you interested in contributing this? Yes, I would be. > If so I can give you pointers on > how to proceed on making this stream based. Not sure if the attachment > InteractionContextManager changes > are also possible, but will be a bit tricker. Please file a single new bug > report for making InteractionContextManager's I/O stream-based. Will do. Do you want a separate bug for TaskListWriter's readRepositories(File) or...? Created attachment 82593 [details]
Mylyn task context for this bug.
(In reply to comment #5) > > If so I can give you pointers on > > how to proceed on making this stream based. > > Not sure if the attachment I meant to say, I'm not sure if the attached patch suits your needs. If not, let me know. I didn't add @since tags since I'm not sure where Mylyn is headed. +1 Any updates on this? Also, did you guys want me to file a separate bug for TaskListWriter's readRepositories(File) or should that be resolved here or merged with InteractionContextManager's (which I have not yet filed because I am waiting on this confirmation)? Remy, this is looking good. Would you be interested in extending this patch to address the readRepositories() method along with the others that don't take streams within this class? Then we won't have to revisit this again. We could then address the InteracitonContextManager's methods on a separate report. Rob, Remy: please make sure that any changes have adequate test coverage. Rob, let's pair on this and review the complete TaskListWriter API for 2.3 to make it stream-based with proper exception handling. The review of the task list serialization code indicated that the task list serialization and import/export code probably needs a larger refactoring. The current logic is spread over multiple classes and duplicates quite a bit of code. In order to design public API to unify task list externalization, import/export and the requirements for this bug it would be helpful to get a better sense of the underlying use case. As far as I understand bug 195737 the goal is to share task list elements through ECF? Essentially that would be very similar to import/export and require serialization of multiple entities that are associated with a task and handled by different sub-systems in Mylyn: * Task * Task Data (for rich editing / offline support) * Task Context * Task Repository Instead of extending TaskListWriter Rob and I discussed introducing a new class that serves as a facade to TaskListWriter, TaskRepositoriesExternalizer, TaskDataManger and InteractionContextManager. The class could be made public API and would support import and export of these entities through a single stream. The interface could look similar to this: class TaskExternalizer { void write(AbstractTaskContainer[] items, OutputStream out, IProgressMonitor monitor); Result read(InputStream in, IProgressMonitor monitor); } class Result { Status getStatus(); void commit(); } The Result class would provide facilities for user interaction in case of problems (e.g. when a task is imported that already exists). The implementation would require significant effort though and we won't be able to do this for 2.3. We might be able to get to it for 3.0 but we need to figure out how that fits into other priorities. Remy, we can make the changes proposed in your patch for Mylyn 2.3 (Europa spring update) but either way the (internal) API is almost certainly going to change for Mylyn 3.0 (Ganymede). Let me know what you think. (In reply to comment #13) > As far as I understand bug 195737 the goal is to share > task list elements through ECF? That's the basic gist of it. Just sending task list stuff around. > class Result { > > Status getStatus(); Return IStatus instead of Status? > void commit(); What is this committing exactly? Doing the actual writing onto the OutputStream? > Remy, we can make the changes proposed in your patch for Mylyn 2.3 (Europa > spring update) but either way the (internal) API is almost certainly going to > change for Mylyn 3.0 (Ganymede). Let me know what you think. Just getting my suggested changes in for 2.3 would be a huge boon. If things are going to change for 3.0, so be it, I don't really have a problem with that. All the Eclipse projects that are a part of Ganymede are meant to work together, so if you guys feel the need to make changes, I'll change the code on our side to adapt accordingly so it all falls into place. :) (In reply to comment #14) > Return IStatus instead of Status? Makes sense. > > void commit(); > > What is this committing exactly? Doing the actual writing onto the OutputStream? My idea is to make the import a two step process. First read the information from the input stream and create a Result object that holds the parsed data and information about potential conflicts. In case of conflicts some type of dialog is presented to the user to allow for manual resolution of conflicts (e.g. replace existing tasks). In a second step the Result object is "committed" to the task list which adds the actual tasks. > Just getting my suggested changes in for 2.3 would be a huge boon. If things are > going to change for 3.0, so be it, I don't really have a problem with that. All > the Eclipse projects that are a part of Ganymede are meant to work together, so > if you guys feel the need to make changes, I'll change the code on our side to > adapt accordingly so it all falls into place. :) Alright. I think it's great that Ganymede projects integrate beyond being installable into the same instance of Eclipse :). Rob, do you have objections to merging the patch for 2.3? (In reply to comment #15) > ... In case of conflicts some type of dialog > is presented to the user to allow for manual resolution of conflicts (e.g. > replace existing tasks). It should be possible to make this dialog optional or have some kind of pluggable call back, so IO stuff could work in a headless mode (i.e. when materializing task list for new user). (In reply to comment #15) > Rob, do you have objections to merging the patch for 2.3? No objections here. Lets go ahead with this patch and see if we can make progress on the larger scale externalization refactoring for 3.0. Created attachment 88957 [details]
updated patch
Patch applied, ip log updated. Marking resolved. Further improvements to externalization apis tracked on bug#217953. |