Comment 1 Dani Megert 2007-05-08 06:07:00 EDT

Yep, the Platform Text code has changed (got fixed) in that area but the real problem that surfaces now is a Platform UI bug of which it has at least one duplicate already. Now that you filed it once again, maybe it gets fixed in a reasonable time frame ;-)

Just look at the stack trace. There is:
EditorManager.createSite(...) which calls IEditorPart.init(...)

And this method is spec'ed that clients should throw a PartInitException when the editor cannot be initialized. Now, how Platform UI handles this exception is completely Platform UI business.

See also bug 112277.

Comment 2 Dani Megert 2007-05-08 06:17:09 EDT

See also bug 185660 for another instance where the PartInitException is (correctly) created and thrown by Platform UI but then unfriendly handled by logging it and showing error dialogs.

Comment 3 Dani Megert 2007-05-08 08:24:50 EDT

I would expect that not every editor implementer has to handle such cases on
its own and write code can show errors inside the editor area.

Having said that. If Platform UI can't address this for 3.3 I can add a
workaround in Platform Text to get the same behavior as in 3.3. Let me know.

Comment 4 Dani Megert 2007-05-08 09:11:20 EDT

FYI: the code that shows the status for text editors can be found in StatusTextEditor.

Comment 5 Mike Wilson 2007-05-08 13:38:54 EDT

The real problem here is in the definition of "error". Users would not expect a an editor on a file that does not exist to be an error that should be displayed with a walkback (i.e. walkbacks in views/editors should *never* happen as part of the normal functioning of Eclipse). They would expect either that the editor would be closed, or it would be left in a state that the file could be recreated, by saving it again. Given our current architecture, the former would be a better choice, so all open editors (even ones that have not been realized) should be closed when their editor inputs are removed from the workspace. If that's not possible, then a presentation like the one we had in 3.2 (i.e. with a simple message indicating that the file is gone) is much better than showing them a walkback. 

The problem with simply having the UI catch the exception and do this is that it doesn't allow us to distinguish between the above case and an actual unexpected situation caused by a coding error. Currently, we assume that if you let it through to us, it was a *serious* problem (i.e. one for which a walkback should be shown).

Comment 6 Dani Megert 2007-05-09 03:56:00 EDT

Mike, the case where the input is no longer valid (i.e. cannot be recreated) is completely handled by Platform UI where the result returned by the IElementFactory is thrown as PartInitException (see bug 185660) by Platform UI code. Nothing any client can do/protect against this. And yes, I agree that there shouldn't be a dialog for this but a nice in-place message.

Now, for the case of FileEditorInput the corresponding factory in UI land does not return 'null' for FileEditorInputs whose files no longer exist but a file editor input pointing to the deleted file and hence the client code (e.g. Platform Text) is called even though it is already clear at that time that the given input doesn't exist - this shouldn't be done as there's no chance a client can ever open its editor. Now, the next step where a client comes into play is: IEditorPart.init(...) which clearly specs:

@exception PartInitException if this editor was not initialized successfully

It doesn't say anything about whether the client has to handle minor issues on its own and personally I wouldn't claim that my editor is successfully initialized if the file didn't exist, would you?

I can put the workaround back in, but as said above my code shouldn't be called in the first place and every other client that has a factory that correctly returns 'null' will also run into this and also every client that considers its editor as not correctly initialized when the file is missing and hence - according to the Javadoc - throws the PartInitException.

Comment 7 Dani Megert 2007-05-09 16:58:27 EDT

Just in case: I would have a patch ready that restores the 3.2.x behavior for textual editors regarding non-existing files.

Comment 8 Mike Wilson 2007-05-10 10:17:49 EDT

As of the latest builds, we are now showing an error part, rather than popping up the error dialog for this case. This is fine for *real* errors, but not a good solution for something that a user can easily make happen with normal workflows.

Re-assigning to Boris...

1) Boris, please determine whether we can actually get the unrealized editor tabs to close automatically. If that's true, then showing the editor part for other cases is acceptable.

2) If we can not make the tabs go away (i.e. don't do anything yet, Dani), then Dani will have to put back his workaround for 3.3 and the UI team will take ownership of the equivalent for 3.3.1 (or 3.4 depending whether new API is required).

Comment 9 Mike Wilson 2007-05-10 10:24:34 EDT

s/showing the editor part/showing the error part/

Comment 10 Tod Creasey 2007-05-10 15:47:52 EDT

Created attachment 66765 [details]
Patch

Here is an implementation that listens for resource deletes and then clears out the  stale references.

This turned out to be more involved then I would have liked so I would prefer we did this for 3.3.1 and had Dani release his workaround for 3.3.0.

Comment 11 Boris Bokowski 2007-05-10 16:22:30 EDT

I looked at the patch and agree with Tod that this would be a high-risk change.  I don't have any other idea for how this bug could be fixed in a simpler way.

If we decide to put this in, for 3.3, or 3.3.1, here are my comments from looking at the code:
- Resource change events can happen on a non-UI thread, the patch assumes that all notifications happen on the UI thread.
- The logic dealing with deleted containers assumes that the container is a project.

Comment 12 Dani Megert 2007-05-14 11:25:44 EDT

Note that my patch would only work for the case where core resources indeed threw a file not found exception (e.g. when the whole project is gone). In the case of e.g. just a file being deleted we do not get such an exception - instead we get an  exception that the content description could not be retrieved (FAILED_DESCRIBING_CONTENTS) and I have no clue why this happened and hence this will show up inside the editor. On the other hand file.exists() still returns true and hence I cannot special-case this on my end.

Comment 13 Dani Megert 2007-05-15 08:40:47 EDT

Filed bug 186977 for the editor input factory problem.
Filed bug 186984 for the problem when getting the charset.

Comment 14 Dani Megert 2007-05-15 08:47:28 EDT

Created attachment 67221 [details]
Patch for Platform Text

This patch fixes the problem on Platform Text side and restores the previous behavior and also works around the problems mentioned in comment 12. The fix also handles external files that got deleted.

Can one of you guys vote for it for RC1?

Comment 15 Tod Creasey 2007-05-15 10:43:15 EDT

+1 from me for Dani's patch

Comment 16 Dani Megert 2007-05-15 10:53:15 EDT

Thanks Tod. I've committed the patch and suggest to defer this bug to 3.4.

Comment 17 Dani Megert 2007-11-12 09:34:33 EST

As of 3.4 M3 there are still various cases where this can happen and it also seems that statement in comment 8 is not true using 3.4 M3:
  "As of the latest builds, we are now showing an error part, rather than popping
  up the error dialog for this case. "
I see a dialog, a log entry and finally the error shown inside your error status editor. This editor also takes away the part title so that users can no longer see the file name.

Note that due to other code changes my workaround is no longer triggered.

Comment 18 Boris Bokowski 2007-11-12 12:08:23 EST

(In reply to comment #17)
> As of 3.4 M3 there are still various cases where this can happen

Do you mean more cases than the one described by comment 0?

Comment 19 Dani Megert 2007-11-13 02:12:10 EST

>Do you mean more cases than the one described by comment 0?
1. everything with the same scenario but not text editor (e.g. class file editor
   PDE, etc.
2. see comment 12

Comment 20 Dani Megert 2007-11-13 11:26:00 EST

Created attachment 82777 [details]
Nicer error part

Comment 21 Dani Megert 2007-11-13 11:31:05 EST

Created attachment 82778 [details]
Improved error handling with nicer look

This patch
- ensures that we do not show a superfluous dialog (as everything is already 
  available via error status part
- creates the attached nicer look for the error status part (for editors and 
  views)
- fixes the problem of disappearing tab title when the NullEditorInput is used

Let me know if you like it and I'll commit it to HEAD.

Comment 22 Boris Bokowski 2007-11-13 15:03:12 EST

Looks good, thanks a lot!

Comment 23 Dani Megert 2007-11-14 03:05:15 EST

OK, I've committed the patch to HEAD. We should still improve the handling of deleted files as this is not Text specific.

Comment 24 Boris Bokowski 2008-05-21 12:50:07 EDT

Dani, can we close this one as fixed and track the remaining issue by bug 186977? Could you check if bug 186977 accurately captures what needs to be done?

Comment 25 Dani Megert 2008-05-22 16:09:06 EDT

>Dani, can we close this one as fixed 
Yup.

Comment 26 Philipe Mulet 2008-06-03 06:22:10 EDT

pls remember to set the target milestone

Comment 27 Boris Bokowski 2008-06-03 08:54:19 EDT

The fix for this went in quite some time ago. I am guessing M4.