Bug 177331 - [Forms] Key binding actions should not go to the editor while header has focus
Summary: [Forms] Key binding actions should not go to the editor while header has focus
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: User Assistance (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P1 major (vote)
Target Milestone: 3.3 M7   Edit
Assignee: Curtis d'Entremont CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-03-14 09:41 EDT by Christof Marti CLA
Modified: 2007-04-25 13:58 EDT (History)
5 users (show)

See Also:


Attachments
Patch for forms.examples to reproduce the problem (3.62 KB, patch)
2007-03-22 04:16 EDT, Christof Marti CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christof Marti CLA 2007-03-14 09:41:01 EDT
3.3M5

When the focus changes between header and page folder the key binding service and all nested services need to be (de-)activated (like in MultiPageEditorPart.setFocus() and .pageChange()). Otherwise actions may target the page while the header has focus.
Comment 1 Christof Marti CLA 2007-03-14 11:46:58 EDT
The patch attached to bug 177356 includes a fix for this one.
Comment 2 Curtis d'Entremont CLA 2007-03-21 12:55:32 EDT
I'm having some difficulty understanding the problem here.. can you give me an example of an action you would want to be enabled in the pagebook but not in the header? I'm trying to come up with a reasonable test case.

Also, do you have an example of a nested service so I can test this part as well? The patch on bug 177356 is using deprecated and provisional API (INestable) which I'd like to avoid if at all possible.
Comment 3 Christof Marti CLA 2007-03-22 04:16:51 EDT
Created attachment 61651 [details]
Patch for forms.examples to reproduce the problem

To reproduce:
- open SingleHeaderEditor
- change to fourth page
- type some text in the page
- type some text in the header
- press shift-home while header text has still focus
-> the page action gets executed

The IKeyBindingService part of the patch would fix this case, I think the INestable part might still be necessary though. I deduced the patch from MultiPageEditorPart.changePage(int) and MultiPageEditorPart.setFocus(int), there does not appear to be a "current" story that is neither deprecated nor experimental.
Comment 4 Curtis d'Entremont CLA 2007-03-22 14:48:29 EDT
Thanks Christof, I can see the problem clearly now. It appears the problem is more general than I thought - it affects even regular single-page text editors.

To reproduce this:

- Check out the readme sample, org.eclipse.ui.examples.readmetool
- Modify the ReadmeControlContribution with this content:

public class ReadmeControlContribution extends WorkbenchWindowControlContribution {
	/* (non-Javadoc)
	 * @see org.eclipse.jface.action.ControlContribution#createControl(org.eclipse.swt.widgets.Composite)
	 */
	protected Control createControl(Composite parent) {
		Text text = new Text(parent, SWT.BORDER);
		text.setText("Type search text"); //$NON-NLS-1$
		return text;
	}
}

- Launch, and note the Text field in the trim. Imagine it is a general purpose workbench search field.
- Open an untitled text editor, type something in it, copy some text to the clipboard
- Click on the trim text field and try to paste it with Ctrl-V.

Bug: The pasted text goes to the editor, not the text field with focus.

In general, the problem happens when the editor or page is the active part but does not have keyboard focus. I'm not sure whether the problem is only with text editors or editors in general. But for text editors, as soon as keyboard focus leaves the StyledText widget (I'm guessing that's what it is), the key bindings need to react.

Sending to UI for further investigation. I'm afraid this one will probably not be fixed in time for M6 (sorry for taking long to debug it)
Comment 5 Christof Marti CLA 2007-03-22 14:53:39 EDT
We have a trim that shows the problem you just described.
Comment 6 Paul Webster CLA 2007-03-22 15:01:56 EDT
The general case was fixed for M6 as part of bug 177806 specifically to fix the Trim edit control case.

A text control that's not part of the workbench lifecycle can register with the IFocusService, at which point it can provide handlers.

PW
Comment 7 Curtis d'Entremont CLA 2007-03-22 15:47:03 EDT
Thanks Paul. I think that bug is a bit different from this one.. that one involves adding new handlers specifically for the trim control, but here we want the text editor's handlers to no longer be called.. otherwise we would have to 'override' all of them in order to completely hide them, unless I'm missing something. For example, what about Shift-Home/End, and all those other key bindings
Comment 8 Paul Webster CLA 2007-03-22 15:57:11 EDT
You're correct, you have to override all of them that you want to override ... or deactivate them, like the MPEP does on page change.

You're MPEP is the active editor, and the MPEP has supplied handlers for cut, copy, paste, selectAll, home and end (although they're called something different).

That's why the MPEP uses nested services to active and deactivate sub-editor handlers.

PW
Comment 9 Curtis d'Entremont CLA 2007-03-22 16:24:20 EDT
(In reply to comment #8)
> You're correct, you have to override all of them that you want to override ...
> or deactivate them, like the MPEP does on page change.

Did you mean as a temporary workaround or as the final solution?
Comment 10 Paul Webster CLA 2007-03-22 18:56:13 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > You're correct, you have to override all of them that you want to override ...
> > or deactivate them, like the MPEP does on page change.
> 
> Did you mean as a temporary workaround or as the final solution?

You're kinda caught in between ... MultiPageEditorPart does the work to hook up pages to the Command Framework for pages it considers to be internal editors ... but this shared header puts you in a state where your MPEP is active and your inner editor is active ... but the shared form has focus.  It's outside of the lifecycle of the MPEP, and it shows.

One way to fix at least this case is to use the internal API and treat the shared form as a page without an editor (disable the services on focusGained and enable them again on focusLost).

In 3.3, our API solution for "I have a control that needs to override the current workbench behaviour" is IFocusService.  It allows the control to participate in Command Framework.  It can be used for the effect of "While this control has focus, use the default cut/copy/paste/selectAll".

PW
Comment 11 Paul Webster CLA 2007-03-23 10:01:28 EDT
(In reply to comment #10)
> In 3.3, our API solution for "I have a control that needs to override the
> current workbench behaviour" is IFocusService.  It allows the control to
> participate in Command Framework.  It can be used for the effect of "While this
> control has focus, use the default cut/copy/paste/selectAll".

There is also API (available in 3.2) to treat a Control as a plain SWT control by disabling the keybinding filters - IBindingService#setKeyFilterEnabled(*).  The 2 choices for something outside the normal workbench lifecycle events are to join in with the IFocusService or disable the key filter (temporarily).  

PW
Comment 12 Curtis d'Entremont CLA 2007-03-23 10:35:46 EDT
Thanks for explaining Paul. Sending back to UA now that I understand what needs to be done.
Comment 13 Paul Webster CLA 2007-03-23 12:33:36 EDT
If you decide to go the IFocusService route, the XML that will reset cut/copy/paste/selectAll for you is in https://bugs.eclipse.org/bugs/show_bug.cgi?id=152736#c11

PW
Comment 14 Erich Gamma CLA 2007-04-25 10:16:28 EDT
This is a bug that Jazz would like to see fixed for 3.3. 
Comment 15 Curtis d'Entremont CLA 2007-04-25 12:54:27 EDT
Paul, I need your help again on this one. I looked into both approaches (IFocusService and IBindingService) but there are problems with both:

- The header client area is open-ended (client can add whatever they want) but IFocusService takes in specific controls. We could crawl the widget hierarchy but who knows if people are adding/removing fields, for example.
- Turning off all key bindings everywhere is too strong - we want people to still be able to do things like Ctrl-Shift-T to open types.

We need a way to turn of only those key bindings that apply to the current editor page below.

Is there any way to do this other than using internal code? If not, can we form an agreement to not change the needed internal interfaces until there is an API solution?
Comment 16 Paul Webster CLA 2007-04-25 13:25:49 EDT
(In reply to comment #15)
> - The header client area is open-ended (client can add whatever they want) but
> IFocusService takes in specific controls. We could crawl the widget hierarchy
> but who knows if people are adding/removing fields, for example.

I thought about this when it went by this morning ... if the clients are creating the header, the IFocusService would not be so useful.

> 
> We need a way to turn of only those key bindings that apply to the current
> editor page below.
> 
> Is there any way to do this other than using internal code? If not, can we form
> an agreement to not change the needed internal interfaces until there is an API
> solution?

You're correct, it is mostly internal code.  For now we can use the same pattern in SharedHeaderFormEditor that we use in MultiPageEditorPart (which I think was Christof's original suggestion), and then work in 3.4 to provide the API version of this (or slightly better) solution.  I'll have a quick look at this later on today.

PW
Comment 17 Curtis d'Entremont CLA 2007-04-25 13:58:46 EDT
Thanks Paul. I've applied Christof's patch as-is, and opened bug 184074 to track the future enhancement for 3.4.