Bug 127108 - [Field Assist] - KeyEvents and content proposal requests
Summary: [Field Assist] - KeyEvents and content proposal requests
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.2 M6   Edit
Assignee: Susan McCourt CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on: 130681
Blocks: 120237
  Show dependency tree
 
Reported: 2006-02-09 12:26 EST by Susan McCourt CLA
Modified: 2006-03-28 16:43 EST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Susan McCourt CLA 2006-02-09 12:26:08 EST
This bug is spawned from bug #125952, which addressed cumulative filtering of content proposals.  In working on that bug, problems with the timing of the trigger of content proposals were found.  When content proposals are requested as a result of a key event, we need to ensure the content of the target control is updated with the result of the key event.  Clients often go back to the control for its content and cursor position in order to decide how to filter the proposals.  More scenarios to follow
Comment 1 Susan McCourt CLA 2006-02-09 12:39:14 EST
Markus: observations from bug #120237

- type "abc", set caret to after "a", use content assist to insert \t
was: every way I tried to insert the proposal, some of the surrounding text was
lost
expected: text is "a\tbc", regardless of way to insert proposals

scenarios:
- Ctrl+Space, doubleclick with mouse to insert proposal
- Ctrl+Space, use cursor keys and enter key to insert
- Ctrl+Space, type "\", then use mouse to insert
- Ctrl+Space, type "\", then use keyboard to insert

A bit more tricky are proposals that set the caret to somewhere inside the
inserted text:
- type "(?"
was: popup comes up in default order
expected: proposals starting with "(?" are first
The relevance sorting seems to work after typing one more character and then
typing backspace.

- in "abc" with caret after "a", insert proposal (?<=)
expected: text is "a(?<=)bc", caret is after  "="
was: surrounding text deleted, only parts of proposal inserted
see also bug #126138.
Comment 2 Susan McCourt CLA 2006-02-09 12:40:22 EST
critical bug, but is waiting in line behind field assist and undo API issues for M5.  This may not get worked on until after M5.
Comment 3 Susan McCourt CLA 2006-03-02 21:22:52 EST
There were bugs in two areas:
- the RegExContentProposalProvider had an off by 1 error.  I have fixed this and will post a new patch to bug #120237
- there was a bug in the ComboContentAdapter that caused the text after the insertion point to not get inserted at all.  I have released that part of the fix.

The content proposals should be filtering as they did in the original dialog now.

One remaining issue is that the combo content is not inserted at the proper location if the user uses the mouse double click to select the proposal.  It works properly if the user cursors to the proposal and presses enter.  It would appear that the SWT combo is returning an incorrect value (0,0) for Combo.getSelection() when the proposal is inserted by double click.  I will investigate this further.  If this sounds familiar to anyone in platform text land, please let me know.  Right now it looks like an SWT issue but I'll build a test case to be sure.
Comment 4 Dani Megert CLA 2006-03-03 02:52:49 EST
>It would appear that the SWT combo is returning an incorrect
Might related to bug 123951?
Comment 5 Susan McCourt CLA 2006-03-03 14:02:30 EST
That bug (123951) deals with selection index within the drop down.  I couldn't find any SWT bugs dealing with the selection within the text part of a combo, but will build a test case to pin down the exact problem.
Comment 6 Susan McCourt CLA 2006-03-06 20:07:17 EST
This cause of this problem is pretty simple, and is either an SWT bug or "feature."  If an SWT combo box loses focus, it does not restore the previous cursor position when it regains focus.  It instead selects the entire content of the combo.  I've opened bug #130681 against SWT.

This causes the content assist adapter to get the text insertion wrong if the content assist popup receives focus during content assist.  The SWT issue can be worked around, but <sigh> an API change in IControlContentAdapter is required to fix it.  

ContentProposalAdapter is designed to operate on any arbitrary control, with an IControlContentAdapter supplied to set/get the control content and find out other relevant info about the control.  Given that cursor position is not retained by an SWT combo, the content proposal adapter must explicitly remember the cursor position when the content assist popup is opened, track it as keys are typed, and restore it whenever it makes an insertion.  The IControlContentAdapter has always had protocol for getting the cursor position of an arbitrary control, but not for setting it.

I optimistically assumed that the cursor position would not change on a focus change, and unfortunately other bugs masked this problem before API freeze.  While I've opened an SWT bug against this, I'm pessimistic that such long standing behavior will change at this point, especially if a workaround is possible.

cc'ing McQ since this involves an API addition.  The proposal is to add API to IControlContentAdapter and implement this protocol in TextControlContentAdapter and ComboControlContentAdapter.  There are no other clients affected outside the workbench.

	/**
	 * Set the current cursor position in the control. The position is specified
	 * as a zero-based index into the string. Valid ranges are from 0 to N,
	 * where N is the size of the contents string. A value of N indicates that
	 * the cursor is at the end of the contents.
	 * 
	 * @param control
	 *            the control whose position is to be set.
	 * @param index the zero-based index representing the cursor position in the
	 *         control's contents.
	 */
	public void setCursorPosition(Control control, int index);
Comment 7 Susan McCourt CLA 2006-03-07 12:48:41 EST
McQ - We've gotten confirmation that SWT won't address this issue.  Do you approve this API change.  I'm trying to set a record for the percentage of my M6 fixes that require API changes... ;-( 
Comment 8 Mike Wilson CLA 2006-03-07 14:00:28 EST
+1. ok to proceed. agree that the current combo box behavior is bogus, but it is the (OS provided) law.
Comment 9 Susan McCourt CLA 2006-03-08 13:06:30 EST
Fixed >20060308.
After releasing the changes on the field assist side, I had to make some changes to the RegExContentProposalProvider.  I will explain and attach those in bug #120237.

With these fixes and those patches, I tried all of the scenarios Markus described in this bug, as well as special cases I could ascertain from examining the RegEx source code.  In all cases, the behavior of the field assist dialog is identical to the original find/replace dialog.  (Side note:  some of these cases seem slightly unexpected, but at least I have not changed the behavior.  My goal is to simply behave the same.)
Comment 10 Susan McCourt CLA 2006-03-08 13:07:31 EST
cc'ing John and Boris since this is an API change.
Comment 11 John Arthorne CLA 2006-03-08 17:09:58 EST
Note that this is a breaking API change because it involves adding a method to an interface that is specified as implementable by clients. Perhaps a heads up on eclipse-dev would be appropriate, on the chance someone in the community has implemented the interface. As far as I know this is the first breaking change since the API freeze.
Comment 12 Susan McCourt CLA 2006-03-08 19:29:56 EST
Point taken, but I think a heads up in platform-ui is likely enough.  

The API was just introduced in M4 and changed a bit in M5, and is likely only known through bug reports and platform-ui.  There are implementations of it for the most common uses, so I do not think use is widespread.  I'll go ahead and post to platform-ui, and if you strongly feel it should be done in eclipse-dev, I don't mind doing it there, too, just not sure it's necessary.
Comment 13 Jeff McAffer CLA 2006-03-08 23:56:58 EST
I believe the notice should go to eclipse-dev as well.  
Comment 14 Susan McCourt CLA 2006-03-09 13:58:16 EST
okey-dokey.  done.
Comment 15 Susan McCourt CLA 2006-03-28 16:21:43 EST
verified on I20060328-0010, Win XP.
Will also verify on Mac/Linux.
Comment 16 Susan McCourt CLA 2006-03-28 16:36:38 EST
scenarios verified on Mac, I20060328-0010.
Insertions and proposals are as expected.  Problem with arrow keys reported in bug#126138.  Considering this bug verified on Mac
Comment 17 Susan McCourt CLA 2006-03-28 16:43:44 EST
verified on Linux/GTK on I20060328-0100