JEP 112 - Context Menu
Myk Melez
Here's where I think we should go with the Context Menu JEP:
- For consistency with other APIs, have the module export Item,
Menu, and Separator constructor functions for creating items, menus,
and separators instead of returning a singleton with methods for doing
so.
The constructors should accept an options parameter containing a property collection of the appropriate properties (which are mostly as currently described in the JEP, except that menus don't have data or command, since they can't be selected; and separators have no properties).
And the insertBefore and replace methods should be represented as (mutually exclusive) properties of the objects.
- For consistency with other APIs, call the command
callback property onCommand.
- Call the menu property of Menu objects items,
which is more descriptive of the values it contains.
Note: although properties that can take either a single value or multiple values (whose order is not significant) should have singular names, this particular property explicitly accepts an array representing the ordered set of items in the menu, so it should not accept single values (only an array of values), and plurality is appropriate.
- Call the insertBefore property precede, which
is a simpler synonym for that phrase.
- When parsing contexts, don't worry about forbidding the CSS
universal selector, given that extensions can just as easily supply a
function context that always returns true.
- When specifying existing items to replace or precede, don't match
against labels, since such matches break on localized versions of
Firefox.
- When specifying existing items to replace or precede, don't
provide regular expression or case-insensitive matches, as their
utility is unclear, given that the set of possible items that can be
referenced is well-constrained.
We should, however, make sure to document those values as part of the documentation of the API, so extension developers don't have to introspect Firefox's DOM or read through its code to discover them.
- Give instances a destroy method, so extensions can also
remove them from the context menu.
- Implement the precede and replace properties
in a second phase of development, providing only the ability to add
items and menus to a Jetpack-dedicated separator-delimited section of
the context menu in the initial implementation.
The reasoning here is that use cases in the JEP all describe adding items (although there is a good use case for replacement in one of the examples), and over in the One UI Element JEP we're constraining placement of widgets using that high-level API to a specific area of the browser window, so we need to think carefully about whether we want to provide dissimilar functionality in this similarly high-level API.
Note: this does not mean I think we shouldn't ever have this functionality, only that its ideal placement requires more thought. It may be that it makes sense to include it in this high-level API, but perhaps it makes more sense for it to be a separate lower-level API, just as the One UI Element API is unlikely to provide functionality for placing a widget anywhere in the Firefox chrome, although such functionality could be provided by another, lower-level API.
-myk
Drew
https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/112
> 1. For consistency with other APIs, have the module export Item,
> Menu, and Separator constructor functions for creating items,
> menus, and separators instead of returning a singleton with
> methods for doing so.
What happens when I do:
var item = new contextMenu.Item(...);
contextMenu.add(item);
// Some time later...
item.label = "And Now for Something Completely Different";
I didn't want to deal with changing menuitem properties after they've
been added. Of course whether the item was instantiated from an Item
constructor or object literal doesn't technically matter, but I thought
if there is a constructor, the likelihood that people would expect
dynamic changes to apply is greater.
> And the insertBefore and replace methods should be represented as
> (mutually exclusive) properties of the objects.
What's the reason for making properties out of menu-modifying verbs that
are on par with "add"?
> 3. Call the menu property of Menu objects items, which is more
> descriptive of the values it contains.
Since you've suggested the value of this property is a Menu instance
(right?), "menu" seems more appropriate. If the value were an array
like the initial proposal, I'd agree.
> 4. Call the insertBefore property precede, which is a simpler synonym
> for that phrase.
OK, if "precede" is a property rather than a method on the module.
> 5. When parsing contexts, don't worry about forbidding the CSS
> universal selector, given that extensions can just as easily
> supply a function context that always returns true.
I thought a lot about this. I fear that if we allowed it, nearly every
bit of random example code on the Web would use it, and people would
copy and paste. It's too easy to use and too easy to ignore its
purpose, breaking the whole point of contexts.
You're right that people can use a function that returns true, but it's
not nearly as terse as "*" or as easy to ignore. I think most proper
selectors will be less typing than such a function, and once you've
written that function it'll be fairly natural to fill it in properly so
that it doesn't always return true.
> 6. When specifying existing items to replace or precede, don't match
> against labels, since such matches break on localized versions of
> Firefox.
Good point! This was always a concern with the prototype, but I kind of
just ignored it, meaning to make a giant mapping of Jetpack-friendly IDs
to XUL IDs. But since the scope this time is only the context menu, the
set of IDs and the potential mapping are small.
> 8. Give instances a destroy method, so extensions can also remove
> them from the context menu.
One thing I like about the proposal is that modifications are bound to
contexts. You don't have to manually add and remove items depending on
whether your context arises, unlike the prototype. Do you think
extensions will commonly want to forever remove items they've added?
> 9. Implement the precede and replace properties in a second phase of
> development, providing only the ability to add items and menus to
Sounds good.
> a Jetpack-dedicated separator-delimited section of the context
> menu in the initial implementation.
I like this but also share your potential concern: What's the point
when it'll be easy to use replace or precede to break out of the Jetpack
section?
Drew
On 2/20/10 12:44 PM, Myk Melez wrote:
> Hi all,
>
> Here's where I think we should go with the Context Menu JEP:
>
> 1. For consistency with other APIs, have the module export Item,
> Menu, and Separator constructor functions for creating items,
> menus, and separators instead of returning a singleton with
> methods for doing so.
>
> The constructors should accept an options parameter containing a
> property collection of the appropriate properties (which are
> mostly as currently described in the JEP, except that menus don't
> have data or command, since they can't be selected; and separators
> have no properties).
>
> And the insertBefore and replace methods should be represented as
> (mutually exclusive) properties of the objects.
>
> 2. For consistency with other APIs, call the command callback
> property onCommand.
>
> 3. Call the menu property of Menu objects items, which is more
> descriptive of the values it contains.
>
> Note: although properties that can take either a single value or
> multiple values (whose order is not significant) should have
> singular names, this particular property explicitly accepts an
> array representing the ordered set of items in the menu, so it
> should not accept single values (only an array of values), and
> plurality is appropriate.
>
> 4. Call the insertBefore property precede, which is a simpler synonym
> for that phrase.
>
> 5. When parsing contexts, don't worry about forbidding the CSS
> universal selector, given that extensions can just as easily
> supply a function context that always returns true.
>
> 6. When specifying existing items to replace or precede, don't match
> against labels, since such matches break on localized versions of
> Firefox.
>
> 7. When specifying existing items to replace or precede, don't
> provide regular expression or case-insensitive matches, as their
> utility is unclear, given that the set of possible items that can
> be referenced is well-constrained.
>
> We should, however, make sure to document those values as part of
> the documentation of the API, so extension developers don't have
> to introspect Firefox's DOM or read through its code to discover them.
>
> 8. Give instances a destroy method, so extensions can also remove
> them from the context menu.
>
> 9. Implement the precede and replace properties in a second phase of
> development, providing only the ability to add items and menus to
> a Jetpack-dedicated separator-delimited section of the context
> menu in the initial implementation.
>
> The reasoning here is that use cases in the JEP all describe
> adding items (although there is a good use case for replacement in
> one of the examples), and over in the One UI Element JEP we're
> constraining placement of widgets using that high-level API to a
> specific area of the browser window, so we need to think carefully
> about whether we want to provide dissimilar functionality in this
> similarly high-level API.
>
> Note: this does not mean I think we shouldn't ever have this
> functionality, only that its ideal placement requires more
> thought. It may be that it makes sense to include it in this
> high-level API, but perhaps it makes more sense for it to be a
> separate lower-level API, just as the One UI Element API is
> unlikely to provide functionality for placing a widget anywhere in
> the Firefox chrome, although such functionality could be provided
> by another, lower-level API.
>
> Thoughts?
>
> -myk
>
> --
> You received this message because you are subscribed to the Google
> Groups "mozilla-labs-jetpack" group.
> To post to this group, send email to mozilla-la...@googlegroups.com.
> To unsubscribe from this group, send email to
> mozilla-labs-jet...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/mozilla-labs-jetpack?hl=en.
Myk Melez
> I didn't want to deal with changing menuitem properties after they've
> been added. Of course whether the item was instantiated from an Item
> constructor or object literal doesn't technically matter, but I
> thought if there is a constructor, the likelihood that people would
> expect dynamic changes to apply is greater.
and error messages. After all, these won't be the only such properties
on the objects in our API, and it will always be necessary for us to
document and enforce (and for developers to learn) which properties are
always modifiable, settable only upon construction, or read-only.
> > And the insertBefore and replace methods should be represented as
> > (mutually exclusive) properties of the objects.
>
> What's the reason for making properties out of menu-modifying verbs
> that are on par with "add"?
Note: my recommendation is actually that we not have an "add" function
but rather add an item to the context menu upon instantiation, just as
for panels and widgets:
let Item = require("context-menu").Item;
let item = Item({ label: "Hello, world!" });
// There is now a "Hello, world!" item in the context menu.
However, if we did have an add method, I'd still recommend that the
insertBefore and replace methods be represented as properties, since
they do the same thing as the add method (add an item to the menu),
except that they specify position (which seems more an attribute of an
item than a different behavior).
> > 3. Call the menu property of Menu objects items, which is more
> > descriptive of the values it contains.
>
> Since you've suggested the value of this property is a Menu instance
> (right?), "menu" seems more appropriate. If the value were an array
> like the initial proposal, I'd agree.
Actually I suggest that the object with the property be a Menu instance,
and the value of the property continue to be an array of items, i.e.:
let { Menu: Menu, Item: Item } = require("context-menu");
let menu = Menu({
label: "Metasyntactic Variables",
items: [
Item({ label: "Foo" }),
Item({ label: "Bar" }),
Item({ label: "Baz" })
]
});
> You're right that people can use a function that returns true, but
> it's not nearly as terse as "*" or as easy to ignore. I think most
> proper selectors will be less typing than such a function, and once
> you've written that function it'll be fairly natural to fill it in
> properly so that it doesn't always return true.
Isn't that also true of the universal selector (i.e. that developers who
start with "*" while prototyping an extension will refine it over time
to find just those elements they really want)? It's not a big deal
either way, it's just not clear how useful this restriction will
actually be.
> Good point! This was always a concern with the prototype, but I kind
> of just ignored it, meaning to make a giant mapping of
> Jetpack-friendly IDs to XUL IDs. But since the scope this time is
> only the context menu, the set of IDs and the potential mapping are
> small.
Yes, and that's a great point about mapping a friendlier (simpler, more
consistent, etc.) ID set to XUL IDs. We should definitely do that!
> One thing I like about the proposal is that modifications are bound to
> contexts. You don't have to manually add and remove items depending
> on whether your context arises, unlike the prototype. Do you think
> extensions will commonly want to forever remove items they've added?
No, I don't, but I do think it should be possible and work consistently
across APIs, which is why I've recommended such methods on the objects
created by other APIs as well. FWIW, the implementation here should be
the same as the one Jetpack itself uses to remove items when the
extension is disabled.
> > a Jetpack-dedicated separator-delimited section of the context
> > menu in the initial implementation.
>
> I like this but also share your potential concern: What's the point
> when it'll be easy to use replace or precede to break out of the
> Jetpack section?
For me it's actually not clear how easy it'll be to use replace/precede
to break out of that section. The purpose of my recommendation that we
shift replace/precede to a future phase is to enable us to gather
feedback on use cases and tradeoffs, so we can make a more informed
decision on whether and how to provide these capabilities.
And even if it does become easy at some point, that doesn't necessarily
mean many extensions will take advantage of it. If we find that users
prefer Jetpack items to be grouped together, extensions may well largely
respect those preferences, even when they don't have to.
-myk
Drew
> and error messages. After all, these won't be the only such properties
> on the objects in our API, and it will always be necessary for us to
> document and enforce (and for developers to learn) which properties are
> always modifiable, settable only upon construction, or read-only.
Good point.
> let Item = require("context-menu").Item;
> let item = Item({ label: "Hello, world!" });
> // There is now a "Hello, world!" item in the context menu.
I don't think constructors should have side effects like that. When you
make a new panel, it's not automatically shown, is it? And what can you
do with |item|? I'd rather have an "add" or "set" or "register" method
on the module that you'd pass |item| to.
> Actually I suggest that the object with the property be a Menu instance,
> and the value of the property continue to be an array of items, i.e.:
OK.
> Isn't that also true of the universal selector (i.e. that developers who
> start with "*" while prototyping an extension will refine it over time
> to find just those elements they really want)?
Some will, but many won't. People often -- and understandably -- do the
simplest thing that works, even if they don't understand why.
When I gave some example code in the prototype's menu documentation, I
declared some functions with expression closures, like:
function () ({ label: "foo", ... })
In retrospect that was dumb. A lot of the menu code I saw in the
Gallery and elsewhere used that syntax. I doubt it's that common, and I
suspect people used it only because the examples used it, and then
people who found their code used it.
> No, I don't, but I do think it should be possible and work consistently
> across APIs, which is why I've recommended such methods on the objects
> created by other APIs as well. FWIW, the implementation here should be
OK.
> to break out of that section. The purpose of my recommendation that we
> shift replace/precede to a future phase is to enable us to gather
> feedback on use cases and tradeoffs, so we can make a more informed
> decision on whether and how to provide these capabilities.
Sounds good, I'll defer my comments until then.
Thanks Myk!
Drew
Myk Melez
> let Item = require("context-menu").Item;
> let item = Item({ label: "Hello, world!" });
> // There is now a "Hello, world!" item in the context menu.
I don't think constructors should have side effects like that. When you make a new panel, it's not automatically shown, is it?
I'd rather have an "add" or "set" or "register" method on the module that you'd pass |item| to.
Context menu items and persistent widgets in the single UI element, on the other hand, are intended to be permanent, such that once an extension creates one, it remains part of the UI until Firefox quits, the extension is disabled/uninstalled, or the extension removes it (f.e. in response to a user preference for which of multiple integration points should expose the extension's functionality). And persistent widgets, as currently specified in the Single UI Element JEP, are automatically shown.
The issue of the side effect seems more an implementation detail than an API consideration. From an implementation perspective, we might see the creation and DOM insertion of the XUL menuitem element as a side effect (which the implementation has to track so it can undo it when appropriate). But from the perspective of the extension developer, it's actually the main effect of the API call.
Another way to think about it is to ask whether there's a use case for extensions to create an item that doesn't get displayed. There is such a case for panels and transient widgets, which is why those objects are hidden by default and have show/hide methods. But there isn't for persistent widgets, which is why those are shown by default.
And it doesn't seem like there's such a case for context menu items either (except when their context filter doesn't match the context menu target, but that's handled automatically via a separate mechanism).
Having said all that, the one aspect of this design I worry about is whether it will confuse developers into thinking that the item will disappear if the variable referencing it falls out of scope:
let Item = require("context-menu").Item;
function makeMenuItem() {
let item = Item({ label: "Hello, world!" });
}
makeMenuItem();
// item is no longer defined; is there still a context menu item or not?
// And if so, then how do I access it?
If so, then it may indeed make sense to have add/remove methods here (and in the Singel UI Element API) in order to make this clear, even though it makes the API more complex:
let cm = require("context-menu");
function makeMenuItem() {
let item = cm.add(cm.Item({ label: "Hello, world!" }));
}
makeMenuItem();
// item is no longer defined, but cMenu obviously holds a reference to it.
And what can you do with |item|?
When a user unchecks the "show in context menu" checkbox, the extension should be able to remove the item from the context menu immediately.
In the future, if we decide that there are valid use cases for changing a context menu item after having created it, we might also enable extensions to change it by setting properties on the instance.
-myk
Drew
user of the API or any API in general, I know I would be surprised if I
instantiated an object and caused a side effect like that. If I'm
skimming someone's code and see sandwiched in between unrelated lines
new Item({ label: "foo" });
it's not at all clear that the item has been added to the page's context
menu. I'd wonder why the value is thrown away and what it has to do
with the lines before and after it.
I'm not against an Item constructor, just the side effect. If you'd
like constructors for consistency with the other APIs and potentially
for instance modifications, I dig it, but IMO it's the side effect that
would actually make the API more complex, less typing notwithstanding.
> You can destroy it. A use case for this is an extension that provides
> its functionality via several integration points (f.e. a context menu
> item, a widget, and a keyboard shortcut) and has settings that let users
> disable those selectively.
The proper way to do that would be to set a flag and check it in the
context predicate. I worry that if there are both add and destroy
methods, people will rightly be confused over whether they should use
context predicates or repeated add/destroys.
Drew
Myk Melez
When I wrote the proposal I wasn't thinking about implementation. As a user of the API or any API in general, I know I would be surprised if I instantiated an object and caused a side effect like that. If I'm skimming someone's code and see sandwiched in between unrelated lines
new Item({ label: "foo" });
it's not at all clear that the item has been added to the page's context menu. I'd wonder why the value is thrown away and what it has to do with the lines before and after it.
I'm not against an Item constructor, just the side effect. If you'd like constructors for consistency with the other APIs and potentially for instance modifications, I dig it, but IMO it's the side effect that would actually make the API more complex, less typing notwithstanding.
let cm = require("context-menu");
cm.add(cm.Item({...}));
The same is true for the Single UI Element API.
> You can destroy it. A use case for this is an extension that provides
> its functionality via several integration points (f.e. a context menu
> item, a widget, and a keyboard shortcut) and has settings that let users
> disable those selectively.
The proper way to do that would be to set a flag and check it in the context predicate. I worry that if there are both add and destroy methods, people will rightly be confused over whether they should use context predicates or repeated add/destroys.
In particular, I don't want developers to get into the habit of holding onto objects that are no longer necessary, wasting memory and CPU cycles in the process.
And I don't think there's actually that much potential for confusion, given that the API doesn't provide a way other than context predicates to detect when a context menu is about to open, which would be a prerequisite to using add/destroy methods instead of context predicates to selectively display items.
However, given that one adds an item to the menu via an add method on the exported singleton, the best way to enable removal of that item would be with an equivalent remove method on that singleton rather than a destroy method on the item itself (as I had previously suggested).
-myk
Drew
https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/112
I didn't follow these points in your original recommendations:
> 4. Call the insertBefore property precede, which is a simpler synonym
> for that phrase.
>
> 9. Implement the precede and replace properties in a second phase of
> development, providing only the ability to add items and menus to
> a Jetpack-dedicated separator-delimited section of the context
> menu in the initial implementation.
I still think that insertBefore and replace ought to be methods, not
properties, so I've left these in the proposal for now. We'll come back
to them when we get to their implementation, if that's OK.
> 5. When parsing contexts, don't worry about forbidding the CSS
> universal selector, given that extensions can just as easily
> supply a function context that always returns true.
Our discussion about this kind of tapered off inconclusively, but I
think forbidding the universal selector is the right thing to do.
> And I don't think there's actually that much potential for confusion,
> given that the API doesn't provide a way other than context predicates
> to detect when a context menu is about to open, which would be a
> prerequisite to using add/destroy methods instead of context predicates
> to selectively display items.
I think a method couple named "add" and "remove" will discourage people
from understanding how to use the context arguments. It's easy to
imagine people calling add when something happens (e.g., a page load)
and then remove when something else happens (e.g., a page unload). But,
I do agree that if there is an add, there ought to be a remove,
regardless of what they're named or how people might misuse them.
Drew
Drew
> > 5. When parsing contexts, don't worry about forbidding the CSS
> > universal selector, given that extensions can just as easily
> > supply a function context that always returns true.
>
> Our discussion about this kind of tapered off inconclusively, but I
> think forbidding the universal selector is the right thing to do.
There are valid use cases for modifying the menu on every node in the
page: DOM inspectors, extensions that hide or modify nodes. Forbidding
the universal selector would make it awkward to write these, and making
it awkward to do reasonable things sucks.
> I do agree that if there is an add, there ought to be a remove,
> regardless of what they're named or how people might misuse them.
I should say "how people might use them in unintended ways," not "misuse
them." Uses other than the ones intended by the people who designed the
thing are often better than those intended uses!
Drew
Myk Melez
> I've updated the JEP per our discussion:
> https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/112
below is making context be a property of the item object rather than a
separate parameter of the add method.
> I still think that insertBefore and replace ought to be methods, not
> properties, so I've left these in the proposal for now. We'll come
> back to them when we get to their implementation, if that's OK.
Ok, sounds good, let's circle back around on those then.
> I think a method couple named "add" and "remove" will discourage
> people from understanding how to use the context arguments. It's easy
> to imagine people calling add when something happens (e.g., a page
> load) and then remove when something else happens (e.g., a page
> unload). But, I do agree that if there is an add, there ought to be a
> remove, regardless of what they're named or how people might misuse them.
Ok, sounds good. Let's just make sure we document the correct (and
incorrect) uses for remove in the API documentation, as you've already
done in the JEP!
-myk
Drew
> below is making context be a property of the item object rather than a
> separate parameter of the add method.
Sorry, missed that. I don't view contexts as properties of items, and
they're definitely not "options" like onCommand and data are. They're
more important. Calling add() means binding an item to a context, like
you bind a value to an identifier.
Drew
Myk Melez
> There are valid use cases for modifying the menu on every node in the
> page: DOM inspectors, extensions that hide or modify nodes.
> Forbidding the universal selector would make it awkward to write
> these, and making it awkward to do reasonable things sucks.
> > I do agree that if there is an add, there ought to be a remove,
> > regardless of what they're named or how people might misuse them.
>
> I should say "how people might use them in unintended ways," not
> "misuse them." Uses other than the ones intended by the people who
> designed the thing are often better than those intended uses!
Indeed!
-myk
Myk Melez
> Reading through it, the only change I don't see that you don't list
> below is making context be a property of the item object rather than a
> separate parameter of the add method.
Sorry, missed that. I don't view contexts as properties of items, and they're definitely not "options" like onCommand and data are. They're more important. Calling add() means binding an item to a context, like you bind a value to an identifier.
And there are a number of reasons for context to be a property:
- It makes the API simpler, as it is then never necessary to
remember the order of the parameters to the add method.
- It makes provision of the context more consistent both with the
way other information about the item is provided and with the way
information about objects is provided across the entire API set.
- A separate parameter suggests that it is possible to add an item
multiple times under different contexts, while making context a
property eliminates this source of potential confusion.
- A separate parameter makes the add and remove methods less
symmetrical, since one takes a context, while the other doesn't, and
the positions of the item parameters in their signatures are different.
- If we ever decide it should be possible to modify the context
of an item, a property makes that interface simpler (it becomes a
simple property setting operation).
- If we ever decide it should be possible to both modify the context and have multiple contexts, a property makes it possible to use the same pattern we use elsewhere for the modification of properties that accept multiple values.
Also, I took another pass through the JEP and noticed an inconsistency with my recommended changes from our earlier discussion: Item shouldn't have a menu property. Instead, Menu should be a constructor with icon, label, and items properties, so consumers add a menu by passing a Menu object to the add method:
let cm = require("context-menu");
cm.add(cm.Menu({ label: "My Menu", items: { ... } }));
Finally, I thought a bit about the newish replace method, and I think we shouldn't provide it, as it's just as easy to remove the old item and add the new one, and we should bias towards fewer, composable methods.
Besides a slight reduction in codesize, the only other benefit to the replace method is that it might enable extensions to replace an item with another item at the same place in the menu.
But we don't guarantee item positioning on add, so we shouldn't do so on replace. If we really want extensions to be able to change an existing item without changing its position, as opposed to removing the item and adding a completely different item, we should do so by enabling them to modify its properties.
-myk
Drew
> properties. It configures the object, but it is not intended to connote
Whether you call them options of an object, properties of an object, or
configurations of an object, that's not the role that contexts play.
Contexts don't configure items anymore than identifiers configure the
values to which they're bound.
> 1. It makes the API simpler, as it is then never necessary to
> remember the order of the parameters to the add method.
You're trading parameter position for option name. Moreover, contexts
are required, and I think it's reasonable and customary to call out
required parameters as formal parameters.
> 2. It makes provision of the context more consistent both with the
> way other information about the item is provided and with the way
Again, contexts are not information about items.
> 3. A separate parameter suggests that it is possible to add an item
> multiple times under different contexts, while making context a
> property eliminates this source of potential confusion.
It is possible, and support of that use case is intended. However, the
JEP doesn't define (but should) whether a new context binding overrides
or coexists with the old. remove() doesn't take a context; it should if
they coexist, or if it doesn't, new should override old. Thoughts?
> 4. A separate parameter makes the add and remove methods less
> symmetrical, since one takes a context, while the other doesn't,
> and the positions of the item parameters in their signatures are
> different.
That's true, and my response to your previous point applies here as
well, but I don't think there is a reasonable expectation that add() and
remove() be symmetrical. In a dictionary interface, there's no
expectation that put() and delete() are symmetrical; add() and remove()
here are analogous.
> 5. If we ever decide it should be possible to modify the context of
> an item, a property makes that interface simpler (it becomes a
> simple property setting operation).
>
> 6. If we ever decide it should be possible to both modify the context
> and have multiple contexts, a property makes it possible to use
> the same pattern we use elsewhere for the modification of
> properties that accept multiple values.
Again, items don't have contexts.
> Also, I took another pass through the JEP and noticed an inconsistency
> with my recommended changes from our earlier discussion: Item shouldn't
> have a menu property. Instead, Menu should be a constructor with icon,
> label, and items properties, so consumers add a menu by passing a Menu
> object to the add method:
I view menus as being containers of items, not a special kind of item
that expands into a container of other items. I don't see how the
latter interpretation is not more complex than the former.
> Finally, I thought a bit about the newish replace method, and I think we
> shouldn't provide it, as it's just as easy to remove the old item and
> add the new one, and we should bias towards fewer, composable methods.
There's a misunderstanding here. replace() as defined in the JEP
replaces a built-in menuitem, not an item previously added by an
extension. remove() as defined in the JEP removes only items created
and previously added by the extension. I can't think of a use case
where people should be removing built-in items, but I can think of some
for replacing them with entirely new items.
Drew
On 3/3/10 1:08 AM, Myk Melez wrote:
> On 03/02/2010 12:44 PM, Drew wrote:
>> > Reading through it, the only change I don't see that you don't list
>> > below is making context be a property of the item object rather than a
>> > separate parameter of the add method.
>>
>> Sorry, missed that. I don't view contexts as properties of items, and
>> they're definitely not "options" like onCommand and data are. They're
>> more important. Calling add() means binding an item to a context, like
>> you bind a value to an identifier.
> The "options" parameter is just the way we initialize an object's
> properties. It configures the object, but it is not intended to connote
> that there are both "options" and "properties" or that one is less
> significant than the other.
>
> And there are a number of reasons for context to be a property:
>
> 1. It makes the API simpler, as it is then never necessary to
> remember the order of the parameters to the add method.
>
> 2. It makes provision of the context more consistent both with the
> way other information about the item is provided and with the way
> information about objects is provided across the entire API set.
>
> 3. A separate parameter suggests that it is possible to add an item
> multiple times under different contexts, while making context a
> property eliminates this source of potential confusion.
>
> 4. A separate parameter makes the add and remove methods less
> symmetrical, since one takes a context, while the other doesn't,
> and the positions of the item parameters in their signatures are
> different.
>
> 5. If we ever decide it should be possible to modify the context of
> an item, a property makes that interface simpler (it becomes a
> simple property setting operation).
>
> 6. If we ever decide it should be possible to both modify the context
> and have multiple contexts, a property makes it possible to use
> the same pattern we use elsewhere for the modification of
> properties that accept multiple values.
>
>
> Also, I took another pass through the JEP and noticed an inconsistency
> with my recommended changes from our earlier discussion: Item shouldn't
> have a menu property. Instead, Menu should be a constructor with icon,
> label, and items properties, so consumers add a menu by passing a Menu
> object to the add method:
>
> let cm = require("context-menu");
> cm.add(cm.Menu({ label: "My Menu", items: { ... } }));
>
>
>
> Finally, I thought a bit about the newish replace method, and I think we
> shouldn't provide it, as it's just as easy to remove the old item and
> add the new one, and we should bias towards fewer, composable methods.
>
> Besides a slight reduction in codesize, the only other benefit to the
> replace method is that it might enable extensions to replace an item
> with another item at the same place in the menu.
>
> But we don't guarantee item positioning on add, so we shouldn't do so on
> replace. If we really want extensions to be able to change an existing
> item without changing its position, as opposed to removing the item and
> adding a completely different item, we should do so by enabling them to
> modify its properties.
>
> -myk
>
Myk Melez
> The "options" parameter is just the way we initialize an object's
> properties. It configures the object, but it is not intended to connote
Whether you call them options of an object, properties of an object, or configurations of an object, that's not the role that contexts play. Contexts don't configure items anymore than identifiers configure the values to which they're bound.
> 1. It makes the API simpler, as it is then never necessary to
> remember the order of the parameters to the add method.
You're trading parameter position for option name.
Moreover, contexts are required, and I think it's reasonable and customary to call out required parameters as formal parameters.
> 2. It makes provision of the context more consistent both with the
> way other information about the item is provided and with the way
Again, contexts are not information about items.
However, as far as I can tell from the JEP and our discussion, contexts do not have a life of their own. One does not instantiate them independently of items, manipulate them on their own, or do anything with them other than pass them with a menu item when adding that item to the menu. And that's why they seem to me to be inherently descriptive of their items, whether we use properties or parameters to specify them.
> 3. A separate parameter suggests that it is possible to add an item
> multiple times under different contexts, while making context a
> property eliminates this source of potential confusion.
It is possible, and support of that use case is intended. However, the JEP doesn't define (but should) whether a new context binding overrides or coexists with the old. remove() doesn't take a context; it should if they coexist, or if it doesn't, new should override old. Thoughts?
let item = Item({ context: ... });
item.context = ...; // overrides previous contexts
item.context.add(...); // coexists with previous contexts
I don't think there is a reasonable expectation that add() and remove() be symmetrical. In a dictionary interface, there's no expectation that put() and delete() are symmetrical; add() and remove() here are analogous.
I view menus as being containers of items, not a special kind of item that expands into a container of other items. I don't see how the latter interpretation is not more complex than the former.
That's especially suggested by the fact that a menu has different properties than an item. Although the two share icon and label, menus don't have data or command callbacks, and they have an items property, which non-menu items don't have.
This approach reduces complexity by reducing the levels of containment. It allows us to tell developers "you can add three kinds of things to the context menu, one of which can contain more of those things" instead of "you can add two kinds of things to the context menu, one of which can contain a third thing that can contain more of those things."
There's a misunderstanding here. replace() as defined in the JEP replaces a built-in menuitem, not an item previously added by an extension. remove() as defined in the JEP removes only items created and previously added by the extension. I can't think of a use case where people should be removing built-in items, but I can think of some for replacing them with entirely new items.
-myk
Drew
reflect our consensus. There are a few particular areas where we
deferred decisions until a later, post-0.2 implementation, and they're
noted in the JEP as being still under review.
Thanks Myk!
Drew
Myk Melez
> Myk and I talked about the proposal offline, and I've updated the JEP
> to reflect our consensus.
-myk