Bugzilla – Full Text Bug Listing
| Summary: | [client] scoped keybindings | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Susan McCourt <susan> |
| Component: | Client | Assignee: | Susan McCourt <susan> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bokowski, eclipse.felipe, john.arthorne, Silenio_Quarti |
| Version: | 0.2 | ||
| Target Milestone: | 0.2 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Bug Depends on: | 334189 | ||
| Bug Blocks: | |||
|
Description
Susan McCourt
Silenio, Felipe: can we get eclipse.KeyBinding moved to its own file so that we wouldn't have to include the entire editor in the navigator page to reuse KeyBinding? Is there a reason not to do this now that we can munge it all together in the build when needed? My alternative is to keep keybinding parameters in the commands and then construct different objects depending on where the command is used. (not terrible but is it necessary) > can we get eclipse.KeyBinding moved to its own file
that means that every html using the editor would have to add:
<script language="javascript" type="text/javascript" src="../js/keybinding.js"></script>
What is it what you really want ?
a) the name "eclipse.KeyBinding"
b) the functionality that the current eclipse.KeyBinding provides (which is almost zero).
I'll note that SSQ does not like our eclipse.KeyBinding object, he particularly dislike the name ("Binding" should not be in the name, it doesn't really bind anything per se).
I was just trying to avoid duplication so that definition of key codes that trigger an action was done the same way (vs. me having a duplicate data structure and then creating your key binding when I want to set it in the editor). But given you probably want to evolve yours (at least the name) we can just have separate data structures for now. I would suggest not to reuse that object at this point (there is a chance that we will change it). Let me know when you have the keybindings story for Orion sorted out. At that point I can change the editor to be consistent with your design -or-, at least, change it so our objects do not get your way (name conflict). moving to M7. I added some bindings to various pages but have not yet come up with a general mechanism to define bindings in the command framework. For M8 we may want to define these custom bindings that are currently in glue code as commands, and have the framework hook the bindings in. The nice thing is that this would let us implement a "key assist" dialog that shows you what bindings are available. This is not critical for June release, but I'll leave it on the list for now. (In reply to comment #6) > I added some bindings to various pages but have not yet come up with a general > mechanism to define bindings in the command framework. For M8 we may want to > define these custom bindings that are currently in glue code as commands, and > have the framework hook the bindings in. > > The nice thing is that this would let us implement a "key assist" dialog that > shows you what bindings are available. This is not critical for June release, > but I'll leave it on the list for now. Boris and I just discussed that the "key assist" panel is an important consumability feature for the June release. One thing I'd like to figure out for M8 is how best to hook and handle keybindings with no ctrl or shift qualifier. For example, I've bound "T" to open resource on all pages. (This is the github binding for similar function). I hooked a keypress on the document as a whole and then had to hack a check to ignore the keypress if the widget receiving it is an input field. Surely there's a better way. (In reply to comment #0) > Boris also suggests we review at the keybinding story in dojo. There's not really a story. dojo.keys helps with key constants, but there's no story above that. There is an old bug for defining a more advanced API.http://bugs.dojotoolkit.org/ticket/2794 but this is still basically an "attach a function to a domNode and deal with browser differences" (In reply to comment #4) > I would suggest not to reuse that object at this point (there is a chance that > we will change it). > > Let me know when you have the keybindings story for Orion sorted out. At that > point I can change the editor to be consistent with your design -or-, at least, > change it so our objects do not get your way (name conflict). I ended up copying eclipse.KeyBinding and calling it eclipse.CommandKeyBinding. I wanted to use the same definition and match handling, but I needed to add the concept of a stored user string, so that we can put up the KeyAssist panel that shows the bindings. I started down the path of inferring a user string from the binding (if mod1 = true then prefix with Ctrl+, etc.) but that doesn't work for non-character keys. So the client has to specify the user string, such as new eclipse.CommandKeyBinding("Ctrl+S", 's', true)... The class is defined in commands.js. We may want to evolve this so that the "Ctrl+S" string could actually be parsed to create the binding but that is not critical right now. (In reply to comment #8) > One thing I'd like to figure out for M8 is how best to hook and handle > keybindings with no ctrl or shift qualifier. For example, I've bound "T" to > open resource on all pages. (This is the github binding for similar function). > > I hooked a keypress on the document as a whole and then had to hack a check to > ignore the keypress if the widget receiving it is an input field. Surely > there's a better way. I couldn't find a better way to handle this generically. I found some open source key binding stuff that plays the same/similar tricks. I committed some initial support but at the moment only the navigator is using it. What we have: - the bindings now go through the command service rather than hard coded - the command service keeps the bindings up to date with respect to selection, handlers, etc...so that any time we refresh the commands in the UI, the bindings are pointing at the latest command data - the '?' binding brings up a (plain) key assist panel under the Orion logo, you can use Esc to get rid of it Still to do: - come up with a strategy so that editor bindings can be made known to the command service for the purposes of putting up the key assist panel. So that whenever our code registers an editor action, we also basically say, "I'm using Ctrl-K for "find again" in this widget. - see if it makes sense for any of the application editor bindings to be active when the editor does not focus. If so, we create commands for these and bind the same key in the command service - right now we have different bindings inside (Ctrl+Shift+R) and outside (T) the editor for open resource. Sort this out. - add keybindings to commands in other pages and remove any hard-coded key events - pretty up the keybinding panel Implementation notes: - I originally thought that we'd scope the keybindings to the dom node at which the command was scoped. For example, hook nav keybindings only on the navigator div. This sounds good in practice but turned out to be problematic because not all of our command targets have focusable nodes, and the user may not have focus on that area at all. So for now, I'm hooking all keybindings from the command framework on the document. To have truly scoped bindings, we'd have to be more diligent in ensuring there is a focus node and setting the focus on page load. - The key event hooking is very naive compared to the editor's checks for all the browser bugs and differences. The basic types of keystrokes (unmodified, ctrl, shift, etc.) seem to be working but we'll see as we go... another issue to look at: there is a variant of bug 345292 that is back when a dialog is opened via a keybinding on FF4. The focusnode at the time the binding is triggered affects whether this problem appears. (In reply to comment #9) > Still to do: > - come up with a strategy so that editor bindings can be made known to the > command service for the purposes of putting up the key assist panel. So that > whenever our code registers an editor action, we also basically say, "I'm using > Ctrl-K for "find again" in this widget. Done. I went down a path of telling the command service about bindings it doesn't manage, and this just felt completely bogus. The command service shouldn't be the one assembling this info, and it meant that we had to basically repeat all the keybinding information that the various editor binding factories already define. Instead, I decided to programmatically traverse the editor actions when the key assist panel is opened. This also meant going back to the idea of programatically generating the user string from a keybinding. I wrote a utility method for this. It also meant renaming the editor bindings to more user friendly names. This also allowed me to remove the extra functionality from eclipse.CommandKeyBinding, and means that if eclipse.KeyBinding were moved to its own file, I could get rid of my copy of it. > - see if it makes sense for any of the application editor bindings to be active > when the editor does not focus. If so, we create commands for these and bind > the same key in the command service None stood out. We can open bugs if we find some. > - right now we have different bindings inside (Ctrl+Shift+R) and outside (T) > the editor for open resource. Sort this out. Leaving as is until someone opens a bug otherwise suggesting what we should do. We could change it to Ctrl+Shift+T. At least the key assist panel shows both now. > - add keybindings to commands in other pages and remove any hard-coded key > events this is covered in bug 347248 > - pretty up the keybinding panel slightly done, it could be way nicer but we can open bugs with ideas or mockups. (In reply to comment #10) > another issue to look at: > there is a variant of bug 345292 that is back when a dialog is opened via a > keybinding on FF4. The focusnode at the time the binding is triggered affects > whether this problem appears. opened bug 347876 for this problem. opened bug 347875 for a problem whereby the editor won't let me propagate the ESC key outside of the editor so that the key assist panel can close. Closing this bug as the basic functionality is complete. |