Bugzilla – Full Text Bug Listing
| Summary: | [EditorMgmt] (regression) openEditor does not throw PartInitException anymore | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Boris Bokowski <bokowski> | ||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | deboer, eclipse, eclipse, ed.burnette, emoffatt, frydzewski, kris.klindworth, Michael.Valenta, michaelvanmeekeren, Mike_Wilson, mkwhitacre, n.a.edgar, randy_giffen, Tod_Creasey | ||||
| Version: | 3.1 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | stalebug | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 218197 | ||||||
| Attachments: |
|
||||||
|
Description
Boris Bokowski
The problem is that we expect the internal editor to fail with a PartInitException so we can fall back to using a default text editor. The internal web browser catches trhe exception itself and shows it to the user. It should throw the exception to the client. Also, the error message that is shown is actually incorrect since there is no Open With in the Repo view (although there probably should be). We fail because embedded web browser can only accept either text or url, while you are passing us editor input that we cannot convert to a url. For resources, we check for 'IPathEditorInput' because it will give us a local path we can use. There are several possible solutions: 1) Add support for all editor inputs by loading bytes from the input stream into a tmp. file, then opening the temp. file. This would work with both external and internal browser, but in case of HTML files with relative references to other files or images, the links would be broken (similar to browsing an HTML file directly from within a zip). 2) We find a way to get a URL from your editor input. This URL would be the one we use to view CVS files from Eclipse.org We should handle the exceptions that result from SWT browser widget failing to create. However, in the case of editor input we cannot handle, we should throw PartInitException. This is not a browser problem. The offending code is in org.eclipse.ui.internal.EditorManager.busyRestoreEditor. It is catching the PartInitException and opening an editor to show the error (just as you suspected, Nick). Marking as major since this is a regression with bad consequences for tools, such as the repo view, that try to open internal editors on remote files. Stefan, this is a regression due to the new error part. The error part should only be used when restoring from a previous session. For programmatic calls to openEditor, if it fails with a PartInitException the exception should be propagated to the caller. Should have a regression test for this too. I got this error too, but I *really* like that cool new error part (more user friendly). Is there a way to fix this that doesn't involve gutting the error part? If there is no other way to do so, I would suggest offering new API to determine if an input is supported by a particular editor type. openEditor was never able to propogate a PartInitException from init() in all situations, and if we promised to do so we'd never be able to honor that promise. It would work in the (common) situation where no previous editor exists on the input and the editor had not been persisted from a previous session, but it would not work in other situations. In cases where there was no previous editor, it was expected that PartInitException would get propagated from init to the caller of openEditor. Changing this now would be a breaking change, as we've already seen in the scenario here. There may be other affected clients. I think it's good to improve our error handling when restoring the workbench state from the previous session, and the error part can be used here, but we should not change the existing semantics of openEditor. OK, so since this is marked as M7, let's decide. Making a breaking change and adding API does not seem like a reasonable option (i.e. I doubt I could convince the PMC of this) especially in the context of performance work. I guess we could offer the following semantics: If a call to openEditor results in a new editor part being created, any exception in that part's init method will be passed up through openEditor. Just so we're clear: the fact that openEditor terminated without throwing an exception will not indicate that the part was created successfully and will not indicate that the input is visible in any editor. I'd be fine with that. In the first case, the PartInitException is an explicit refusal from the editor to deal with the given input, and should be communicated back to the caller. Any other exception is a programming error and can be handled with the error part. Although this change is reasonable, it will not be trivial and there is not a lot of time left for 3.1. Is a workaround possible? If not, I can try to address this but something else important will get bumped. This is a regression to base line functionality (in an effort to improve error handling cases), and needs to be fixed for 3.1. Let's discuss priorities. Marking as P1 since this is a regression from 3.0, and I believe it is a must-fix for 3.1. I agree with Nick that this is a must fix. I have a bug (bug 97990) to use the new content type support to determine the best editor to use. This is a fairly straight forward fix on my part but I can't put it in until this bug is fixed. Okay. Just asking. I'll look into this one next. Created attachment 22576 [details]
Fix for this PR
This patch propogates the PartInitException when opening an editor.
However, I'm not committing it because I can't reproduce the original problem
with the repo view and can't confirm if it's fixed.
Is this still a problem with the repo view (or anyone)? If so, how can I
reproduce it in a new build?
I suspect that the logic involved in opening the internal web browser has changed. I did some debugging and the Web browser is opened as an external editor and this fails properly with a PartInitException when the content being open is not contained in a local file. With this patch, what is the behaviour if the view or editor does not exist (extension not found)? It should throw PartInitException in this case too. I believe that the risks involved in removing a tab while inside the error handling code exceeds the risks of some client relying on a PartInitException for some other specific error. (There were many 3.0 bugs caused by exactly this pattern) After a long debate with Nick, we've comprimised on, for 3.1, only throwing the exception for EditorPart.init, but not for other errors. However: the patch attached here does not actually fix the bug. Although it checks for the exception in openEditor, the part would have already been created when the tab was added to the presentation. This means that the error part still gets created. To fix this, we need to force part creation before adding the part to the presentation... but this changes event order, making it an even higher risk change. Since there is no more time for RC2 and this no longer causing the original problem, I'm deferring this. Mike can you approve this for RC3, please talk to UI or Michael Valenta for more information. We need to understand why this works in todays build RC2 moving to 3.2, Mike V. or Stefan please provide a summary of our discussions on this? The editor registry handles the case where an external editor is being opened on an input that is not an IPathEditorInput. In this case the PartInitException is thrown and the caller can revert to using a text editor. This means that the only potential problem case is opening remote contents with an internal editor. The internal editors that ship with the SDK can handle remote contents. However, it is possible that other do not. Given the complexitiy of the change, the decision was made to hold off on this for 3.1 with the expectation that it can be fixed in 3.1.1 if the impact is larger than we suspect. downgrading priority, this is not as critical given the 3.1 status downgrading priority, this is not as critical given the 3.1 status I can still reproduce the problem in 3.1RC3, i.e., when I double-click on a .html file in a CVS repository I can't see the file. The only way to see it is to check it out, or do a CVS Annotate on it, which for some reason always brings up the text editor. This is strange - when I click on a .html file in the Repository view (3.1RC3), the text editor opens. Does this happen in a fresh 3.1RC3 install with a fresh workspace too? No. The difference is in file associations. To duplicate, do Window > Preferences > General > Editors > File Associations. Select *.html, Add..., select Internal Web Browser, press Ok, select Internal Web Browser, select Default, then press Ok. Then double-click on a .html file in cvs and it will fail. Under Preferences > General > Web Browser I have the "Use internal Web browser" option turned on so I don't know why Web Browser is different from Internal Web Browser. Since this is not a browser issue, I'm reassigning this bug to the inbox so that it can be picked up by the correct owner. Deferring *** Bug 184089 has been marked as a duplicate of this bug. *** For 3.3, I have released a change to Team and CVS that will detect that the proper editor failed to open and will fall back to using a text editor. I have also released some regression tests. To make the above work, I had to reference workbench internals (i.e. check whether the opened editor is an instance of ErrorEditorPart). I also noticed that, with the new status handling support, the user is prompted with an error and an empty editor part is opened. Perhaps, now that we have the status handler prompt, we shouldn't open an empty editor (since it serves no purpose). If we decide to keep it open, we should still display the error in it. Now that I think about it, one simple solution that would help all clients would be to open the ErrorEditorPart and present the user with the option to view the contents using the text editor. (In reply to comment #32) > I also noticed > that, with the new status handling support, the user is prompted with an error > and an empty editor part is opened. Perhaps, now that we have the status > handler prompt, we shouldn't open an empty editor (since it serves no purpose). Tod, do we have a bug for this already? Moving to 3.4. Marking for investigation early in 3.4. Changed summary (old summary was: "Error when opening .html file from CVS repository explorer") *** Bug 203353 has been marked as a duplicate of this bug. *** Eric, you are looking at the code in this area right now, aren't you? Could you add this to the list of "desirable outcomes" as well? I'll mark this as blocking Kim's bug and update it to be more generic... *** Bug 127600 has been marked as a duplicate of this bug. *** Eric, is this still on your radar, or would I be better off looking at this myself? Seems bug 127600 got merged into this bug; so I'd like to make sure the case is being covered: There should be a way to prevent the 'error editor part' from coming up - in an RCP application specifically, error handling (even during initialization) should feel integrated as part of the application. We're currently accomplishing this in what seems like a very hacky way: if our init method fails, we internally catch any exception, display our error dialog, and asyncExec a 'closeEditor' call. Our editor must be able to handle an init call that failed (but did not throw a PartInitException, since doing that would create the undesired 'error editor part'). Boris, this is marked P2/Major with target 3.4. Are we still planning to address this for 3.4? I am sorry - I investigated this for more than a full day (some time ago) but wasn't able to find a safe fix. I see lots of people on the cc list, but only one vote. How important would it be to get this fixed for 3.5? I am willing to upgrade to P2 again based on your feedback. We just got hit by this again (i.e. bug report from customer) since we did not implement the CVS hack workaround in Jazz because it requires the use of workbench internals. If the internal editor fails to open on an IStorageInput, we need a fallback which allows the default text editor to be opened. Perhaps the ErrorEditorPart could just have a button that provides the option to open the file in a text editor.
Here's the hack CVS does to get around this problem:
plugin: org.eclipse.team.ui
class: org.eclipse.team.internal.ui.Utils
method:
public static IEditorPart openEditor(IWorkbenchPage page, FileRevisionEditorInput editorInput) throws PartInitException {
String id = getEditorId(editorInput);
try {
IEditorPart part = page.openEditor(editorInput, id);
// See bug 90582 for the reasons behind this discouraged access
if (part instanceof ErrorEditorPart) {
page.closeEditor(part, false);
part = null;
}
if (part == null) {
throw new PartInitException(NLS.bind(TeamUIMessages.Utils_17, id));
}
return part;
} catch (PartInitException e) {
if (id.equals("org.eclipse.ui.DefaultTextEditor")) { //$NON-NLS-1$
throw e;
} else {
return page.openEditor(editorInput,"org.eclipse.ui.DefaultTextEditor"); //$NON-NLS-1$
}
}
}
Remy is now responsible for watching the [EditorMgmt] component area. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |