Bug 710107 - Support radio type for simple-prefs #597

Merged
merged 7 commits into from Oct 17, 2012

Conversation

Projects
None yet
4 participants
Contributor

taku0 commented Oct 3, 2012

Added “menulist” and “radio” types to simple-prefs.

Sample:

"preferences": [
  {
    "name": "openButton",
    "title": "Mouse button to open menu",
    "type": "menulist",
    "value": 2,
    "options": [
        {
            "value": "0",
            "label": "left_button"
        },
        {
            "value": "1",
            "label": "middle_button"
        },
        {
            "value": "2",
            "label": "right_button"
        },
        {
            "value": "3",
            "label": "fourth_button"
        },
        {
            "value": "4",
            "label": "fifth_button"
        }
    ]
  },
  {
    "name": "radio",
    "title": "Label color",
    "type": "radio",
    "value": "red",
    "options": [
        {
            "value": "red",
            "label": "red"
        },
        {
            "value": "blue",
            "label": "blue"
        }
    ]
  }
]

The values of the “label” are used as l10n keys, or used vervatim as labels if no matching entries are exist in locale file.

Note that “value” of items are always strings like “on” and “off” properties for “boolint” type.

Member

Gozala commented Oct 8, 2012

@erikvold I think you'd be the perfect reviewer for this, could you please pick this one up ?

Thanks!

erikvold was assigned Oct 8, 2012

Member

Gozala commented Oct 8, 2012

@taku0 @erikvold Also I think creating a bug for this one would make a lot of sense.

Contributor

taku0 commented Oct 8, 2012

@Gozala
I found a relevant bug after submitting this pull request.
Bug 710107 - Support radio type for simple-prefs

Contributor

taku0 commented Oct 11, 2012

Updated the title.

Contributor

erikvold commented Oct 12, 2012

hey @taku0 thanks for taking this one on! I'm just starting to review this, but so far I noticed there aren't any tests, that is something that should be added.

There is a new type of test for Jetpacks called add-on tests which use a addon as a test, which would allow us to test this bug most extensively, but there are other tests that currently exist that should be extended as well. Let me know if you need/want help with this.

Contributor

erikvold commented Oct 12, 2012

I think that we should just use "options" instead of "menulist" and "radio" to specify the preference value options

Contributor

erikvold commented Oct 12, 2012

please also mention these new types in the documentation, but I won't block on this, it can be another bug.

Contributor

taku0 commented Oct 14, 2012

Added test for options.xul generation.
I am not sure how to test l10n.
It seems that there are no tests for conventional preference types.

@erikvold erikvold commented on an outdated diff Oct 15, 2012

packages/api-utils/lib/l10n/prefs.js
@@ -25,6 +25,12 @@ function onOptionsDisplayed(document, addonId) {
let title = core.get(name + "_title");
if (title)
node.setAttribute("title", title);
+
+ for (let item of node.querySelectorAll("menuitem, radio")) {
+ let label = core.get(item.getAttribute("label"));
@erikvold

erikvold Oct 15, 2012

Contributor

Let's use core.get(name + "_options." + item.getAttribute("label")) to stick to the convention already started.

Contributor

erikvold commented Oct 16, 2012

This branch should be updated, master has changed a bit. Other than the one change mentioned for the l10n this looks good. Once you do the merge with master I'll run the tests and it should be good!

Contributor

taku0 commented Oct 17, 2012

Merge succeeded without conflict and test_xpi.py works fine :).

Contributor

erikvold commented Oct 17, 2012

looks good! I just need to get home and try on Fennec now..

Contributor

erikvold commented Oct 17, 2012

hmm, there is an issue on Fennec with the menu drop down, when I select it I don't see the "left_button" option, and when I select a value I am asked to make a selection a second time. I can take a look into these issues.

Contributor

erikvold commented Oct 17, 2012

ok this looks like it is a platform bug to me

erikvold merged commit 940f7ae into mozilla:master Oct 17, 2012

Contributor

erikvold commented Oct 17, 2012

Thanks @taku0 !

Contributor

erikvold commented Oct 17, 2012

I filed bug 802651

Contributor

brettz9 commented Dec 12, 2012

This is cool. Does the menulist option allow for omitting the value or label to serve as the other? What about hierarchical menus? Thanks!

Contributor

brettz9 commented Dec 12, 2012

Also, any way to distinguish single select while allowing multiple select?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment