Bugzilla – Full Text Bug Listing
| Summary: | Notify when user tries to import a project from repository that already exists but is closed | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Ankur Sharma <ankur_sharma> | ||||||||||||||||||||||
| Component: | UI | Assignee: | Ankur Sharma <ankur_sharma> | ||||||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||
| Priority: | P3 | CC: | curtis.windatt.public, daniel_megert, markus.kell.r, Michael_Rennie, Szymon.Brandys, tomasz.zarna | ||||||||||||||||||||||
| Version: | 3.7 | Flags: | curtis.windatt.public:
review+
Michael_Rennie: review+ ankur_sharma: review+ |
||||||||||||||||||||||
| Target Milestone: | 3.7 RC2 | ||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Bug Depends on: | 330490 | ||||||||||||||||||||||||
| Bug Blocks: | 346078 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Ankur Sharma
Created attachment 192319 [details]
Patch
The patch makes import operation throw a message box to show the projects that could not be imported
Curtis plz check if the message can be worded better. The message is not helpful enough. Just telling the user that some projects cannot be imported gives them no explanation as to why or what they can do about it.
Tomasz, the API says that performImport() will throw a CoreException if it is unable to import projects. In this case, it is unable to import a project but it is just returning a subset of the descriptions.
In what cases can a subset be returned? The API should specify that this can happen and possible causes.
If the only cause is a problem creating the project in the workspace, the message should be something like:
"Unable to create the following projects in the workspace. Please check that a project with the same name does not already exist.\n{0}"
This message gives the user some idea what to do, plus it puts the list of projects at the end, in case it is a long list.
Also Ankur, the messages must be NLS'd properly. It's not hard to use the quickfix to do it, and is safer to NLS them in your patch than waiting until later.
1. I haven't NL'd the message yet so that it would be easier for you to review it (my bad I forgot to mention it was a working patch not final one) 2. Projects might not get created to credentials mismatch, network timeout and other issues too. That is why I was trying to keep the message generic. 3. Other way to inform user abt this could be to add 'KEEP' property to the PluginImportOperation job and then we set the proper message to it. It will be non-intrusive. But then user will have to close/clean them from the progress view. Tomasz, maybe we can set up a chat/call with you about this. The implementation is certainly not following the API. We talked about this in the PDE status call this morning, but it looks like we had some wrong info. When working with the CVS import wizard, both open and closed existing projects are handled correctly. In the PDE import wizard, we are managing the overwrite, which doesn't handle closed projects. The API inconsistently must be fixed by a) Changing the API to support status retrieval b) Changing the comments on the API to reflect actual behaviour or c) Change the behaviour to match current API. The rest of the fix will depend on how the API changes. PDE is limited with what we can do with closed projects because we have no knowledge of what the final project name will be (though is all Eclipse cases I know about the project name matches the bundle name, but this is not guaranteed). (In reply to comment #5) > When working with the CVS import wizard, both open and > closed existing projects are handled correctly. In the PDE import wizard, we > are managing the overwrite, which doesn't handle closed projects. I will take a look why is this happening, today. BundleImporterDelegates[1] (used by PDE to import a project having its SCM URL) pass ProjectSetSerializationContext to ProjectSetCapability.addToWorkspace(String[], ProjectSetSerializationContext, IProgressMonitor) which is supposed to work in headless mode. While, the CVS import wizard passes a UIProjectSetSerializationContext which prompts the user in the case described above (existing projects)[2]. One way out I can think of is creating a UIProjectSetSerializationContext and passing it to IBundleImporters when running the PluginImportOperation[3]. This would require a change in the API[1], but it's marked experimental/provisional so I guess we can do that. Any other ideas? [1] org.eclipse.team.core.importing.provisional.BundleImporterDelegate.performImport(ScmUrlImportDescription[], IProgressMonitor) [2] org.eclipse.team.internal.ui.UIProjectSetSerializationContext.confirmOverwrite(IProject[]) [3] org.eclipse.pde.internal.ui.wizards.imports.PluginImportOperation.runInWorkspace(IProgressMonitor) Created attachment 192546 [details] Quick and dirty patch, do not apply Here's a quick patch illustrating the kind of API change I meant in comment 7. But there is another thing this patch shows: once PluginImportOperation gets a lock on workspace CVSProjectSetCapability.checkout is not able to proceed because it requires a project lock... Created attachment 192547 [details]
mylyn/context/zip
It would be better if we didn't have to run the whole import in the UI thread. Since we are looking at changing the (provisional) API anyways, is it possible for it to return a status code indicating what went wrong? Also, Tomasz, can you explain when the API may return a subset of the projects? Obviously when a closed project exists it skips over, but are there other circumstances where the same behaviour will be seen? (In reply to comment #10) > It would be better if we didn't have to run the whole import in the UI thread. Sorry, but doing it in the UI thread is a no go. We should improve IBundleImporterDelegate.performImport(...) to specify that the returned array has the same length as the description (never 'null'). In addition we should specify that it won't abort when importing one of the descriptions fails but instead - set the corresponding slot in the returned array to 'null' - add an error status to the multi-status that will be thrown at the end It should also specify what it does if the project already exists or when it is closed. Given the import runs in the background it's probably best to report both cases back to the caller who can then ask the user and call performImport(...) again with the not yet imported projects. (In reply to comment #10) > Also, Tomasz, can you explain when the API may return a subset of the projects? > Obviously when a closed project exists it skips over, but are there other > circumstances where the same behaviour will be seen? If you're asking about org.eclipse.team.core.ProjectSetSerializationContext.confirmOverwrite(IProject[]), it returns an array of projects that should be overwritten (picked by the user). The array is empty if none of the projects should be overwritten which is default for (headless) ProjectSetSerializationContext. The UIProjectSetSerializationContext implementation doesn't care if the project is open or not, it assumes that if the project/folder exists it can be overwritten. If the operation is cancelled null is returned. I'm not aware of any other circumstances when a subset of projects is returned. (In reply to comment #11) > We should improve IBundleImporterDelegate.performImport(...) to specify that > the returned array has the same length as the description (never 'null'). In > addition we should specify that it won't abort when importing one of the > descriptions fails but instead > - set the corresponding slot in the returned array to 'null' > - add an error status to the multi-status that will be thrown at the end > > It should also specify what it does if the project already exists or when it is > closed. Given the import runs in the background it's probably best to report > both cases back to the caller who can then ask the user and call > performImport(...) again with the not yet imported projects. How about mentioning even the closed projects in "Delete Plug-in Projects" dialog which we already show for open projects. Do we need to distinguish between the open and closed projects? PS; I am working on patch as suggested in Comment #11 (In reply to comment #13) > How about mentioning even the closed projects in "Delete Plug-in Projects" > dialog which we already show for open projects. Do we need to distinguish > between the open and closed projects? > > PS; I am working on patch as suggested in Comment #11 If you can make it work, adding closed projects to the dialog would be fine. The problem is that PDE doesn't know anything about projects that are closed. We don't know the plug-in name/version so we don't know if there is a conflict. Created attachment 193529 [details] Patch for comment 13 > How about mentioning even the closed projects in "Delete Plug-in Projects" > dialog which we already show for open projects. Do we need to distinguish > between the open and closed projects? Good idea! But we should make the same when importing from the target. In addition, we should not treat non-PDE projects differently: currently, if I have a simple project with the same name the imported wont offer me to delete it - instead it creates the imported project with a version suffix. >PDE doesn't know anything about projects that are closed. >We don't know the plug-in name/version The project name is know, that should be enough. Comment on attachment 192319 [details]
Patch
Dani, would you have time for reviewing this patch for M7?
Comment on attachment 192319 [details]
Patch
i haven't handled the non-pde projects yet.
The patch in comment #18 adds the closed projects as well to the 'Delete Plug-in Projects' wizard. We can put this simpler change in for RC1 and can pursue the larger changes in 3.8 Dani, let us know your thoughts. The patch has several issues:
- concatenating the projects is bad (there could be 100s) - a list needs to be
shown like in the initial dialog that asks for deletion
- the job name is wrong
- the dialog is not correctly parented
==> use PlatformUI.getWorkbench().getModalDialogShellProvider().getShell()
- the following code is wrong:
if (dialog.open() == Window.OK) {
return Status.OK_STATUS;
}
return Status.CANCEL_STATUS;
==> whether one clicks 'OK' or the 'X' shouldn't matter for a warning dialog
==> use MessageDialog.openWarning(...)
- dialog.setBlockOnOpen(true) is not needed
- strings are not externalized
- if description and projects length is the same then nothing to do
(In reply to comment #19) > The patch in comment #18 adds the closed projects as well to the 'Delete > Plug-in Projects' wizard. We can put this simpler change in for RC1 and can > pursue the larger changes in 3.8 > > Dani, let us know your thoughts. Maybe you meant the patch in comment 15 (attachement 193529)? If so, that one has issues as well ;-) - we don't decorate closed projects with a label but use the 'closed project' icon - I don't like that dialog to behave differently depending on the import kind - to preserve existing behavior we should not check closed projects by default - this is too verbose and misleading: project.exists() && project.isOpen() == false ==> project.exists() && !project.isOpen() - the addAll(...) code is wrong: we'd better use a Set At this point I don't think we should touch that dialog - this is something for 3.8, especially since it doesn't fix all cases of the repository import. ==> we should go with patch from comment 18 + my feedback given in comment 20. Created attachment 195217 [details]
'Delete' dialog with closed projects (for 3.8)
Bumping to 3.8. (In reply to comment #23) > Bumping to 3.8. I disagree with this. The import from repository is a new 3.7 feature. Failing without any notification to the user is not good. The patch from comment 18 which only notifies the user at the end is not far from being complete. Most of my comments in comment 20 regard completing/polishing that patch. The part which needs most work is: >- concatenating the projects is bad (there could be 100s) - a list needs to be > shown like in the initial dialog that asks for deletion If that change gets too big then I could live the concatenation for 3.7. Created attachment 195337 [details] Updated patch as per Comment 20 This is updated version of the patch in comment 18. It just displays a warning dialog with 'list' of projects. It does not changes the delete plugin dialog The patch might need 1. better wording of the dialog message 2. Width setting. Its too wide. I am unable to make to narrower. In reply to comment #25) > The patch might need > > 1. better wording of the dialog message > 2. Width setting. Its too wide. I am unable to make to narrower. 1) PluginImportOperation_WarningDialogMessage=The following projects could not be imported. Check that they do not already exist in the workspace in a closed or read-only state. This at least gives the user some direction on how to fix the problem. 2) I don't find it particularly wide on Win7. Can't do anything about it while using the standard message dialog. I don't think this is worth worrying about for 3.7 since we are planning on moving the changes to the better conflict resolution dialog in 3.8. Dani won't be available to review for RC1. I'll take another look tomorrow. The updated patch satisfies Dani's comment #20. If you make the changes I suggested in comment #26, I think we are good here for RC1. +1 Updated the message from comment #26. Applied to HEAD One known glitch that when the unchecked projects on 'Delete plug-in dialog' (which exists and are open and not chosen to be deleted) too appear on this warning dialog. Since this dialog will be removed in 3.8 so not worth touching all the code for this small change. This fix did not make it in for RC1. Moving to RC2. Mike, can you please review the code as for RC2 we need a second review. I can't say I am a fan of a warning dialog with a list stuck on it. Perhaps it would be better to use a error / warning dialog with the 'Details' button on it and provide the projects + the reason they failed to import in there. Also it would be good to fix the 'known glitch' mentioned in comment #29 for 3.7. And instead of a Table, it would be better if the projects would be listed (with \n between project names) in a new Text(parent, SWT.H_SCROLL | SWT.V_SCROLL | SWT.READ_ONLY | SWT.BORDER) with text.setBackground(getDisplay().getSystemColor(SWT.COLOR_WIDGET_BACKGROUND)); gd.widthHint = convertWidthInCharsToPixels(60); gd.heightHint = convertHeightInCharsToPixels(10); That would allow the user to copy the list of projects for later processing. Created attachment 195776 [details] updated with comment 32 Curtis, Mike, plz review and check it in if good. Take a shotgun approach here, adding myself, Dani, Markus and Mike as reviewers. We need two +1's for RC2 OK to fix in RC2. The patch/fix works but needs some polish:
- NotimportedProjectsMessageDialogs: class name should not be in plural
- NotimportedProjectsMessageDialogs.setProjects(String) has no Javadoc and from
reading the name I would not expect that a string is passed in. Formatting
should be left to the dialog. Either pass an array or a collection. And instead
of adding a new method it would be better to add a new argument to the
constructor.
- The dialog message "The following projects could..." is not OK: we don't use
plural if only one project is affected
==> you need one for singular and one for plural, like e.g. in the dialog that
warns about existing projects.
- The local variable 'projects' needs a better name and no for-loop is needed
==> List projectNamesList = Arrays.asList(projects);
I will set the review+ flag once that's done.
If there's not already a bug for providing the real fix for 3.8, then please file one with a link to attachment 195217 [details] and post it here.
Created attachment 195849 [details] Patch updated as per comment 20, 32 and 36 Bug 346078 opened for providing the Real Fix in 3.8 (In reply to comment #37) > Created attachment 195849 [details] > Patch updated as per comment 20, 32 and 36 The patch introduces a bug: List projectList = Arrays.asList(projects); for (int i = 0; i < descriptions.length; i++) { String project = descriptions[i].getProject(); if (!projectList.contains(project)) { projectsNotImportedList.add(project); } } is trying to compare IProject handles to Strings. If you update this to have projectList be a list of project names the 'known glitch' is fixed. (In reply to comment #39) > (In reply to comment #37) > > Created attachment 195849 [details] [diff] [details] > > Patch updated as per comment 20, 32 and 36 > > The patch introduces a bug: > > List projectList = Arrays.asList(projects); > for (int i = 0; i < descriptions.length; i++) { > String project = descriptions[i].getProject(); > if (!projectList.contains(project)) { > projectsNotImportedList.add(project); > } > } > That's due to my bad suggestion from comment 39. Sorry about that. That's what happens when one uses bad variable and method names ;-). Besides that the patch has some minor issues: - "Check that it does not already exists in the workspace" must use "exist" and not "exists". - The messages have two spaces after the dot instead of just one space. - New Javadoc on NotimportedProjectsWarningDialog is good but please terminate the sentences with a dot. - NotimportedProjectsWarningDialog even better: NotImportedProjectsWarningDialog. Created attachment 195936 [details] Updated patch as per comment 20, 32, 36 and 40 (In reply to comment #39) > is trying to compare IProject handles to Strings. If you update this to have > projectList be a list of project names the 'known glitch' is fixed. It won't. The 'Known Glitch' is that the open projects that were deselected on Delete Plug-in Projects dialog too appear on this warning dialog. To suppress them we will have to compute projectsNotImportedList - (PluginRegistry.getWorkspaceModels() - OverwriteProjectsSelectionDialog.getResults()) ps: the minus sign, methods and types are indicative and not a Java expression. Comment on attachment 195936 [details] Updated patch as per comment 20, 32, 36 and 40 This does not work either as you now compare paths with project names. Created attachment 195950 [details]
Fix
When testing, please at least have several open projects and several closed ones and then import them all together. This will allow to verify the dialog and whether the shown projects are OK. +1 for the latest patch. +1 That's two +1's, but I will wait for a couple of hours in case Ankur wants to review or if Dani would like to commit the fix. Patch applied to HEAD. Verified in I20110519-0800. |