Description
Erin Harris
2005-05-20 12:51:51 EDT
Stefan, could you please help with this one? In the absence of Stefan, moving these to 3.1 RC2. They will not be addressed for 3.1 RC1. Created attachment 22644 [details] Exception that's thrown Reproduced the exception using the updated tilededitor from bug 42641. This problem seems somehow related to changes in the editor lifecycle? The code that used to be in EditorManager called to part.setChildren() ... but now EditorManager.createEditorTab() doesn't make the call, since the part hasn't been instantiated yet. PW No more time for RC2. The updated tilededitor example works in 3.0.2 It looks like the org.eclipse.ui.internal.EditorManager was refactored in 3.1. EditorManager.openEditorFromDescriptor() in 3.0.x calls EditorManager.openInternalEditor(), which created the main editor part and called EditorManager.createEditorTab() -> EditorManager.openMultiEditor() which set the inner children. In 3.1, EditorManager.openEditorFromDescriptor() gets the EditorReference and then calls EditorManager.createEditorTab(), which just adds the editor reference to the editorPresentation. The old code from createEditorTab() wouldn't work, because the main editor part wasn't created. It looks like the EditorAreaHelper? is now responsible for creating parts, and when it calls into TiledEditor.createPartControl(), with no children set it throws the NPE. PW Created attachment 22931 [details]
Recursive exception thrown
EditorManager used to call createPart() and then call createPart() for all of
the inner children. The createPart() functionality has been moved to the
EditorAreaHelper.
Moving the call to EditorManager.openMultiEditor() to the equivalent place as
the call to createPart() in the EditorAreaHelper fixed the NPE, but caused a
recursive activation error.
!MESSAGE WARNING: Prevented recursive attempt to activate part
org.eclipse.ui.examples.tilededitor.TiledEditor while still in the middle of
activating part org.eclipse.ui.examples.tilededitor.TiledEditor
Kevin can you approve fixing this for RC3? Once we have a fix Erin, can you verify that it works for you? In addition, the WARNING should only happen in debug mode Let me know when you have a fix and I will try to test it. My plugins depend on a group of plugins that are only building with RC1 right now so I will try to test the fix with that level. Created attachment 23457 [details]
patch to ui.workbench to fix NullPointerException
MultiEditors now have their children parts created by the same component that
creates MultiEditor parts. Disposing the EditorReference for a MultiEditor
also disposes the child EditorReferences first.
There is still some investigation into activating the inner part correctly.
PW
For 3.2 or possibly 3.1.1. Created attachment 24032 [details]
Fix ui.workbench NPE
This is the RC4 compatible patch that fixes the NPE, unfortunately without
actually fixing MultiEditor.
PW
Created attachment 24096 [details]
multieditor tests v01
First 2 MultiEditor unit tests. They include a ui.tests version of the
TiledEditor.
The tests are being managed in patch form, since they fail outright against
3.1.
PW
Actually I don't see why we don't just put these tests into the test plug-in now, so we don't lost track. The only thing we have to make sure is that until we are ready, they are not being run as part of the test cycle. Created attachment 24227 [details]
multieditor tests v02
More information gathered (including my disabled coolbar icons), but they still
need some more work.
PW
Created attachment 24358 [details]
multieditor tests v03
There are 3 mutlieditor tests.
1. basic open with the same file type
2. call history order of operations that works in 3.0
3. swapping between different inner editors to check that the outline view gets
updated by the inner ant editor.
These tests are not attached to the main test suite run. All 3 tests will
fail without the NPE patch, and tests 2 and 3 will fail even with the NPE
patch.
These can be committed to HEAD.
PW
Created attachment 24437 [details]
Updated tests after code review
I've updated the tests after a code review with Stefan.
1. Comment adjustments
2. Any unexpected exception on editor open is flagged
3. call history test includes open, a setFocus, and a close
4. coolbar tracking split into different test than outline view tracking.
Later,
PW
Committed the tests patch from comment 17. What is the status on this bug? In particular: Did either of the patches to fix the NPE (comment 10 or comment 12) make it into 3.1? Is there a way to work around the problem? Finally, could this be targeted for 3.1.1? Unfortunately this was not fixed in 3.1. We are still investigating an acceptable solution for 3.1.1. Created attachment 25128 [details]
NPE ui.workbench patch updated to post-3.1 HEAD
It's the NPE patch updated against the 3.2 changes.
PW
Investigation shows that at least some of the 3.0 functionality can be returned by focusing on the PartList/ActionationList/PartListenerList. Some of the interfaces were updated to deal with the reference instead of the part, and then optimizations were added to avoid re-activating if the reference was already active. MultiEditor is in the situation where it is the same active reference (the parent MultiEditor) but a new active part (the inner editor). More investigation is needed. PW marking for 3.1.1 as discussed Paul will keep investigating to find a solution that brings us to the 3.0 support level or better for 3.1.1 Created attachment 25262 [details]
MultiEditor functionality patch for 3.1.1 v01
This patch seems to provide most of the MultiEditor functionality like 3.0.
It's still in the hack stage, and encapsulating the information I need probably
means updating IWorkbenchPart and IWorkbenchPartReference. I'm hesitant to
expose MultiEditor checks in the interfaces.
It does need a better encapsulation of MultiEditor checks in both
IWorkbenchPart and IWorkbenchPartReference, as now there are MultiEditor and
MultiEditorInnerPane checks throughout WorkbenchPage and PartList.
PW
Created attachment 25425 [details]
MultiEditor functionality patch for 3.1.1 v02
This patch brings back most of the functionality to MultiEditor that was
available in 3.0.
All of the UI tests pass with this patch, as well as the 4 MultiEditor tests.
PW
Created attachment 25426 [details]
UI tests for 3.1.1 v01
The tests for MultiEditor for 3.1.1 ... hooked into UITestSuite.
PW
Created attachment 25620 [details]
MultiEditor functionality patch for 3.1.1 v03
Updated some comments for methods that support null.
PW
Created attachment 25735 [details]
patch for word completion
This patch brings back the word completion from patch 3.1.1 v01.
One thing is broken in the patch for 3.1.1 v03 that worked in patch v01. The
word completion.
The patch requires the patch for 3.1.1 v03.
Things broken in patch v3 working in v1: Selection: Actions are going into the wrong editor after set setting the focus from one editor to the other. Menu: Actions without keyboard shortcuts. Many of them don't work at all. (Word Completion for instance.) Outline: Editor displays only the Outline from the first Editor. v1 of the patch was able to switch the outline together with editor. Created attachment 25831 [details]
MultiEditor functionality patch for 3.1.1 v04
This contains Ulrich's update and a minor matching fix in WorkbenchPage.
Thanx, Ulrich
PW
Created attachment 25832 [details]
UI tests for 3.1.1 v02
This contains the updated tests that match MultiEditor 3.1.1 v04
PW
Created attachment 25834 [details]
UI tests for 3.1.1 v03
Fixed a test and added some comments
PW
Working in v4: Selection: Actions are going into the right editor after set setting the focus from one editor to the other. Menu: Actions with keyboard shortcuts. (Word Completion for instance.) Outline: Each Editor has the correct outline. One problem: The "mark occurencies" in the embedded Java Editor won't work. I don't know if it ever worked in an embedded editor. So far so good. Is there an estimate when the stuff goes into the cvs. Problems with this patch: - IWorkbenchPage.getActivePart() points to a different part than IWorkbenchPage.getActivePartReference() when a multi-editor is active (the reference points to the outer editor and the part points to the inner editor). This will cause various code that uses the active part to get out of synch. - There are extra focus changes on part activation. Paul: when we first discussed this, I said that there was another problem in that the active part (an inner editor) would not be in the editor list or activation list. I now agree with you: this is probably okay to leave for a future fix. All we need to ensure initially is that both the active part and active reference both point to the same spot (the inner editor). The use of a child editor list within editor reference is clever and useful. I'd suggest expanding on that in 3.2: it could permit MultiEditors to be persisted between sessions if the list were recreated in the constructor rather than with the part. Created attachment 25890 [details]
MultiEditor functionality patch for 3.1.1 v05
I've updated the patch so that IWorkbenchPage#getActiveEditorReference()
returns the inner editor reference, not the MultiEditor editor reference.
I've checked 3.0.2, and for MultiEditors we've always returned the active inner
part and the active MultiEditor reference, and that is how we've fired change
events.
If we need to fire change events for Inner Editor references, then we'll need
to open a bug report for this and look at addressing this for 3.2.
PW
Created attachment 25923 [details]
MultiEditor functionality patch for 3.1.1 v06
I've updated the patch so that IWorkbenchPage#getActiveEditorReference()
returns the MultiEditor editor reference.
That makes it consistent with what was available in 3.0.2
PW
Created attachment 25936 [details]
MultiEditor functionality patch for 3.1.1 v07
Slightly re-worked patch ... the outline tracks the inner editors, but it
maintains the invariant (activePartReference().getPart(false)==activePart())
Markers show up, but some of the Java pre-compile squiggles don't.
PW
Created attachment 25937 [details]
UI tests for 3.1.1 v04
Tests that match the MultiEditor 3.1.1 v07
Review of patch: + This patch rolls back (well, modifies) the fix for Bug 43010. Does the problem described in Bug 43010 exist with this patch? + The patch leaves a TODO in MultiPageEditorSite and CompatibilityPartSite. Is this intentional? + This patch adds API. I'm not sure that this is kosher.... Also, some of the tests are marked as "@since 3.1" and some aren't marked at all. Created attachment 25990 [details]
MultiEditor functionality patch for 3.1.1 v08
Removed new API for 3.1.1 patch.
PW
Patches committed to CVS, and should appear in M20050811-0400. This patch has not caused Bug 43010 to reappear. The API has been removed. All new test classes are marked as 3.1.1. The TODOs are gone. Does this patch also need to be applied to the 3.2 stream? Or do you want to modify it for 3.2 (e.g., to include the new API)? Created attachment 26109 [details]
MultiEditor 3.2 fix
The 3.2 version of the patch.
PW
Created attachment 26111 [details]
Minor update to the 3.2 MultiEditor tests
Some minor updates to the MultiEditor tests that were already added.
PW
Created attachment 26121 [details]
Minor update to the 3.2 MultiEditor tests v2
Update with a fix to avoid interactions with the drag tests.
PW
3.2 patches added for build >20050815 Should this bug move to RESOLVED FIXED now that the fix is in both the 3.1.1 and 3.2 streams? Released into 3.1.1 and 3.2 PW Two issues left: 1. The outline is still visible after closing the editor. I've found a bugreport for that issue. 2. The find diaglog is able to find find/replace but does not display the selection (the highlighting) of the search result. I had no luck to find a bug report for that issue. Should I open a new report or reopen this one? (In reply to comment #51) > 2. The find diaglog is able to find find/replace but does not display the selection (the highlighting) of the > search result. I had no luck to find a bug report for that issue. Should I open a new report or reopen this > one? I was hoping that this might have to do with bug 108324 (text selection and post selection listeners) but that PR is a problem with MultiPageEditorParts. Please open a new PR for issue #2. Thanx. PW verified in 3.2 |