Community
Participate
Working Groups
This is a follow up bug, related to bug 326926. Bug 326926 provides an API for obtaining a project reference from an SCMURL. This request is to provide a user interface that allows the user to further configure a set of SCMURLs for import. For example, a user might want to import a project from HEAD vs. a specific version when importing from CVS. I would suggest an API that allows a team provider to contribute a wizard that is capable of configuring a set of SCMURLs (or URIs). The client would pass in the original set of URIs when opening the wizard and the wizard could provide the modified set of URIs back to the client. This would allow clients to configure a set of URIs to import and then use the core team API to perform the actual import.
I talked today with Ankur about how this could look like. We agreed on most parts, but after considering it again I think there are stills some aspects to discuss: * We agreed that the UI that allows the user a set of SCM URLs to import should be provided by repository providers. For instance, for CVS the user might decide to import a project from HEAD or using a specific tag * Currently the configuration can is done using an implementation of IBundeImportWizardPage (e.g. CVSBundleImportPage) * The page can be part of PluginImportWizard or RepositoryImportWizard * I suggested that we could reuse the existing wizard in Team (ProjectSetImportWizard), make it accept SCM URLs as input and add PDE-like pages for each provider in the set. Then PDE could either open the Team's wizard when needed (i.e. in org.eclipse.pde.internal.ui.views.plugins.ImportActionGroup.handleImport(int, IStructuredSelection), combine their wizard with Team's (PluginImportWizard + ProjectSetImportWizard) or just ask Team for the pages to init their wizards.
(In reply to comment #1) > * I suggested that we could reuse the existing wizard in Team > (ProjectSetImportWizard), make it accept SCM URLs as input and add PDE-like > pages for each provider in the set. Then PDE could either open the Team's > wizard when needed (i.e. in > org.eclipse.pde.internal.ui.views.plugins.ImportActionGroup.handleImport(int, > IStructuredSelection), combine their wizard with Team's (PluginImportWizard + > ProjectSetImportWizard) or just ask Team for the pages to init their wizards. I think it is best for PDE to be able to add the wizard pages to our current import wizard. All of your comments are reasonable and in line with what we want. Can you provide a rough timeline for this work? If we can define the API earlier in M5 we would have more time to create an example implementation (perhaps using our current CVS import wizard). We also have several other bugs related to this import feature that we would like to fix at the same time (in case the api needs some tweaking). If it is a matter of resources, Ankur or myself can put some time towards creating a possible API.
(In reply to comment #2) > I think it is best for PDE to be able to add the wizard pages to our current > import wizard. Fine with me. > Can you provide a rough timeline for this work? I would like to start working on the bug as soon as we (Team) are done with bug 315694 and bug 327785 (backports can wait). I think we're almost there. > If it is a matter of resources, Ankur or myself can put some time towards > creating a possible API. Thanks, I will ping you in case of any doubts. I will submit a first version of the patch as soon as possible so you can comment on it. In other words, I will update the bug as soon as I have something worth sharing.
Created attachment 185434 [details] Fix v01 Here's an initial version of the fix. The new API in Team is made of 2 parts: * new extension point - org.eclispe.team.ui.scmUrlImportWizardPages, which replaced org.eclipse.pde.ui.bundleImportPages. Pages added in the extension should implement IScmUrlImportWizardPage interface which is almost identical to IBundleImportWizardPage from pde.ui. The main difference is that the interface from team.ui accepts URIs not BundleImportDescriptions. * new API method in team.ui - org.eclipse.team.ui.TeamUI.getPages(URI[]) (the name is subject to change) which returns an array of contributed IScmUrlImportWizardPages for the given SCM URIs. The pages are already populated with the URIs. It's the first thing that came to my mind. If you think it should be changed, just let me know. I'm sure we'll work it out. CVS/UI provides a scmUrlImportWizardPages extension. A concrete implemention of IScmUrlImportWizardPage is made in CVSScmUrlImportWizardPage which basically CVSBundleImportPage moved to CVS. I also made some changes in pde.core and pde.ui so both wizards (PlugImportWizard and RepositoryImportWizard) consume now the new API. I must admit the code is a little bit messy but at least it works, so I hope it can serve as a starting point. I'm open to any comments, especially regarding the new API. For instance, I'm not convinced if URI[] is the best type to use, what do you think? The patch contains changes for both team.* and pde.*.
Created attachment 185435 [details] mylyn/context/zip
thanks for doing the pde side changes too :-) couple of points to discuss 1. We should be re/moving 'bundleImporters' Ext. Pt. from pde.core and thus CvsBundleImportDescription and CvsBundleImporterDelegate too. I suppose you are already on it. I feel PDE should not be knowing anything about CVS 2. The new API getPages can accept String[] instead. For the plugin org.eclipse.equinox.http.jetty which has the scmurl as 'scm:cvs:pserver:dev.eclipse.org:/cvsroot/rt:org.eclipse.equinox/compendium/bundles/org.eclipse.equinox.http.jetty6;project="org.eclipse.equinox.http.jetty";tag=v20100503' Now the quotes in the project tags throws an URISyntaxException. May be we can have an internal method in team to 'clean' such urls instead of failing.
(In reply to comment #6) > thanks for doing the pde side changes too :-) couple of points to discuss No worries. I did that to verify if the new Team API is any good. > 1. We should be re/moving 'bundleImporters' Ext. Pt. from pde.core and thus > CvsBundleImportDescription and CvsBundleImporterDelegate too. Agree. Eventually, we should be able to get to the point where the action required for each provider would be the same: get configured URIs from the wizard and then do import on the String returned from CVSProjectSetCapability.asReference(URI, ...). CvsBundleImportDescription will also became obsolete once Team is only interested in URIs (or Strings representing the URIs, as you suggested below). > I suppose you are already on it. I feel PDE should not be knowing anything about CVS I didn't plan to do any more changes on the PDE side, I think I messed up the code enough ;) > 2. The new API getPages can accept String[] instead. For the plugin > org.eclipse.equinox.http.jetty which has the scmurl as > 'scm:cvs:pserver:dev.eclipse.org:/cvsroot/rt:org.eclipse.equinox/compendium/bundles/org.eclipse.equinox.http.jetty6;project="org.eclipse.equinox.http.jetty";tag=v20100503' > Now the quotes in the project tags throws an URISyntaxException. May be we can > have an internal method in team to 'clean' such urls instead of failing. What do you mean by "'clean' such urls"? Should Team API ignore such String or should it try to strip off the problematic "s before creating an URI for it.
(In reply to comment #7) > I didn't plan to do any more changes on the PDE side, I think I messed up the > code enough ;) I will take care of rest :) > > 2. The new API getPages can accept String[] instead. For the plugin > > org.eclipse.equinox.http.jetty which has the scmurl as > > 'scm:cvs:pserver:dev.eclipse.org:/cvsroot/rt:org.eclipse.equinox/compendium/bundles/org.eclipse.equinox.http.jetty6;project="org.eclipse.equinox.http.jetty";tag=v20100503' > > Now the quotes in the project tags throws an URISyntaxException. May be we can > > have an internal method in team to 'clean' such urls instead of failing. > > What do you mean by "'clean' such urls"? Should Team API ignore such String or > should it try to strip off the problematic "s before creating an URI for it. By cleaning I mean Team shall take care of such cases as well. SCM URL provided by PDE or anyone might not be URI but is surely formatable into one. So instead of failing or missing, it shall be handled for quote characters and any other such similar slight deviation. Hope am making sense :)
(In reply to comment #6) > 2. The new API getPages can accept String[] instead. On second thought, I must say I feel sceptical about this approach. I don't think String[] is the most appropriate parameter for this method. imo, we should use most specific possible input parameter type, which would move error from runtime to compile time. Strings are cumbersome, error-prone and slow, so when possible they should be avoided. I'm not sure whether we should use URI[] or URL[] though. The later is in line with the "SCM URL" term but the former seems like the right choice. Also, should the method return pages for different providers or should it expect that the given array (String[], URI[], URL[] or whether we decide to use) holds only references from a single provider (eg. CVS SCM URLs only)? Lastly, should the return pages be already initialized with the passed refs, or should I leave it to the client, so he can call page.setSelection(...) or something similar? Currently, the method accepts mixed refs and returns init'ed pages for all providers.
(In reply to comment #9) > (In reply to comment #6) > > 2. The new API getPages can accept String[] instead. > > On second thought, I must say I feel sceptical about this approach. I don't > think String[] is the most appropriate parameter for this method. imo, we should > use most specific possible input parameter type, which would move error from > runtime to compile time. Strings are cumbersome, error-prone and slow, so when > possible they should be avoided. I'm not sure whether we should use URI[] or > URL[] though. The later is in line with the "SCM URL" term but the former seems > like the right choice. I agree the String is not an apt choice. But one problem with URL type is that, if we can not convert it from String to URL (like the one in comment 6 due to quotes), then what do we do. It will not be good for PDE to know the format of the URL and handling such cases (thats what I meant by cleaning a url). > Also, should the method return pages for different providers or should it expect > that the given array (String[], URI[], URL[] or whether we decide to use) holds > only references from a single provider (eg. CVS SCM URLs only)? It should accept an array of String or URL (whichever we decide) and return an array of pages >Lastly, should > the return pages be already initialized with the passed refs, or should I leave > it to the client, so he can call page.setSelection(...) or something similar? > Currently, the method accepts mixed refs and returns init'ed pages for all > providers. The current implementation is good and let the pages come initialized. We also need the Team API for BundleImportDescription and IBundleImporter for finally importing the plug-ins.
(In reply to comment #10) > We also need the Team API for BundleImportDescription Wouldn't a SCM URL (whether it's URI/URL or a String representing the URI) be enough here? Team is not interested in any other information hold in the current implementation of BundleImportDescription. Same thing for CvsBundleImportDescription... well, maybe except for the project name. That name is needed when we want to create a reference string using a different name then the one from URI[1]. So how about moving a simplified version of BundleImportDescription to Team/Core, would that work in your opinion? > and IBundleImporter for finally importing the plug-ins. I wonder if we still need it, given the fact that all import operations for bundles with SCM URL will look the same: * parse manifest and retrieve String representing the URL (Eclipse-SourceReferences) * open wizard with provider specific import pages[2] * get ProjectSetCapability for each page and create ref strings for URLs[1] * import the project using the ref strings[3] No matter what provider (CVS, SVN) is associated with a bundle, the steps will be the same. Of course, if a provider does not provide an import page or project set capability these project won't be imported. Can we do the actual import[1][3] in org.eclipse.team.ui.IScmUrlImportWizardPage.finish()[4]? [1] org.eclipse.team.core.ProjectSetCapability.asReference(URI, String) [2] org.eclipse.team.ui.TeamUI.getPages(URI[]) [3] org.eclipse.team.core.ProjectSetCapability.addToWorkspace(String[], ProjectSetSerializationContext, IProgressMonitor) [4] added in Fix 01, a copy of IBundeImportWizardPage
(In reply to comment #11) > [...] moving a simplified version of BundleImportDescription to Team/Core [...] > [...] do the actual import in org.eclipse.team.ui.IScmUrlImportWizardPage.finish() I'm preparing a rough patch to see whether these would work.
Created attachment 186484 [details] Fix v02 Here it is. The patch looks even worse on the PDE side, but the purpose of this patch is to illustrate comment 11. I haven't removed CvsBundleImporterDelegate, IBundleImporterDelegate and ext. point scheme since CvsBundleImporterDelegate#validateImport method is still used to create import descriptions. When moved to a different place, the importer and all related classes could be removed. This would simplify the PDE code even more, moving the responsibility for importing projects to Team and provider specific import pages. Note: I didn't try importing using the Import wizard with the patch but did manage to import a project from the Plug-ins view. Selection between tag/HEAD doesn't work, the project is always checkout from HEAD. The import is not performed as a job as it should be. I'm not sure if the last thing isn't going to be a problem. Is it ok to schedule couple of import jobs for each provider page being closed? Or should they be gathered and run in a single job?
Ankur, since the patch is getting quite heavy and the changes are harder to track, I suggest we create branches on both sides (Team and PDE). What do you say?
We have to break the patch anyway as we will have t check it in differently. Patches I have attached is nothing but Fix v02 for PDE and TEAM separately. Also, the comment 11 is very explanatory (thanks for it). I am trying to complete the PDE patch will let me know if need more help/changes from TEAM
Created attachment 186610 [details] Fix v02 (PDE)
Created attachment 186611 [details] Fix v02 (TEAM)
(In reply to comment #15) > We have to break the patch anyway as we will have t check it in differently. > Patches I have attached is nothing but Fix v02 for PDE and TEAM separately. Thanks, I've created a branch for Team side, named it branch_20110112_bug330490. Then I applied both patches (Fix v01 and Fix v02) as separate commits. You can now switch to the branch and be up to date. Branched projects: * org.eclipse.team.core * org.eclipse.team.cvs.core * org.eclipse.team.cvs.ui * org.eclipse.team.tests.cvs.core * org.eclipse.team.ui > Also, the comment 11 is very explanatory (thanks for it). I am trying to > complete the PDE patch will let me know if need more help/changes from TEAM All right, in the meantime I will try to polish the code in the Team side branch.
We will continue to work on this during 3.7 M6.
Created attachment 188123 [details] Fix v03 (PDE)
Created attachment 188124 [details] Fix v03 (Team)
TO DO 1. Verify comments are correct (first pass done). 2. Add code for handling 'tag' so that right version is checked out 3. test for non-cvs
Thanks for the patch Ankur, I've already committed the Team part to the branch. Couple of minor things I will fix and update the branch when done: * current version of Team API is 3.6 so I'll update @since tags from 3.7 to 3.6 * the extension point description you added to team.core needs to updated, it still thinks it's in PDE * there is no need to set selection on pages returned by TeamUI.getPages, we agreed that the pages should have the selection set when returned (you're doing this in org.eclipse.pde.internal.ui.wizards.imports.RepositoryImportWizard.addPages()) * I think we're still missing a progress monitor when importing bundles in IScmUrlImportWizardPages like CVSScmUrlImportWizardPage * it's a good idea to mention that the new API is experimental, so I'll add it where needed * I guess CvsBundleImporterDelegate can be made abstract and renamed to BundleImporterDelegate. A concrete implementation in CVS could be there only to return a set of supported SCM prefixes. By default, all importer delegates would do the same thing: create an array of ScmUrlImportDescription in validateImport and then convert them to references and import in performImport * I'll update copyrights to 2011 where missing * I'll update test cases in org.eclipse.team.tests.ccvs.ui.ProjectSetImporterTests to cover the new stuff you added, I will also try to write some tests for non-cvs cases
(In reply to comment #23) > * current version of Team API is 3.6 so I'll update @since tags from 3.7 to 3.6 Done. > * the extension point description you added to team.core needs to updated, it > still thinks it's in PDE Done. > * there is no need to set selection on pages returned by TeamUI.getPages, we > agreed that the pages should have the selection set when returned (you're doing > this in > org.eclipse.pde.internal.ui.wizards.imports.RepositoryImportWizard.addPages()) This is in PDE. > * I think we're still missing a progress monitor when importing bundles in > IScmUrlImportWizardPages like CVSScmUrlImportWizardPage Haven't looked at it yet. > * it's a good idea to mention that the new API is experimental, so I'll add it > where needed Done. > * I guess CvsBundleImporterDelegate can be made abstract and renamed to > BundleImporterDelegate. A concrete implementation in CVS could be there only to > return a set of supported SCM prefixes. By default, all importer delegates would > do the same thing: create an array of ScmUrlImportDescription in validateImport > and then convert them to references and import in performImport Moved the delegate to team.cvs.core, haven't abstracted it yet. > * I'll update copyrights to 2011 where missing Done. > * I'll update test cases in > org.eclipse.team.tests.ccvs.ui.ProjectSetImporterTests to cover the new stuff > you added, I will also try to write some tests for non-cvs cases Added one tests, working on the others. All changes have been committed to the branch.
(In reply to comment #24) > > * I guess CvsBundleImporterDelegate can be made abstract [...] > Moved the delegate to team.cvs.core, haven't abstracted it yet. Done, created an abstract BundleImporterDelegate (team.core) and a concrete implementation - CvsBundleImporterDelegate (team.cvs.core).
Another thing to fix: current implementation of org.eclipse.team.internal.ccvs.ui.wizards.CVSScmUrlImportWizardPage.finish() imports projects which is redundant given the fact PDE is calling PluginImportWizard.doImportOperation in its wizards on performFinish. This results in the import to happen twice, the later probably does nothing, but it still counts.
Ankur, I know that we agreed to import projects within org.eclipse.team.ui.IScmUrlImportWizardPage.finish(), but after looking at the code I don't think this is the right approach. IScmUrlImportWizardPages know nothing about importers (importer delegates) that should be used to perform the import. The importer (delegates) are far better option for doing the import than referring to RepositoryProviderType.getProjectSetCapability(), which could be done in a page. The wizard also gathers all descriptions from its pages and do the import in a single job, doing the import in each page seems to be awkward comparing to this. Having said that, I recommend reverting changes from your patch made to org.eclipse.pde.internal.ui.wizards.imports.RepositoryImportWizard.performFinish(). With the new Team API the method should look like this: Map importMap = new HashMap(); List plugins = new ArrayList(); IWizardPage[] pages = getPages(); for (int i = 0; i < pages.length; i++) { IScmUrlImportWizardPage page = (IScmUrlImportWizardPage) pages[i]; if (page.finish()) { ScmUrlImportDescription[] descriptions = page.getSelection(); ... The other wizard which could be used to import projects from repository (PluginImportWizard) should also be responsible for doing the import. It should not assume it has been done in pages. I've updated the branch with some minor fixes, keeping the above in mind. I'll know check why we get no feedback when importing a project which is already in the workspace but has been closed. Other than that, I think we're fine. Of course, the code still need some polishing and there are places when we're completely missing documentation. I'll fill these holes on the Team side as well.
Created attachment 189412 [details] Fix v04 (PDE) I have made the required changes in PDE code. The import ends silently when a closed project already exists. But this is the current behavior too (that does not means its right). As discuss with Tomasz, we will let the current behavior. This is because the import operation is launched as job in non-ui thread from Team and thus, it can not come up with a dialog. The notification to the user will happen in PDE code like it is done for conflicting open projects. A separate bug will be opened to track this.
Created attachment 189425 [details] Fix v04 (Team) I think we're good Ankur. This patch contains the result of merging the branch_20110112_bug330490 branch into HEAD. This is the code I would like to commit to HEAD. Please let me know if this works for you or if you noticed anything suspicious. If you're fine with the current shape of the branch in Team I would like to commit the changes after tomorrow's I-build. How does it sound?
Created attachment 189466 [details] Fix v04 (PDE with JUnits) Added and updated Junits
The only thing I found when testing both patches today is the case when you try to import a bundle from _a night build_. The options you have on the "Import Projects from CVS" page[1] are: * Import specific version -- <bundle name> HEAD [dev.eclipse.org] * Import from HEAD -- <bundle name> [dev.eclipse.org] No matter which way you go the result will be exactly the same. Note that this is an expected behavior but the choice you're given is rather awkward. [1] an implementation of IScmUrlImportWizardPage
Created attachment 189506 [details] Screenshot
Both patches (for Team and PDE) have been applied to HEAD. Available in >=N20110222-2000. Thanks Ankur! I filed bug 337862 for the thing from comment 31.
I saw org.eclipse.team.internal.ccvs.ui.IHelpContextIds.CVS_SCM_URL_IMPORT_PAGE but could not see any doc update. Is there a bug for that?
(In reply to comment #34) > Is there a bug for that? There is now, it's bug 337958.
Created attachment 189645 [details] Patch to correct PDE doc issues There was a schema file and some stale links left in PDE. I have fixed them in HEAD.
