Bug 899434 Menuitem JEP

[erikvold]

No description provided.

[Gozala]

Posting my comment from bugzilla:

I think I would suggest to rephrase as follows:

Array of group constants that menu may be placed in. Note that menu item will appear only under one, most appropriate parent from the given options. Add-on's provides is given from left to right. This enables add-ons to handle different platforms in order of their preference (note not all groups will be available
on all platforms).

I don't like that this property is string or number or array of them. I think it
should always be an array and instead of string or numbers we should expose constants with readable names representing different parent groups. This would
also align with identical approach in places API. This is also going to allow us
to start with just a few well thought out groups and later we may add more.

[Gozala]

@erikvold Note sure if it slipped trough or if you disagree with my comment, but I'd like to know your opinion on this.

To elaborate about why I would like to always have array is to encourage users putting multiple items in it

[Gozala]

If we'll agree that context proposed in #1214 is a good way to go, I think it would make a lot more sense to convert this to context as well. Not saying we have to, or that we should wait on resolution on the context subject, it's just be prepared that we may have to adjust this later, unless you disagree in regards that context here would make sense.

[erikvold]

Yeah I didn't like group, but I do like context so I'll make that change.

[Gozala]

One general comment, in Navbar Buttons we do have notion of chained states. Here we don't seem to have that, maybe it does not makes sense or maybe it does make sense to have state differences across windows ?

For example following a location bar button example if you are on a new tab page you don't get bookmark button because it's redundant (I guess it's on UX build because I see it on nightly), but also "Bookmarks > subscribe to this page" is disabled if I'm on tab without RSS feed, making me think that contextual state does in fact makes sense.

[erikvold]

making me think that contextual state does in fact makes sense.

If anyone asks for it then we can still add it, for now I'd like to avoid doing so.

[erikvold]

making me think that contextual state does in fact makes sense.

If anyone asks for it then we can still add it, for now I'd like to avoid doing so.

The reason being that I don't like how this was implemented in buttons, and you haven't weighed in on my proposed alternative in the mailing list yet, and ZER0 seems to be against it or doesn't understand it.

[Gozala]

We should expose constants as module exports to be consistent with places API.

[Gozala]

Also note that in the docs you call this context and here you call it menu.

[ZER0]

@Gozala, could you point to an example about what you mean mentioning the Places API? From what you said it sounds like Place API are not consistent with all the rest of high level API we have in the SDK, but maybe I misunderstood.

[erikvold]

@Gozala ping, I have the same question as @ZER0

[Gozala]

@Gozala, could you point to an example about what you mean mentioning the Places API? From what you said it sounds like Place API are not consistent with all the rest of high level API we have in the SDK, but maybe I misunderstood.

Both places API:

https://github.com/mozilla/addon-sdk/blob/master/doc/module-source/sdk/places/bookmarks.md#searching-for-bookmarks

And indexed-db API:
https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/sdk/indexed-db.html

Export globals like this as regular exports. @ZER0 do you imply that some APIs export them as properties of
the constructor ? I don't seem to recall or see it to be a case.

[Gozala]

making me think that contextual state does in fact makes sense.

If anyone asks for it then we can still add it, for now I'd like to avoid doing so.

That works for me, as far as I can see it will just work as navbar buttons global state, to which we will be able to add per window and per tab state support in next iterations.

[Gozala]

    making me think that contextual state does in fact makes sense.

If anyone asks for it then we can still add it, for now I'd like to avoid doing so.

The reason being that I don't like how this was implemented in buttons, and you haven't weighed in on my proposed alternative in the mailing list yet, and ZER0 seems to be against it or doesn't understand it.

@erikvold I have not seen that alternative API I'm afraid. Again I think our JEP feedback loop was very broken. Let's don't go over it again, but now I think we have a process that hopefully will work, so let's use it! Please comment on
the my pulls that I have made based of our discussion other day.

To be honest I was pushing @ZER0 towards more FRP style API but was told that everyone hated it & current API seemed like a good compromise.

Now all this aside, we do need to have a common API for dealing with this, if @erikvold you see issues with a current API please outline them so we can address (and it's almost if not already too late). Once we have settle
on the API we have to stick to it across different APIs we build regardless of our taste preferences because having
different ways is worth that one imperfect way.

[erikvold]

@Gozala I'm starting to think that we should use menu instead of context since the context will always be a menu. Is there some reason we can imagine why it would not be? I think menu is better because it is easier to understand than context is.

Looking at the comments above it looks like you were not against using menu instead of context we just wanted to have uniformity for some reason, and I think that adds confusion now (thinking about this as a user).

[erikvold]

Now all this aside, we do need to have a common API for dealing with this, if @erikvold you see issues with a current API please outline them so we can address (and it's almost if not already too late). Once we have settle
on the API we have to stick to it across different APIs we build regardless of our taste preferences because having
different ways is worth that one imperfect way.

This is addressed in bug 912208

[erikvold]

To be honest I was pushing @ZER0 towards more FRP style API but was told that everyone hated it & current API seemed like a good compromise.

@Gozala I think there is some miscommunication about this, I'm for FRP anyhow..

[Gozala]

Is visible really needed ? I would leave it out unless you're sure about it! We can always add it later too. Either way I think it would make sense to rename it to hidden to be more consistent with disabled.