Community
Participate
Working Groups
I20090114-1322. 1. make sure you are in double-click mode where the editor must get focus on double click 2. enable General > Always run in background 3. in the History view compare two files by using double-click or via context menu > Compare Current with Local ==> editor does not get focus. I guess this is a regression introduced by enhanced Java compare and should get fixed for M5 as this forces an extra click (or key binding shortcut) on users to transfer the focus to the editor.
Editor does not get focus because check in line 446 of CompareEditor[1] returns false. The active part from the check is History View in this case. For txt compare the active part is Compare Editor so focus is set properly. The other thing I've observed is that when I stop and resume on a breakpoint at org.eclipse.compare.CompareEditorInput.setFocus() the active part for Java compare in the check is Compare Editor and focus is set properly. [1] private boolean isActive() { return getSite().getPage().getActivePart() == this; }
>because check in line 446 of CompareEditor[1] Is that new? Note that this bug didn't happen before.
(In reply to comment #2) > >because check in line 446 of CompareEditor[1] > Is that new? Note that this bug didn't happen before. No, I didn't touch it. What has changed is the fact that the active part is different for txt and Java compare.
I think I'm getting closer to the real cause of the situation. While configuring SourceViewer in JMV[1] we set an input for the editor adapter[2]. This runs a runnable operation. WorkbenchWindow executes the runnable, but when doing this it forces the focus to be set back on the History View[3]. This in consequence activates the History View and results in isActive() check (mentioned in comment 1) to return false, thus not setting focus on Compare Editor. [1] org.eclipse.jdt.internal.ui.compare.JavaMergeViewer.configureTextViewer(TextViewer) [2] org.eclipse.jdt.internal.ui.compare.JavaMergeViewer line 271 => org.eclipse.ui.texteditor.AbstractTextEditor.init(IEditorSite, IEditorInput) [3] ApplicationWindow line 807
Sorry, didn't make it for M5. We will investigate it and look for possible solutions during M6.
*** Bug 263575 has been marked as a duplicate of this bug. ***
Created attachment 125374 [details] Patch v01 A dirty patch that proves that not setting the input from the runnable[1] helps a lot here. I have no experience in this area, but I guess the way setting the input is done right now is on puprose. The patch only tries to pinpoint the real cause of losing the focus. Anyway, with the patch applied the compare editor gains the focus when expected. [1] local variable "runnable" in org.eclipse.ui.texteditor.AbstractTextEditor.internalInit(IWorkbenchWindow, IEditorSite, IEditorInput)
Created attachment 125375 [details] mylyn/context/zip
The other thing that the Patch v01 shows is that the not-given-focus issue doesn't necessarily cause bug 261530.
From some debugging and looking at the code I think that problem is created by initialization of the CompareEditor. In the scenario described in this bug, the workbench will call setFocus() on the CompareEditor after the editor has been created. However, that call will be ignored as the internals of the CompareEditor are created in a separate asynch job later on. In this particular scenario a workbench runnable is called in the time between the original ignored setFocus() and the time CompareEditor is ready. Workbench runnables try to preserve the OS focus. As the CompareEditor ignored setFocus(), the OS focus is still on the Tree in the HistoryView. After a runnable is run, it sets focus back to the OS widget, now making workbench switch its idea of where the focus is. It is possible to imagine several ways to fix this: A) The simple and crude - add setFocus() to the end of the Job created by CompareEditor#initializeInBackground() Pro: simple, one line fix, works for this bug Con: fixes the focus problem, there might be others. Also, in theory, if initialization takes long this might cause focus to shift unexpectedly as user started doing something else B) Make CompareEditor initialization to be done synchronously. Probably not going to happen as somebody somewhere is bound to use compare editor to compare something really complicated :-). (From *my* practical perspective as an IDE user, compare action is reasonably fast, there is no need to make it a separate job). C) Real solution - make CompareEditor produce initial contents (aka, editor area and text saying "Working...") to be replaced later with real contents. I think it already has the code to do it (see CompareEditor.INITIALIZING). It might be just a question of creating CompareEditorInput.fComposite earlier in the lifecycle, but I somehow doubt it will be that easy :-). Personally, I'd go for either (B) or (C) as I have a feeling that the incomplete editor initialization might produce other problems, aside from setting focus. ================= Some code points: WorkbenchPage.openEditor - opens CompareEditor; last step is to activatePart, which calls setFocus. The setFocus eventually finds its way into CompareEditorInput#setFocus() => does nothing because fComposite is null CompareEditor#initializeInBackground => runs a Job that eventually calls CompareEditorInput#createCompareControl() CompareEditorInput#createContents(Composite parent) <= this creates fComposite [In the meantime WorkbenchWindow(ApplicationWindow).run() does something and sets focus back to the OS active widget, in this case Tree from the history view causing it to become an active view for the workbench: Control currentFocus = display.getFocusControl(); <= at this time this is a Tree // ... runs runnable currentFocus.forceFocus(); Which causes it to become active. This is the part that Patch v01 eliminates.] =================
Created attachment 126775 [details] Illustrative patch - not meant to be released Boris stopped by and in 30 seconds produced this patch that shows code from the comment 10 and a hack that fixes it. (Obviously we should not use RuntimeException to pass results; this is just a quick way to illustrate the problem and a possible approach to fixing it.)
Created attachment 126832 [details] Patch v03 Thanks Oleg! You and Boris helped a lot. I totally agree with everything you wrote in previous comments. I would pick C but looking at your latest patch from Boris I think this is the best thing we can do, in terms of both simplicity and effectiveness. I've just tweaked it a little bit with a piece of code already proposed on bug 261530 (in Workaround v03). Thanks again, I greatly appreciate all your help.
Released to HEAD, available in builds >N20090226-2000. Thanks guys!
