Community
Participate
Working Groups
The way the command service stores information about commands no longer maps well to the most common use cases. Everything used to be stored by scope for quick retrieval. Then along came keybindings and url bindings, where you can contribute the command without necessarily rendering it in a dom element. So we grew another list of active bindings. Then along comes the need for third parties to find commands in the command service (related task extension, openwith extension in fileCommands). Now we have outside parties reaching into the internal data stuctures. We need to revisit the implementation and ensure that all the "reachy" clients are using API, that the API really works for all scopes, and that when we are rendering we aren't traversing too many lists of commands. I thought I'd get to this during 0.4 but I don't think it's going to happen, so opening this bug for just after.
see bug 370340. "visibleWhen" isn't a great name when talking about something that renders only as a keybinding or a URL binding.
Having implemented bug 360986, some thoughts: 1. the main problem we have today is that contributions are stored by command id key and this doesn't distinguish contributions of the same command in different dom nodes. The variables at play for contributions are: - paths/groups - dom nodes as the root of most paths - "rootless" paths when object scoped commands are at play since we don't look at dom node. So we probably need to define a path in terms of a dom node, where "null" would mean "it doesn't matter." From that path, then we add groups. Then when we render dom or global commands, we can jump straight to the dom node root. When we render object commands we jump to the "rootless" groups. Today we have real live cases of object and dom contributions of the same command on the same page. This needs to work in cases where the groups are different and the same. 2. A nice optimization might be contributing object commands with a dom node specifying the parentage in which that object contribution is to be shown. Right now we show all object commands that validate against an item, but in a multi-pane scenario, you might only want, say, "open with" to show on certain file items but not in any pane that shows file items. 3. We need to revisit all the API concerning command rendering/contributions. The "text only" flag is no longer honored and we should make sure no one is using it. 4. For our arbitrary "style guide" rendering rules...how is that expressed in command definition? For example, "button" contributions that are spatial arrows are shown as icons, the rest is text. Right now we accomplish this by having the spatial command not supply a name (cheesy). There should be a place to register style exceptions in the framework itself.
I was thinking this weekend that if everything is defined in term of its dom contribution, then we could probably just get rid of the whole idea of command "scope" and you would contribute a command to a dom node. If that node were null, it would work like an object command. Then the model is that you simply create the command in a common place, no decisions about scope have to be made. Then you contribute to as many dom nodes (or null) as you like on a page.
Another point is whether we want to have clients stop emptying command parents/toolbars and instead ask the framework to do it. If the framework did it as part of the render sequence, then we could implement things like "whenHidden" (see bug 371698). I think we have cases where clients call render on the same div for multiple target items, so it would definitely have to be a flag telling the framework if you were replacing the commands that were there.
In bug 372182, even though the command service was being called via the service registry, and a promise created for the method call, all the work is blocking/synchronous. Need to figure out what should be happening internally to prevent this from happening. For example, create the parent menu but then the population of a menu should be asynch? So it's both a tuning issue as well as making sure we aren't blocking.
(In reply to comment #5) > In bug 372182, even though the command service was being called via the service > registry, and a promise created for the method call, all the work is > blocking/synchronous. Need to figure out what should be happening internally > to prevent this from happening. For example, create the parent menu but then > the population of a menu should be asynch? This was indeed a good solution for the hack done in bug 372182. We added a menu in the client code, called the command framework asynchronously, and then bashed our fake menu with the real one. I think we need something similar inside the framework. What we do today is get paranoid about adding to the dom if it's not needed. The thought was "performance" but it turns out not to be helpful. Today when we build a menu, we do it all synchronously so that if the commands don't validate and there are no valid menu items, we never insert anything into the dom. Instead, we should probably just stick a dropdown in the DOM, asynchronously render the child menu items, and if truly the menu is empty at the end, remove it. I bet it would rarely happen in practice. Likewise, the "get rid of separators that weren't necessary etc." code can happen at the end. This could result in flash for corner cases but I think will greatly help the typical case. So...the todo's for this bug are: - reorganize the commands so we don't need scopes anymore/clean up API - have contributions made by DOM node - get rid of extra crap in API we don't need anymore - investigate asynch population of menus
(In reply to comment #5) > In bug 372182, even though the command service was being called via the service > registry, and a promise created for the method call, all the work is > blocking/synchronous. Need to figure out what should be happening internally > to prevent this from happening. As far as the whole issue that even when we use the service registry/promise the command framework rendering was blocking, Simon says: >in a nutshell it may be async but a promise will not do the equivalent of a settimeout if it can do the work immediately >to be honest setTimeout is right here >we honest to god want to schedule work here >we're doing too much work on the main thread before returning to the user >in this case we're doing too much of the action rendering work (that the user can't even see) before rendering the stuff he/she can
Fixed two major areas. 1) changed the API for scopes so that all contributions are based on a contribution id, and that id no longer HAS to be tied to the dom id of the element where the commands will be rendered. This greatly simplifies the internals and hte API. Changes to the API: * addCommand no longer requires a scope. This means commands are only ever added once and the creator of the command doesn't have to guess about its scope anymore. * addCommandGroup and registerCommandContribution now take a "scopeId" as their first parameter. This id can be a DOM id, but doesn't have to be. It's simply the id that will be supplied when rendering the command. For what used to be "dom" commands, this is typically the dom id being rendered into. For what used to be "object" commands, any id representing object contributions will be used and this will be used in the renderCommands call. In other words, the contribution is no longer tied to the rendering parent. * renderCommand now takes a required scopeId as its first parameter and no longer takes a scope parameter like "dom" or "object" These changes mean you can more precisely control which object commands will appear in a particular list. Performance can be improved by specifying different scope id's for different kinds of objects on a page and rendering them uniquely. But for now, where lots of pages are sharing commands (like git) I used a scope id like "itemLevelCommands" which effectively uses the old "object" behavior of looking at all object-based commands when rendering, and then ruling out many of them based on the active item. Internally, we now look up the contributions by scope id and only ever validate those. Much simpler than before when we had all contributions stored together. An interesting side note is that cleaning all this up did not change the performance in any noticeable way. Hence...the second thing: 2) The calls that render commands grouped by menu are now made asynchronously. This is basically a generalization of the fix in bug 372182. I removed that workaround from the navigator. Other API cleanup: 3) removed all the old, unused parameters (textOnly, inactiveCommandClass, etc.) 4) rearranged parameters so that the most commonly used ones are first. This way you don't see a lot of registerCommandContribution("foo," 1, null, null, "baz") instead it's registerCommandContribution("baz", "foo", 1) (In reply to comment #4) > Another point is whether we want to have clients stop emptying command > parents/toolbars and instead ask the framework to do it. If the framework did > it as part of the render sequence, then we could implement things like > "whenHidden" (see bug 371698). Since we are actually separating the rendering scope id from the dom node, doing magic emptying doesn't really make sense. It will be a common pattern to render multiple combinations of scopeId and items into the same parent div. So I think leaving it up to the client is the right thing to do.
