Bug 364155

Comment 1 Steffen Pingel 2011-11-18 09:43:06 EST

Shawn, could you take a look if the proposed changes look good to you?

Comment 2 Steffen Pingel 2011-11-20 07:11:28 EST

We need to have this information available on task deactivation to persist the active branches, right? 

One concern could be the coupling from the EGit Mylyn integration to Mylyn Context which we may want to avoid. I don't think it's a concern right now since some of the team components are still in Context anyways but we may need to revisit this in the future. We could enhance the notion of what a context is in Tasks (right now we only have a context store which is file based but agnostic to the actual content of the context). The first step could be an extension in Tasks that provides a mapping from task -> resources (projects) with an implementation in context that does that based on interaction history.

Comment 3 Shawn Minto 2011-11-21 12:00:15 EST

The basics of this patch seem right.  My only concern is making context require o.e.c.resources as it is designed to not have any resource couplings.  I wonder if this should be in the resource structure bridge somewhere and then only look for projects (i.e. look specifically at the handles to determine if it is a project or not) as for a large context this could be a long running operation as it iterates over all elements in the context and could cause java structure bridge loading.

Comment 4 Manuel Doninger 2011-12-23 11:05:17 EST

So to determine, if a handle is a project, i would check if the handle contains only one path segment, like in ResourceStructureBridge#getObjectForHandle()? And if it contains more than one segment, it would be a resource, which i could ignore?

Comment 5 Manuel Doninger 2011-12-24 16:36:27 EST

Created attachment 208797 [details]
mylyn/context/zip

Comment 6 Steffen Pingel 2011-12-25 04:54:31 EST

That sounds right to me.

Comment 7 Manuel Doninger 2011-12-25 10:37:48 EST

I pushed a second draft to Gerrit (http://review.mylyn.org/#change,180,patchset=1), where i put the method in the ResourceStructureBridge, so that the context doesn't get dependencies to o.e.c.resources

Comment 8 Steffen Pingel 2011-12-26 07:42:57 EST

Looks fine to me. We just need one or more test cases in order to apply to master.

Comment 9 Manuel Doninger 2011-12-26 15:49:59 EST

I'm working on some test cases. I think i should be able to finish them at the end of the week.

Comment 10 Manuel Doninger 2011-12-26 16:57:46 EST

Created attachment 208808 [details]
mylyn/context/zip

Comment 11 Manuel Doninger 2011-12-27 08:03:19 EST

Ok, i've added a small test just to check if there are no duplicate project entries in the result. If you need more tests, then just give me a hint. Right now, i don't have an idea, what else i could test.

Comment 12 Manuel Doninger 2011-12-27 08:03:24 EST

Created attachment 208813 [details]
mylyn/context/zip

Comment 13 Steffen Pingel 2012-01-03 18:27:49 EST

Thanks for the updated change set! It'd be great if addressed the comment in the review and attached a patch so that we can merge it and track the contribution in the IP log.

Comment 14 Manuel Doninger 2012-01-04 17:01:47 EST

Created attachment 209036 [details]
Patch

Comment 15 Manuel Doninger 2012-01-04 17:01:52 EST

Created attachment 209037 [details]
mylyn/context/zip

Comment 16 Manuel Doninger 2012-01-04 17:02:53 EST

I made some changes according to your comments (removed logging, since no checked exceptions are thrown, added checking, if we are adding real projects).

Comment 17 Steffen Pingel 2012-01-05 15:01:30 EST

Looks good to me. Only two minor nits:

* ResourceStructureBridgeTest should be added to the All*Tests suite.
* Instead of using List<IProject> projectsInWorkspace = Arrays.asList(workspace.getRoot().getProjects()) could we invoke project.exists() before adding the project to the result collection?

Comment 18 Manuel Doninger 2012-01-05 15:21:13 EST

Created attachment 209100 [details]
Edited according to last comment from Steffen

Comment 19 Manuel Doninger 2012-01-05 15:21:16 EST

Created attachment 209101 [details]
mylyn/context/zip

Comment 20 Shawn Minto 2012-01-09 13:06:10 EST

Patch committed.