Community
Participate
Working Groups
org.eclipse.ui.ide from HEAD. [EditorMgmt] IWorkbenchPage#openEditors(IEditorInput[], ..) does not set keyboard focus Steps to reproduce: 1. in dev workspace modify org.eclipse.ui.internal.ide.handlers.OpenResourceHandler to use the new API 2. start target workspace 3. in target workspace create project 'P' and file 'f', close editor 4. in target workspace use Ctrl+Shift+R to open 'f' ==> f is opened, editor seems active but keyboard focus is still in the Package Explorer
Hmm.. OK, now I agree, that API is cursed :-). This seems to be a side effect of using deferUpdates() in the WorkbenchPage#openEditors(). Too bad that removing those calls causes extra editor activations.
Created attachment 153533 [details] Patch The source of the problem: in this code path we do editor activation while deferring updates. (Otherwise all editors will be activated.) In text editors, StyledText.parent.isVisible() == false when they gets focus set request. As a result, the StyledText widget ignores set focus request raised during the activation. The patch adds code to perform second request to set focus once deferred updates are processed. This is not the most elegant solution, but all other things I tried had some interesting side effects.
Patch applied to CVS Head.
Sorry Oleg to be a pain but the fix violates the IWorkbenchPage contract that says: * <li>When a part is activated or gets focus: * <ul> * <li>call <code>part.setFocus()</code></li> * <li>fire <code>partActivated</code> event to all listeners</li> * </ul> * </li> While calling setFocus() twice might not be the big deal (you never know), it's a problem that - setFocus() is called again after partActivated() got called where it e.g. might set the focus on something different than in setFocus() and hence get broken - the parent isn't visible yet and the focus not in the part when partActivated is called. This again can lead to unexepcted result.
sigh... it's never easy, is it?
(In reply to comment #4) > Sorry Oleg to be a pain but the fix violates the IWorkbenchPage contract ... (That's IWorkbenchPart.) That is debatable as workbench code does do everything outlined there on part activation. The Javadoc does not say "nothing else happens" :-). To me, setting the focus again is the simplest change with the lowest risk of breaking anybody. It is similar to, say, what happend when a user switches from Eclipse to another app and then back to Eclipse. I guess I can do the whole proper activation request after deferred updates are processed - as opposing to simply setting the SWT focus.
Created attachment 153772 [details] Patch doing full activation
Verified in I20091207-1800.
>(That's IWorkbenchPart.) Yep of course. I cannot see a difference with your latest patch. Calling setFocus() twice is not nice but I could live with it but that the widgets don't have the focus yet when partActivated(IWorkbenchPart) is called is not good.
(In reply to comment #10) > I cannot see a difference with your latest patch. The second patch is doing full activation as you have experessed a concern in the comment 4 that simply setting focus won't be good enough. Could you provide a code snippet or an example in Eclipse where the functionality added in the "Patch doing full activation" is going to create a problem?
>Could you provide a code snippet or an example in Eclipse where the >functionality added in the "Patch doing full activation" is going to create a >problem? Just imagine a listener with this code in partActivated(...): if (myControl.isFocusControl()) doSomethingImportant() The important code will not be called with your fix. I don't have time to look through the SDK and poll clients whether such code exists. And as I've tried to explain before: the API contract in IWorkbenchPart says: * <li>When a part is activated or gets focus: * <ul> * <li>call <code>part.setFocus()</code></li> * <li>fire <code>partActivated</code> event to all listeners</li> * </ul> * </li> Which clearly indicates that I can expect the part's main control to have focus because setFocus() specifies: * Asks this part to take focus within the workbench. Parts must * assign focus to one of the controls contained in the part's * parent composite.
(In reply to comment #12) > Just imagine a listener with this code in partActivated(...): > if (myControl.isFocusControl()) > doSomethingImportant() That code *will* be called because we do full activation after ensuring that tabs are properly positioned in the tab folder.
>That code *will* be called because we do full activation after ensuring that >tabs are properly positioned in the tab folder. On what tests is your strong "*will*" based? I guess the difference here is that I actually tested it while you assume it works ;-) Here's how you can test it: 1. get 'org.eclipse.ui.workbench.texteditor' from CVS 2. add the follwing code to AbstractTextEditor.ActivationListener.partActivated(IWorkbenchPart): if(fSourceViewer.getTextWidget().isFocusControl()) System.out.println("xxx"); 3. add a breakpoint to the line with System.out 4. repeat steps from comment 0 and observe: breakpoint is hit when opening the file normally but not when using Open Resource that uses the new API NOTE: >1. in dev workspace modify > org.eclipse.ui.internal.ide.handlers.OpenResourceHandler to use the new API This can be done by applying the patch I attached to bug 178229.
The missing focus (see test case in comment 14) is still not fixed in 3.8 and 4.2.
Dani, when I open a file using the resource dialog it appears to be setting the KB focus correctly in both 3.8 and 4.2... 1) Hack the OpenResourceHandler to use the multi-open API. 2) Create "P" and "f" as indicated 3) Close any open editors 4) Open 'f' using the OpenResource dialog start typing...the editor gets the text What am I missing ?
Created attachment 220936 [details] Here's a screen cap taken immediately after the open using the dialog Do you have any preference mods that might be affecting this ?
(In reply to comment #16) > Dani, when I open a file using the resource dialog it appears to be setting > the KB focus correctly in both 3.8 and 4.2... > > 1) Hack the OpenResourceHandler to use the multi-open API. > 2) Create "P" and "f" as indicated > 3) Close any open editors > 4) Open 'f' using the OpenResource dialog > > start typing...the editor gets the text > > What am I missing ? You list different steps than those I give. The point is, that the focus is not there yet when it should (part activated ==> must have focus). Simply try the given steps ;-).
Perhaps I'm missing the point here. I just added the listener code and now see the effect you describe...very strange because the text editor does end up with the focus.
Just to avoid any confusion: in 4.2 my test case will probably even fail on the old/current Open Resource scenario due to bug 375576 (I did not verify that).
(In reply to comment #19) > Perhaps I'm missing the point here. I just added the listener code and now > see the effect you describe...very strange because the text editor does end > up with the focus. Whether it "looks" good and works in the end does not justify wrong behavior/state when the listeners are informed.
The real question is whether there are differences in behavior depending on how the file is opened... Double-click, right-click -> Open, F3, Open Resource, Open Type If not then if there's an issue it doesn't really have anything to do with *this* defect does it ?
(In reply to comment #22) > The real question is whether there are differences in behavior depending on > how the file is opened... > > Double-click, right-click -> Open, F3, Open Resource, Open Type > > If not then if there's an issue it doesn't really have anything to do with > *this* defect does it ? Sorry, I don't get what you try to say or avoid. Yes, at least in 3.8 there were differences compared to the specification/APIs. And you can simply try the steps from comment 14.
Ooops, I didn't realize that this defect *is* specifically about the focus...thought is was the 'mementos' bug I already have a fix ready for this in 4.x which I'll go over with Paul Monday...it's just a case of moving the 'firePartActive' out of the e4 listener and add it directly after we set the focus... This will be in early next week and will fix all the cases I've been able to check so that we see 'xxx' however the part is opened. On 3.x I'll (where it currently doesn't work) I'll see what I can do when implementing the new API...
Created attachment 222248 [details] Patch to move the part activation firing until after the focus is set
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=358512f39dc0be2f7af3ed06229c9aa4c9db5074 Moves the 'firePartActivated' call to a point immediately after the focus is set. The snippet provided now exhibits the same behavior on both 3.8 and 4.x.
(In reply to comment #26) > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?h=R4_2_maintenance&id=358512f39dc0be2f7af3ed06229c9aa4c9db5074 > > Moves the 'firePartActivated' call to a point immediately after the focus is > set. > > The snippet provided now exhibits the same behavior on both 3.8 and 4.x. Sorry if I can't follow here: The bug was reported against 3.6 and no fix was provided for 3.8.x yet (if I read the Git repo content and comment 24 correctly). In that case, it would be wrong if you get the same results for 3.8 and 4.2.2 when using the steps from comment 14. Also, I can't see the fix in 'master' and hence this bug can't be marked as FIXED yet.
Eric, this fix should be in master now, can you confirm it's in M4? PW
(In reply to comment #28) > Eric, this fix should be in master now, can you confirm it's in M4? > > PW I verified that it is fixed in 'master' and 'R4_2_maintenance'.
