Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 310601

Summary: [FieldAssist] ContentAssistCommandAdapter enablement not toggled when field has focus
Product: [Eclipse Project] Platform Reporter: Markus Keller <markus.kell.r>
Component: UIAssignee: Susan McCourt <susan>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: ob1.eclipse, pwebster, remy.suen, susan
Version: 3.6Flags: pwebster: review+
ob1.eclipse: review+
Target Milestone: 3.6 RC1   
Hardware: PC   
OS: Mac OS X   
Whiteboard:
Attachments:
Description Flags
patch
none
the correct patch none

Description Markus Keller CLA 2010-04-27 06:37:38 EDT
I20100426-0852

The fix for bug 301196 made bug 199127 reappear (Mac only).
Comment 1 Markus Keller CLA 2010-04-27 08:34:34 EDT
(In reply to bug 301196 comment #20)
I would replace the "CONTROL_ID_PREFIX + hashCode()" hack by
"CONTROL_ID_PREFIX + nextID++" (and a new "private static int nextID = 0;")
Comment 2 Susan McCourt CLA 2010-04-27 11:58:42 EDT
Markus, would you mind running
TextContentAssistCommandAdapterTest.testBug301196CorrectHandlerConflictResolution()
on the Mac to see if it would have caught this failure?

You have to re-enable the tests in the ui FieldAssistTestSuite in order to run them (the test suite has these tests diabled in the builds for other reasons).

If the error is caught by the test, (and corrected by the fix), then I will investigate separating this test out to ensure it runs in the build (and on all platforms).
Comment 3 Markus Keller CLA 2010-04-27 12:31:20 EDT
(In reply to comment #2)
Nope, testBug301196CorrectHandlerConflictResolution() and testHandlerPromptsPopup() are still green.

The last 3 tests fail with:
junit.framework.AssertionFailedError: unable to post event to display queue for test case
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.assertTrue(Assert.java:20)
	at org.eclipse.jface.tests.fieldassist.AbstractFieldAssistTestCase.sendKeyDownToControl(AbstractFieldAssistTestCase.java:168)
Comment 4 Susan McCourt CLA 2010-04-28 14:19:59 EDT
Assigning to M7, there's no reason to introduce a regression and the fix is safe.  Will attach a patch with the proposed fix.  Oleg, I will need help verifying this on a Mac (I trust Markus already did this but I'd feel better with a real patch and verification.)
Comment 5 Susan McCourt CLA 2010-04-28 23:20:56 EDT
moving back to RC1, Markus says this is not urgent.
Comment 6 Susan McCourt CLA 2010-05-03 13:59:38 EDT
Created attachment 166814 [details]
patch

This patch ties adapter enablement to handler enablement rather than including it in the activation expression.  The handler is made a non-anonymous class so that we can expose the enabling protocol.
Comment 7 Susan McCourt CLA 2010-05-03 14:00:22 EDT
Paul, can you please review this patch?
Comment 8 Susan McCourt CLA 2010-05-03 14:01:56 EDT
Oleg, 
can you please verify that this patch actually fixes the observed problem on the Mac?

I can only verify that it still works on Windows.  ;-)

(copied from bug 199127)
---------------------------
To reproduce, open the Find/Replace dialog on a text editor and then toggle the
Regular Expressions checkbox. The lightbulb on the find field is correctly
added/removed, but the availability of Ctrl+Space to open the content assist
proposals popup is only updated when the field has lost focus.

The problem is not reproducible on Windows, probably because the find field
always loses focus on Windows when the checkbox is toggled.
Comment 9 Susan McCourt CLA 2010-05-03 14:10:05 EDT
Created attachment 166818 [details]
the correct patch

this is the real patch.  The other one is from an old bug.
Comment 10 Oleg Besedin CLA 2010-05-03 14:35:35 EDT
(In reply to comment #8)
> Oleg, 
> can you please verify that this patch actually fixes the observed problem on
> the Mac?

works for me.
Comment 11 Susan McCourt CLA 2010-05-03 15:17:29 EDT
Fixed in HEAD >20100503.
Markus, can you please verify in the next build?
Comment 12 Markus Keller CLA 2010-05-05 04:49:10 EDT
Verified in HEAD.