Community
Participate
Working Groups
One of the most desirable Java Editor's features Compare Editor missing (when comparing two CUs) is Content Assist. This might be non-trivial to non-local part of a comparison (when comparing with HEAD for instance), but should be achievable for the local, editable part.
is this still planned for M3 ? (doesn't work with I20081026-2000)
With the investigation we made when addressing this bug and bug 250288, we realized that there is a group of bugs (under common "Richer Compare Editor" label) which are not so straightforward to fix. Some of them may even require to address bug 193324. This forces us to postpone this bug to M4. Nevertheless, this bug is still on top of my list.
Created attachment 116833 [details] Patch v01 The first version of a patch. There is still plenty of issues to fix (handler conflicts, NPE on window focus etc) but at least the patch gives the idea of how Java features will look like in Compare Editor. Content Assist action is available from context menu, under action named "Default". There are still some issues with handlers so the key binding doesn't work. The patch also adds Ctrl+clicking and Javadoc popus which I actually got for free while trying to add the Content Assist. When an API change was required I used reflection so changes are limited to JavaMergeViewer and classes in org.eclipse.compare. Comments are welcomed. This is a work in progress...
Created attachment 116834 [details] mylyn/context/zip
Created attachment 117649 [details] Patch v02 Next version of the patch proposition. It fixes some issues from the previous patch: * Handler conflicts are partly covered. For example, the key binding for Content Assist action works now as expected. The same for Delete line and some others * NPEs thrown on focus change has been replaced with an assertion failure. This is caused by the fact that when disposing JavaMergeViewer we disconnect a document (removing a "dflt" category position from it), and in consequence when the file is opened in a different editor the assertion is thrown (the category is missing) Remaining issues are: * Missing API is hacked by using reflection * I added new API to TextMergeViewer (overridden in JavaMergeViewer). It would be great if some more experienced (like Dani) could check if this is worth anything. I'm aware that the new API might be far from perfect but I was looking for a working solution. * IllegalArgumentException when Ctrl+clicking a class name on a viewer for non-existing resource (like ancestor or remote part when comparing with HEAD) * Activating context should be more flexible for Compare Editor Other issues are emphasized in the code by TODO, FIXME and XXX tags. Let me know if I missed anything. As before, this is still work in progress but I would be very happy to hear any feedback from you... Dani ;) and others. The good news is that fixing this bug will cover couple of others like: bug 250288, bug 250903 and bug 116152.
(In reply to comment #5) > The good news is that fixing this bug will cover couple of others like: bug > 250288, bug 250903 and bug 116152. ... forgot to mention bug 2845 as well.
What should do in the first place is a short introduction to the approach taken in the patch: In bug 6823 we implemented an ITextEditor adapter which wraps MergeSourceViewer from Compare Editor and imitates a Text Editor which can be used in actions like Go to Line. This idea has been extended in the latest fix, the ITextEditor has been moved from MergeSourceViewer to TextMergeViewer class and it now uses ContributorInfo as document provider. As for comparing CUs, JavaMergeViewer implements an anonymous version of CompilationUnitEditor which wraps the adapter passed from TextMergeViewer. The editor in JMV is then initialized with an input from the adapter, a viewer in ITextEditor adapter is replaced with the one from anonymous CUE and then actions are created for CUE. The problem is that at the time the Compare Editor part is created, ContributorInfo (our document provider) objects are not available. This is the reason for the new API method called updateAdapters. It's called as soon as input is set on the Compare Editor, so the ContributorInfo objects are initialized and we are ready to properly create actions as described above. The other issue is switching actions handler which is done in TMV, so another API method (getContributedActionId) has been added so TMV knows which actions to handle.
Created attachment 117901 [details] Patch v03 Context activation handled more properly. Checks to prevent NPE while displaying JMV in context different than Compare Editor (Java Editor features are disabled for such contexts).
I started to review the code (Patch_v03). Here are's a list of the bigger/conceptual issues I found: 1) too many framework methods to switch (set/update) the activation. This makes it complicated. I would expect to see something like: setActiveViewer(viewer) or maybe setActiveViewer(viewer, context), where context is left, right, ancestor similar to - org.eclipse.ui.IEditorActionBarContributor.setActiveEditor(IEditorPart) - org.eclipse.ui.part.MultiPageEditorPart.setActiveEditor(IEditorPart) 2) if there's a problem to fit the new workflow (suggested in 1) into the existing framework then this could be separated with a tagging interface or even better by adding a new (TextEditor?) merge viewer class. Having a new class might be cleaner and more readable than hacking both flows into one class. It would also be easier for clients (subclasses) as they don't have to know/deal with both workflows: either they subclass the old or the new class. 3) reflective code can be accepted for some exceptions (e.g. during API freeze or if unsure whether this should be API at this point) but not as being part of the design. Most of this is not needed as you can make it API in compare if really needed (e.g. text editor adapter compare viewer methods, etc.), see 4. 4) almost all of org.eclipse.jdt.internal.ui.compare.JavaMergeViewer.updateAdapters() code could go into Compare (actually that method should go away, see 1). Only let the subclasses create (and configure) the viewers. That way you can most likely get rid of most issues from 3. 5) some methods (e.g. connectGlobalContributedActions) only take a source viewer. The client does not know whether to configure left, right or ancestor, see suggested API in 1) 6) the current API is too limiting as it only allows to set handler for existing stuff (by asking for the contributed ids). Subclasses should also b able to add things to the tool bar or the menu. Instead of asking for the ids and then register the handlers/actions in the framework I would pass the handler service (or even something more powerful like ICompareViewerSite, similar to org.eclipse.ui.part.MultiPageEditorSite) to the subclass. Besides that, the code is missing @since tags and Javadoc and as stated in previous comments there are still several handler conflicts.
Some more findings: - I wouldn't use 'part' as parameter name for (source) viewers - another problem is that the (source) viewer type is given/hard-coded (MergeSourceViewer) and hence functionality that already exists in another source (e.g. JavaSourceViewer) needs to be duplicated. It would probably be better if subclassess could create their own source viewer for each area and compare would then configure/plug-in additional compare features - I *think* one reason for the handler conflicts is that we register the handlers for all source viewers together to the same scope instead of switching when one of them gets activated
Created attachment 118783 [details] Patch v04 I have rewritten the patch from the scratch. The main difference is the new class extending TextMergeViewer called TextEditorMergeViewer. It modifies the flow of TextMergeViewer, so now we configure text viewer after editor adapters are available. The other important change is introduction of ITextEditorAdapter[1] interface. I manage to get rid of handler conflicts by disposing/deregistering all actions created for CompilationUnitEditor which I cannot control (switch on focus change) with the Compare Handler Service[2]. Dani, any chance to give it a second look? [1] org.eclipse.compare.contentmergeviewer.TextEditorMergeViewer.ITextEditorAdapter [2] org.eclipse.compare.contentmergeviewer.TextMergeViewer.fHandlerService
Remaining issues: * closing a Compare Editor throws the following exception: Exception in thread "org.eclipse.jdt.internal.ui.text.JavaReconciler" java.lang.NullPointerException at org.eclipse.jdt.internal.ui.compare.JavaMergeViewer$2.getSite(JavaMergeViewer.java:444) at org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitEditor.reconciled(CompilationUnitEditor.java:1614) ... * Ctrl-clicking on ancestor will throw an NPE (see comment 5) * Assertion failures when closing a CE with JavaMergeViewer for a file opened in other editor. Happens only when comparing two local resources. Fixing bug 193324 should help here. * createActions called for the adapter duplicates some actions already added in TextMergeViewer (like Find). This is an example of what Dani mentioned in comment 10, bullet 2.
btw, the latest patch doesn't have the java features installed on the ancestor viewer.
Created attachment 118993 [details] Patch v05 Updated patch, it now applies against HEAD (releasing fix for bug 251215 made to previous patch out of date with HEAD). Also, I added Java feature to the ancestor viewer (if available) so previous comment is no longer valid. As mentioned before, ctrl-clicking on non-local CUs like an ancestor or a remote revision will throw an NPE. Is there anything we can do about it and the JDT side? I noticed that when doing the same thing for FileRevisionEditorInput[1] (open a revision in editor) ctrl-clicking works fine, because it gets the CU (needed to open an editor) from fFakeCUMapForMissingInfo[2] map. [1] org.eclipse.team.internal.ui.history.FileRevisionEditorInput [2] org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider.fFakeCUMapForMissingInfo
Created attachment 119010 [details] Patch v06 The patch leverages the way comparing local resources works. We no longer depend on ResourceCompareInput, but from now the editor for local files comparison uses SaveablesCompareEditorInput. This fixes the issue mentioned in comment 12 bullet 4 (assertion failure when closing CE...). Unfortunately, we didn't get it for nothing, without adapting the ResourceCompareInput to work with improved CE we will get in trouble (get bunch of exceptions) when we launch Eclipse without Team component (Compare only). CE will try to open itself on ResourceCompareInput, not on SaveablesCompareEditorInput, as it does now and this will cause the trouble. One way of handling this situation is mention in bug 256805, comment 1.
Tomasz, when starting to look at the patch I first tried in the UI whether it works. However, I get various exceptions (NPE, CCE, assert exceptions) working with the patch installed, especially as soon as (Java) compare is opened while the file is already open in the Java editor (which is a common scenario) things break: it marks the file as dirty/changed directly after opening the compare editor. Closing most of the time fails and I ended up in states where the workbench was unusable due to endless dialogs. I'm not sure whether I should start spending time on a patch that doesn't even work stable. It definitely needs more work until it has commit quality. I can comment on the taken approach if you want but hopped to get a working patch for review. Sorry, Dani
It's my fault, sorry for giving you false impression that it's a ready-to-commit patch. My intention was to show it to you and ask for your opinion even though it still has some defects (like the NPEs, CCEs and AFEs you mentioned). I wouldn't like to end up with a dead-end approach, so I asked for you feedback on the changes I made in the fix (starting from Patch v04, comment 11). If in your opinion the new approach is worth anything I will fix all the exceptions asap. Please give it another look... focusing on the general approach.
Created attachment 119276 [details] Patch v07 Fixed CCE thrown when changing preferences for the Compare Editor via context menu. The Compare Editor can now embed Viewers provided by a client[1]. The general approach remains intact, as well as other issues (at the moment). [1] org.eclipse.compare.contentmergeviewer.TextEditorMergeViewer.getSourceViewer(Composite)
Created attachment 119277 [details] mylyn/context/zip
I looked at the patch. I don't like the fact that the client/subclass now has to handle/store all viewers. That seems like an unnecessary change. I would expect that the Compare framework creates three JavaMergeViewers as before. Some other stuff I saw: - CompareEditor: why does it always activate the textEditorScope and not just when needed (i.e. when in new "editor mode"): service.activateContext("org.eclipse.ui.textEditorScope"); //$NON-NLS-1$ Note that the "org.eclipse.jdt.ui.javaEditorScope" needs to be activated - use interfaces (e.g. ISourceViewer) and not the class (e.g. SourceViewer) - some of your new APIs leak internal type MergeSourceViewer which makes the API unusable for clients
Created attachment 119613 [details] Patch v08 Actually, this should be the first patch for Dani to review. So, thanks for looking at the patches I send you earlier. What has been fixed in this patch: 1. immediately marking file as dirty when comparing with local review (see bug 257696) 2. we're now properly using text file buffers, no more errors when closing the CE editor while another editor is open 3. javaEditorScope activation 4. API leaks 5. using enhanced CE in dialogs has been disabled, no exceptions thrown now The remaining issues are: 6. trying to ctrl+click on a non-local part of the comparison causes NPE 7. hover tips for a non-local part may be inadequate. This seems to be a problem on JDT side, since opening the Java Editor on a revision gives the same results. 8. closing CE editor when a change has been made in it causes NPE (working on this)
(In reply to comment #20) > I looked at the patch. I don't like the fact that the client/subclass now has > to handle/store all viewers. That seems like an unnecessary change. I would > expect that the Compare framework creates three JavaMergeViewers as before. I didn't change that, JavaMergeViewer (the client) has been handling the viewers for the whole time. However, I did make JavaMergeViewer to keep a map of CU adapters, maps of source configurations, preference stores and property listeners.
Created attachment 119693 [details] Patch v09 Fixed all the issues from comment 21 (6., 7. and 8.) and implemented some tips from Szymon.
Created attachment 119716 [details] Patch v10 Added "Experimental" tags for the new API and fix for an issue with configuring ancestor part.
Before looking at the code I did some quick testing and found several issues that are not OK (see below). Given how important/prominent the Java compare feature is, we cannot risk to break the the stable build which is planned this week. Several people have to self host on the new code for at least one full week. We need to shift this to early M5. - in Java Compare editor type something (OK: dirty) and hit Ctrl+S ==> save is not performed - whole undo/redo stack lost after save - undo/redo key bindings no longer work in compare dialog - quick views leave compare editor i.e. open element in Java editor instead of navigating to it inside compare editor - still hit NPE when running in the debugger: Thread [main] (Suspended (exception NullPointerException)) JavaMergeViewer$CompilationUnitEditorAdapter(CompilationUnitEditor).updateStateDependentActions() line: 1657 JavaMergeViewer$CompilationUnitEditorAdapter.updateStateDependentActions() line: 534 JavaMergeViewer$CompilationUnitEditorAdapter(AbstractTextEditor).validateState(IEditorInput) line: 4750 AbstractTextEditor$24.run() line: 4781 BusyIndicator.showWhile(Display, Runnable) line: 70 JavaMergeViewer$CompilationUnitEditorAdapter(AbstractTextEditor).validateEditorInputState() line: 4776 JavaMergeViewer$CompilationUnitEditorAdapter(StatusTextEditor).validateEditorInputState() line: 97 JavaMergeViewer$CompilationUnitEditorAdapter(AbstractDecoratedTextEditor).validateEditorInputState() line: 1009 JavaMoveLinesAction(TextEditorAction).validateEditorInputState() line: 143 JavaMoveLinesAction.runWithEvent(Event) line: 307 ActionHandler.execute(ExecutionEvent) line: 119 Command.executeWithChecks(ExecutionEvent) line: 476 ParameterizedCommand.executeWithChecks(Object, Object) line: 508 HandlerService.executeCommand(ParameterizedCommand, Event) line: 169 WorkbenchKeyboard.executeCommand(Binding, Event) line: 471 WorkbenchKeyboard.press(List, Event) line: 823 WorkbenchKeyboard.processKeyEvent(List, Event) line: 879 WorkbenchKeyboard.filterKeySequenceBindings(Event) line: 570 WorkbenchKeyboard.access$3(WorkbenchKeyboard, Event) line: 511 WorkbenchKeyboard$KeyDownFilter.handleEvent(Event) line: 126 EventTable.sendEvent(Event) line: 84 Display.filterEvent(Event) line: 1190 StyledText(Widget).sendEvent(Event) line: 1002 StyledText(Widget).sendEvent(int, Event, boolean) line: 1027 StyledText(Widget).sendEvent(int, Event) line: 1012 StyledText(Control).traverse(Event) line: 3624 StyledText(Control).translateTraversal(MSG) line: 3606 StyledText(Composite).translateTraversal(MSG) line: 1091 Display.translateTraversal(MSG, Control) line: 4414 Display.filterMessage(MSG) line: 1204 Display.readAndDispatch() line: 3466 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2384 Workbench.runUI() line: 2348 Workbench.access$4(Workbench) line: 2200 Workbench$5.run() line: 495 Realm.runWithDefault(Realm, Runnable) line: 333 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 490 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 113 EclipseAppHandle.run(Object) line: 193 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 366 EclipseStarter.run(String[], Runnable) line: 177 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 39 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 25 Method.invoke(Object, Object...) line: 597 Main.invokeFramework(String[], URL[]) line: 550 Main.basicRun(String[]) line: 505 Main.run(String[]) line: 1237 Main.main(String[]) line: 1213
I'll comment on the code once M4 is cleared.
If I understand correctly, the bug summary should be something like "Embed text editors in compare viewers", since the work here is more general than just adding Java content assist.
You're right John, my original intention was to add only Java Content Assist, but as it came out, with a little more effort I should be able to kill two birds with one stone (see comment 6). The next version of the patch will be attached to bug 169386. This seems to be more appropriate place for it, since as you said, the fix became more general in the meantime. As soon as we decide that the patch is ready to release I will update all dependent bugs, marking them as fixed if the general patch actually fixes them.
The issue has been fixed in bug 169386 - Java Content Assist has been added to the local, editable part. We decided to disable the feature for non-local parts since the results were ambiguous. All patches from this bug are obsolete. Marking as FIXED.
