Bugzilla – Full Text Bug Listing
| Summary: | [Field Assist] - API - Specification of invoking key stroke/sequence | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Susan McCourt <susan> |
| Component: | UI | Assignee: | Susan McCourt <susan> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bradleyjames, daniel_megert, douglas.pollock, markus.kell.r, michaelvanmeekeren |
| Version: | 3.2 | ||
| Target Milestone: | 3.2 M5 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
|
Description
Susan McCourt
Doug's response:
A single key event cannot be equated to a KeySequence.
The proper way to do this is probably a little bit complicated, as it should
integrate cleanly with the workbench. I don't have a lot of time to work on
this, so I'm not sure what to suggest.
If this existed at the workbench level, then you would provide an
IHandlerService to ContentProposalAdapter. When the corresponding control
gains focus, you would:
IHandlerService.activateHandler(contentAssistCommandId, handler);
where handler is the guy that carries out the work normally done in the
control listener. When you lose focus, you would deactivate.
(If you have more time, then the ideal solution would be to add a
ControlFocusSourceProvider (implements ISourceProvider), hook it up to
HandlerService, and then create an expression that checks the current focus
control. You then activate the handler once with the expression, and the
handler service would manage conflicts and activation/deactivation. See
Workbench.init to see how source providers are hooked into services.)
You can take the approach you are taking, but it has weaknesses:
+ Commands are bound to KeySequence instances, not KeyStroke
+ Your listener code would need to mimick the complexities of the listener in
WorkbenchKeyboard (e.g., listen to traverse as well, how to handle special
keys, prefix masking, key binding state etc.)
+ None of the command execution listener code will be hit
Quick thoughts:
+ A JFace interface which IHandlerService could implement -- allowing it to be
passed down into JFace
+ A service at the workbench level with which ContentProposalAdapters can be
registered.
But, with either of those suggestions, I think you still need to provide a
simple KeyDown listener approach for JFace standalone applications.
okay, I see two things to do here: 1 - agree on the preferred way to specify invoking key sequence for stand-alone JFace users. Options are to: (a) leave as is, since a keystroke can be easily mapped to a key event. (use it as a convenient data structure) (b) change to a lower level specification, such as SWT keycode. Since it's not using the complete key bindings story, using the keystroke class may be misleading. 2 - define the story for workbench-level users and complete key bindings. Something whereby the client (dialog) registers their ContentProposalAdapters with the workbench. This means we need API on the adapter side to invoke the popup, as well as define the mechanism for registering adapters. note to self: once resolved, there should be dynamic API for setting and querying the keystroke in addition to the constructor parameters. Since autoactivation characters must be dynamically settable for platform text (see bug# 120458), it makes sense that the keystroke should be also. Doug & Dani - I'm particularly interested in your thoughts. Any other ideas are welcome, too... I went down the "ideal" path to see what it felt like: - defined ISources.ACTIVE_CONTROL - ControlFocusSourceProvider monitors focus events on the display level and fires changes on this source - this source provider is hooked into the HandlerService - an expression to match the controls is created, - a handler to invoke the content assist is defined - clients register the control with the service when they install content proposal adapters (this could be done for clients by providing a workbench-level content proposal adapter that does this for them) I got this far and then stopped...(ie, it's not working, but I see how it could...) I stopped because this all feels very heavyweight to me. I agree it's the most idealistic in terms of handlers/services, and in allowing multiple keystrokes to be used to trigger content assist. But I'm worried about the overhead of watching all the focus event changes when we only care about a few fields at a time. We haven't done much for the client except to allow multiple keystroke sequences to be supported for content proposal, which I don't think is typical. In fact, auto-activation is probably a more important scenario (such as typeahead completion). All I really want from the commands/bindings/handlers world is a way to know when a sequence is invoked for a particular control. Since fields are fairly short lived, I think I'd feel better if there were some way I could tell a handler service "watch this control and tell me if this command is invoked" and then I'd unregister the control when I didn't want it watched anymore. I realize there are reasons for having to centrally manage this, different ISource priorities and scopes, etc. But I don't feel like field based content assist is a good enough reason to introduce such a heavy-handed source provider. If there were already a control focus source provider for some other good reason, then I'd feel better about this approach. So... I propose we keep it simple for now: - define a command for content proposal at the org.eclipse.ui level (use the same key definitions currently defined in the text content proposal command) - provide API for clients to get the command name so they can use it to get the key sequence (this is what platform text does today) - this key sequence is given to the content proposal adapter Then we are back to the question of how best to specify the sequence when creating the content proposal adapter - Specify KeySequence...this is misleading because all I would do inside is pull off the first keystroke and watch for it. However, it keeps clients from having to do the same thing everywhere. It leaves it open for a better solution in the future. - Specify KeyStroke...more "honest" but then all clients will repeat the pattern of getting the KeySequence for a command and stripping off the first keystroke (this pattern is already present today) I'm leaning toward doing the following: - keep the ContentProposalAdapter API as is. A KeyStroke for explicit invocation can be specified in the constructor. KeyStroke is basically serving as a convenient data structure, you get the parsing of key names, etc. - make a workbench-version of the content proposal adapter where you can specify the command ID that should activate the proposal provider. The API for auto activation characters is also provided, but we don't expose the notion of an explicit invoking keystroke. This gets us a simple API and leaves me all the discussed implementation options hidden within the workbench-level adapter: a) get the binding for the specified command and strip off the first keystroke, pass it through in the call to the ContentProposalAdapter constructor(simple but limited) b) activate/deactivate the handler as the control gains and loses focus as Doug describes c) if there is ever a ControlFocusSourceProvider provided in the future for other reasons, the workbench adapter could drop the focus management code and just register an expression for comparing the focus control to the adapter's control (In reply to comment #4) > I stopped because this all feels very heavyweight to me. The focus control tracking is not strictly necessary, if that's the only part you feel is awkward. My other suggestion was simply that we track focus when appropriate, and activate/deactivate a handler when appropriate. > We haven't done much for the client except to allow multiple keystroke > sequences to be supported for content proposal, which I don't think is > typical. We would also have done the following: allow tracking and playback of command execution (useful for cheatsheets, macros, instrumentation, etc.), allow the user to change the key binding after the field is created, take advantage of all the SWT/event/platform knowledge encapsulated in WorkbenchKeyboard, allow third-party plug-ins to override the behaviour of content assist, and every other feature that gets added to commands as the product matures. > But I don't feel like field based content assist is a good enough reason to > introduce such a heavy-handed source provider. If there were already a This I can agree with. I only mentioned it because tracking the focus control is something that's done in other places as well (e.g., TextActionHandler). > - define a command for content proposal at the org.eclipse.ui level (use the > same key definitions currently defined in the text content proposal command) Defining a duplicate command is not a good idea. There should only be one command, if at all possible. If necessary, we can move the content assist commands and bindings from org.eclipse.ui.workbench.texteditor to org.eclipse.ui. Oh, I definitely understand the coolness of commands ;-) And as you say, I can activate the handler myself. I just wanted to try the full-on approach to understand it and see how it felt. I can't get around implementing my own keyboard stuff since field assist is defined at the JFace level, but the approach I'm taking described in comment #5 will give me command-oriented API at the workbench level and allow me to get there later if not initially. I prefer approach (b) but may back off to (a) if I get into serious trouble somehow. Dani, two questions for you: - are there client issues on your side with moving the command to org.eclipse.ui? I can always commit the new one first. Note that I've renamed it to org.eclipse.ui.edit.contentAssist.proposals instead of org.eclipse.ui.edit.text.contentAssist.proposals. - do you feel that the workbench content proposal adapter should just know what command to use or do you think it should be supplied by the client? Renaming isn't an option. The command ID is API as well and clients can get the command via that string. And I agree with Doug, we should not define a new command but keep the original one with its ID and move it down, otherwise there will be two "Content Assist" commands which will be confusing for users (see bug 67099 for such an example and with agreement to keep the IDs from Platform Text and JDT Text). Since we should use the same key sequence for all content assists we should also move down the key binding definitions. And for consistency I'd also want to move the "org.eclipse.ui.edit.text.contentAssist.contextInformation" command with its key binding definitions. >do you feel that the workbench content proposal adapter should just know what >command to use or do you think it should be supplied by the client? It should use the "org.eclipse.ui.edit.text.contentAssist.proposals" command per default and offer an option to override this (advanced usage). (In reply to comment #8) > And for consistency I'd also want to move the > "org.eclipse.ui.edit.text.contentAssist.contextInformation" command with its > key binding definitions. +1 I've released the support for the WorkbenchContentAssistAdapter. Available >20060127. It uses approach (b) and it turned out rather clean. The handlers are activated when the control gets focus and deactivated when it loses it. Dani/Markus - we have to coordinate the release of the command and key definitions because they will not work if they are defined in both places. I'm going to close this bug and we can take up the coordination of removing/adding the content assist commands and keys in bug #120237 as the relevant patches are there. verified in 20060214-0800 on Win XP - Content Assist and Context Information commands are now defined in org.eclipse.ui instead of org.eclipse.ui.editors - Content Assist command key binding works on patched find/replace dialog - JFace-level specification of keystroke is working in the field assist example |