Bugzilla – Full Text Bug Listing
| Summary: | Should action IDs be externalized strings, or something else | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Mark Macdonald <mamacdon> |
| Component: | Editor | Assignee: | Silenio Quarti <Silenio_Quarti> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | grant_gayed, Silenio_Quarti, susan |
| Version: | 0.5 | ||
| Target Milestone: | 0.5 RC1 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
Why do you externalize ids? They should be just marked as NON-NLS and perhaps refactored out to a constants file. I think we need to decide if the editor would ever store a user facing name when an action is defined. - as implemented today, the action "name" is just an id. And from a strictly textView point of view, it shouldn't be externalized. - but...today we show that action name in the key assist panel, so it does show up. One could argue this is a bug in the client UI, that we should keep a separate table of id->name mappings. But we've also talked about the editor having its own "popup command panel" so that you could use the bindings on a tablet. So in that case, the editor would have to know the user names (bug 354698). Silenio, what do you think? If the textView is going to have some kind of "execute command" mode for tablet users, do you see that the API would have an action id and a string? Or... Maybe there is an API call for the client to pass some object to the editor that it could use to lookup a string name if needed...rather than have the editor store them. I pushed a new branch with changes to fix this problem [1]. I went with option 1) the text view stores the localized names of the actions. TextView.setAction() kept the same number of args, but "name" was renamed to actionID to make it clear it is an ID and "handler" can be either a function (for backwards compatibility) or an Action object. Action objects have "name" and "handler" for now. There is still some cleanup to be done, but please take a look. The one concern I have is that the Action object will grow into a parallel implementation of Commands. For example, it should probably have an image as well. [1] http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/log/?h=bug377828 Looks good to me. It also fixes that bug where 'Go To Line' appeared in the key assist panel as 'gotoLine'. (In reply to comment #4) > There is still some cleanup to be done, but please take a look. The one concern > I have is that the Action object will grow into a parallel implementation of > Commands. For example, it should probably have an image as well. hmmm.... The first part (changing to actionID and the hash lookup) looks good. I'm skeptical about the second part for the same reason you are. It feels like we are growing a new "Command". Not only is it a parallel implementation, but clients that traffic in commands would have to build a separate object for the editor. And in particular, the use of the word "handler" is very different than how the command framework uses that word, which is who the callback function is bound to. I'd like to see a solution that would allow high level clients who use commands to reuse the command info, and editor clients to use something simpler. What if we use a third (optional) actionDescription parameter that includes descriptive information about the action. Such as.... setAction(actionId, handler, actionDescription) actionDescription minimally has a "name" property, and it can grow to include image, tooltip, all that other stuff. A command client would simply pass the command info. So if I already have a command object for something (because I support the binding for it outside of the editor), I could do... setAction(myCommand.id, myCommand.callback, myCommand) and then we just make sure that the properties for your actionDescription have the same name as the command properties. Direct clients of editor can use setAction(actionId, function() {do my thing}, {name: "My Action}); It also might be worth looking at the gcli command implementation. If we have command line contributions that we want to bind in the editor, then ideally we could supply the gcli command instead, like setAction("myGCLICommandID", command.exec, command) GCLI commands also have a "name" so this would work. I opened bug 378420 to talk about the general idea of having gcli and Orion commands be more alike. Thanks Susan. I changed TextView.setAction() to take the third argument. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=b8209309762eb9fb2b22a153ad8338af35e512ac |
Now that the editor has been NLS'd, editorFeatures.js is full of code like this: > this.textView.setKeyBinding(new mKeyBinding.KeyBinding(191, true), messages.toggleLineComment); > this.textView.setAction(messages.toggleLineComment, function() { > ... > }); Here we're using an externalized string (whatever messages.toggleLineComment happens to be) to serve as the identifier for an action. This seems wrong... Don't we want a way to identify actions independently of the current locale? At some point we will probably need non-externalized action IDs. For example, if a plugin wants to provide a custom implementation of the 'toggle line comment' action, it shouldn't have to be NLS-aware. And if we're going to have those non-externalized IDs anyway, why not use them internally in the editor as well? Another problem with our current approach is that it couples a TextView instance to the message bundle that it was created with. To change the locale of the current page, you can't just swap in another message bundle -- you need to remove and re-add all the editor actions. This may not be a huge problem in practice (you'll probably have to reload the whole page anyway), but it seems like an unnecessary coupling.