Community
Participate
Working Groups
by providing an activation expression Eclipse can correctly resolve conflicts should the occur. applies to org.eclipse.ui.fieldassist.ContentAssistCommandAdapter see @org.eclipse.ui.internal.handlers.HandlerAuthority.resolveConflicts(String, SortedSet, MultiStatus)@ for details on how handler conflicts are resolved. The handler should only be active when the control has focus: bc. private void activateHandler() { if (activeHandler == null) { activeHandler = handlerService.activateHandler(commandId, proposalHandler,new ActiveFocusControlExpression(getControl())); } } the focus control expression would look something like this: bc.. public class ActiveFocusControlExpression extends Expression { private final Control focusControl; public ActiveFocusControlExpression(Control control) { focusControl = control; } @Override public void collectExpressionInfo(ExpressionInfo info) { info.markDefaultVariableAccessed(); // give it a very high priority info.addVariableNameAccess(ISources.ACTIVE_SHELL_NAME); info.addVariableNameAccess(ISources.ACTIVE_WORKBENCH_WINDOW_NAME); } @Override public EvaluationResult evaluate(IEvaluationContext context) throws CoreException { if (Display.getCurrent() != null && focusControl.isFocusControl()) { return EvaluationResult.TRUE; } return EvaluationResult.FALSE; } }
If you wanted to use an expression, you would register your control with org.eclipse.ui.swt.IFocusService and then your ActiveFocusControlExpression evaluate(*) would have something like: if (context.getVariable(ISources.ACTIVE_FOCUS_CONTROL_NAME)==focusControl) { return EvaluationResult.TRUE; } return EvaluationResult.FALSE; PW
Thanks for the comment. Do you agree in principle that ContentAssistCommandAdapter should always use an expression? The problem is that without expressions, handler conflicts are not resolved correctly.
(In reply to comment #2) > Thanks for the comment. Do you agree in principle that > ContentAssistCommandAdapter should always use an expression? The problem is > that without expressions, handler conflicts are not resolved correctly. I'm not sure how this was meant to be used. Do we ship with a default handler? If there is one, then this code would generate a conflict. PW
(In reply to comment #3) > I'm not sure how this was meant to be used. Do we ship with a default handler? > If there is one, then this code would generate a conflict. I don't believe there is a handler, but even if there was don't you think that content assist on the field should take priority when the field has focus? Currently there is no expression, so the handler becomes lowest priority.
(In reply to comment #3) > (In reply to comment #2) > > Thanks for the comment. Do you agree in principle that > > ContentAssistCommandAdapter should always use an expression? The problem is > > that without expressions, handler conflicts are not resolved correctly. > > I'm not sure how this was meant to be used. Do we ship with a default handler? > If there is one, then this code would generate a conflict. > > PW This adapter was meant to be used when an app wants the content assist command to be applied for a specific control. (to invoke field assist using the content assist key sequence). Currently it activates the handler manually when it gets focus and deactivates it when it loses focus. I see how an expression could be used instead, but I'm not sure I understand the problem of not using one. (In reply to comment #4) > I don't believe there is a handler, but even if there was don't you think that > content assist on the field should take priority when the field has focus? > Currently there is no expression, so the handler becomes lowest priority. What conflict are you seeing? It would have to be caused by some component registering a content assist handler on something other than focus. David, you are saying that the use of the expression instead of dynamic registration causes the correct handler to win in case of conflict? Paul, can you confirm?
(In reply to comment #5) > This adapter was meant to be used when an app wants the content assist command > to be applied for a specific control. (to invoke field assist using the > content assist key sequence). Currently it activates the handler manually when > it gets focus and deactivates it when it loses focus. I see how an expression > could be used instead, but I'm not sure I understand the problem of not using > one. I'm not suggesting that we change when the handler is registered/activated. The problem is that if another handler is already registered, the conflict resolution algorithm is used and handlers with no enablement expression get lowest priority when resolving conflicts. Thus the wrong handler is used. > What conflict are you seeing? It would have to be caused by some component > registering a content assist handler on something other than focus. I've got a multipage editor with a text editor in one tab and a forms-based editor in another tab. Content assist works fine in the forms-based editor unless the forms-based editor is used after the user has switched to the text editor and back again. The conflict arises because the text editor has not properly disabled or unregistered its handlers. > David, you are saying that the use of the expression instead of dynamic > registration causes the correct handler to win in case of conflict? Yes, providing that the expression has high enough priority. (The one with the highest priority wins, which is why @collectExpressionInfo(ExpressionInfo info)@ calls @info.markDefaultVariableAccessed()@)
(In reply to comment #5) > > This adapter was meant to be used when an app wants the content assist command > to be applied for a specific control. (to invoke field assist using the > content assist key sequence). Currently it activates the handler manually when > it gets focus and deactivates it when it loses focus. I see how an expression > could be used instead, but I'm not sure I understand the problem of not using > one. > It is as David mentions. If you just active the handler, then it has an implicit priority based on where you got the IHandlerService from ... if you get it from PlatformUI.getWorkbench(), then you have a default handler (lowest priority). This isn't necessarily a bad thing, but that means that any other contribution with an expression that can be active when the command is invoked will override it. IFocusService and the focus control expression will give your handler a fairly high priority (as the specific Control should override most anything, and IFocusService is the command replacement for the TextHandler action-based control API). PW
David, if you could prepare a patch and a test case, I can look this over for M6.
Created attachment 157839 [details] the unit test was committed, but not the patch to the code. I've attached a fix and unit test. I looked around and it seems that most expressions used by the platform are declared as anonymous inner classes, so I did the same here. I tried using IFocusService however when registering a control one must pass an id for the control. It was not clear to me what id one would provide, or if providing an id for the control would cause problems if someone had already registered the control with a different id. Instead I ended up providing the original test in evaluate(). It seems to me that there's no need to track focus again somewhere else anyways, since ContentAssistCommandAdapter already does that. Please let me know if you need anything else. I look forward to seeing this in M6!
Created attachment 157840 [details] mylyn/context/zip
I forgot to mention: the Unit test succeeds, however if the test is run against the original unpatched ContentAssistCommandAdapter it fails. (this is good)
(In reply to comment #11) > I forgot to mention: the Unit test succeeds, however if the test is run against > the original unpatched ContentAssistCommandAdapter it fails. (this is good) Yes, very good. Often it's the lack of this kind of test (proving the problem) that costs me the most time, and therefore decreases the chances of getting a patch in. I'll take a look at this early in M6.
punting to M7 as my plate runneth over with API-freeze bugs.
Created attachment 164923 [details] alternate patch This patch uses the focus service and a corresponding expression to achieve similar results.
I looked at this and it didn't feel right to be activating the handler by hand, and then using an expression to check the same state. So I tried to use Paul's suggested approach in comment #1, to use the focus service to track the control and register the handler with an expression that checks focus with the focus tracker. I removed all the code that does dynamic activation and deactivation of the handler. The everyday behavior as exemplified in the find/replace dialog works correctly (good). However the test case you provided fails in this case, the handler does not get activated. I'm not sure where the failure is and it's hard to debug given it involves focus. Before I get too much deeper into this path: David - can you try this patch with the actual scenario that had the problem and report back? Paul - can you look at this patch and comment on whether this *should* work and whether you have any clue why a handler that uses the active shell variable would "win" over the handler with the focus control? ie, what's wrong with this test case. I can run some further tracing on this but I thought I'd have Paul comment on whether this was the right track. (In reply to comment #9) > I tried using IFocusService however when registering a control one must pass an > id for the control. It was not clear to me what id one would provide, or if > providing an id for the control would cause problems if someone had already > registered the control with a different id. Instead I ended up providing the > original test in evaluate(). It seems to me that there's no need to track > focus again somewhere else anyways, since ContentAssistCommandAdapter already > does that. I think the main thing with id is that it be qualified so that we don't accidentally conflict with some other plugin's choice of id. Since we don't check the control id in the expression, it otherwise doesn't really matter what we do here, and I elected to use the same id for all content assist controls. If we find later we need a unique id we could append some kind of unique id on the end of it (the control's hash?)
(In reply to comment #15) > The everyday behavior as exemplified in the find/replace dialog works correctly > (good). However the test case you provided fails in this case, the handler > does not get activated. It might have to do with the ID ... apparently IDs need to be unique per control, otherwise the source provider won't fire an update. PW
(In reply to comment #14) > Created an attachment (id=164923) [details] > alternate patch The patch looks good. PW
(In reply to comment #16) > (In reply to comment #15) > > The everyday behavior as exemplified in the find/replace dialog works correctly > > (good). However the test case you provided fails in this case, the handler > > does not get activated. > > It might have to do with the ID ... apparently IDs need to be unique per > control, otherwise the source provider won't fire an update. > > PW So it was right there in front of me. (David, disregard my previous "id's don't matter if you aren't checking them" comment!) I appended a hash onto the id and the test passes. Fixed in HEAD >20100415. Thanks for pushing on this, David.
Comment on attachment 157839 [details] the unit test was committed, but not the patch to the code. Marking iplog flag since the unit test was used. The code changes to ContentAssistCommandAdapter were not used. Also please note the test is not run in the builds because there are problems with the suite on some platforms (see bug 275393). I do run this suite when I'm making changes as they tend to work correctly on windows platforms.
I was a little reluctant to use the id+hash approach, since there's a very slim possibility that the hash may not be unique. I know that the chances are slim, but... ;) In any case, I'm glad to see it fixed. Susan, Paul, thanks for driving this one home!
forgot to mark fixed...
verified on I20100425-2000, Win7. I ran TextContentAssistCommandAdapterTest.testBug301196CorrectHandlerConflictResolution() locally (the test suite is disabled in the builds for other reasons). David, if you have a chance to verify the end-user scenario that triggered this bug, that'd be great.
This fix caused bug 310601 on the Mac.
