Comment on attachment 163906 [details] [review]
Patch to include switch menu items

style nits. I'm mentioning each issue only once, but most of them apply to several places in the patch.

>+function Switch() {
>+    this._init.apply(this,arguments);

space after comma

>+}
>+Switch.prototype = {

blank line between the function and the prototype definition

>+        this.actor = new St.BoxLayout({ style_class: "switch" });
>+        this._on = new St.Label({ style_class: "switch-label", text: _("ON") });

use "" around translated strings and '' around non-translated ones

>+        this._set(state);
>+    },
>+    _set: function(state) {

blank line between functions

>+        if(state) {

space between "if" and "("

>+        this._set(! this.state);

no space after unary operators ("!")

>+PopupSwitchMenuItem.prototype = {
>+    __proto__: PopupBaseMenuItem.prototype,
>+    _init: function(text, active) {

blank line between the __proto__ and _init lines

>+        this.active = Boolean(active);

hm... I guess that's so that if the caller didn't pass a value, it gets converted to "false" rather than being left as "undefined"? We don't use that convention anywhere else... I can't think off the top of my head what the "standard" idiom we use is...

(In reply to comment #0)
> Styling is not optimal, but it the closest possible to mockups.

I haven't actually seen what they end up looking like, but you probably want to use either SVGs or cairo to draw them.

Created attachment 164354 [details] [review]
Same patch, with correct style

Here it goes

Created attachment 165328 [details] [review]
[PopupSwitchMenuItem] support indeterminate state

Changes the way state is represented in a switch, which now can be
1/true/'true' for on, 0/false/'false' for off, or anything else for
indeterminate. Also, allow setting without toggling (and emitting
the related event), which is useful if some sort of processing is
necessary for the application before approving a state change.

do we need an indeterminate state anywhere?

the switches-in-menus are a little dubious anyway (see unanswered questions at http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/UniversalAccess), and this would just add a bit more dubiosity, since if you selected an "indeterminate" menu item, it would change to either "on" or "off", but the menu would also immediately close and so you wouldn't see which...

(In reply to comment #5)
> do we need an indeterminate state anywhere?

Not in current designs, but the possibility is there and adding it is a small correction. (Actually, dbusmenu requires it, that's why I added it, but it remains to be discussed whether we need dbusmenu)

> the switches-in-menus are a little dubious anyway (see unanswered questions at
> http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/UniversalAccess),

Answers from the current implementation:
1) Any activation to the menu items, either through the switch, by clicking the label, or by pressing enter, toggles the item and emits a 'toggled' signal. The behaviour of this, of course, depends on the application connected to it.
2) Toggles don't animate, as the menu is closed when the item is activated, and it is impossible to animate the change since it happens with two independent style pseudo-class changes (that is, you cannot slide)
3) Toggles never change unless activated (or the application explicitly requests a state change)
4) Toggles don't need dragging, just clicking / pressing Enter.

These answers come from a simple design: a PopupSwitchMenuItem is a GtkCheckMenuItem with a switch instead of a checkmark.

> and this would just add a bit more dubiosity, since if you selected an
> "indeterminate" menu item, it would change to either "on" or "off", but the
> menu would also immediately close and so you wouldn't see which...

1) If you prefer, I will change so that "indeterminate" items are not changed automatically, until the code controlling the menus changes them explicitly.
2) It should be possible to prevent the "activate" signal (and thus closing the menu) while still keeping the "toggled" signal. The switch by itself doesn't emit any signal, they all come from PopupSwitchMenuItem.

Comment on attachment 164354 [details] [review]
Same patch, with correct style

ok, I'm not going to worry about the look of the toggles; someone owes us some SVGs.

>+        this.label = new St.Label({ text: text });

I don't think this field needs to be public. Oh, but look, that's how PopupMenuItem does it. Sigh. OK, probably want to refactor this later...

Comment on attachment 164354 [details] [review]
Same patch, with correct style

oops. forgot the big problem I'd noticed:

>+        if (state) {
>+            this._on.add_style_pseudo_class('checked');
>+            this._on.text = '|||';
>+            this._off.remove_style_pseudo_class('checked');
>+            this._off.text = _("OFF");
>+        } else {
>+            this._off.add_style_pseudo_class('checked');
>+            this._off.text = '|||';
>+            this._on.remove_style_pseudo_class('checked');
>+            this._on.text = _("ON");
>+        }

This is backwards. The word that is visible should match the current state. (I can see how you could interpret them this way, but if you look through the mockups, you'll see that the mockups are definitely interpreting it the other way, which is the same way that toggle switches work on the iPhone, although it's a lot more obvious there due to the use of color and animation.)

Comment on attachment 165328 [details] [review]
[PopupSwitchMenuItem] support indeterminate state

so first, obviuosly this has the same backwardsness problem as the previous patch.

setToggleState is needed by some of the icons, so we need at least that part of the patch. I think you should remove "indeterminate" for now, and make setToggleState just take an ordinary boolean. If we want indeterminate later, we can add a separate setIndeterminate() method.

Also, this should just be squashed into the previous patch.

Created attachment 166341 [details] [review]
2 state toggle switches

Now settable and reversed.

Comment on attachment 166341 [details] [review]
2 state toggle switches

OK. I added a comment for translators re: _("ON")/_("OFF"), and
added popupMenu.js to po/POTFILES.in (which I forgot to check before)
and removed some trailing whitespace in the .js files that git
complained about when applying the patch.

Leaving the bug open since we will need to do additional work on
the look of the switches, once we get artwork.

Created attachment 166403 [details] [review]
Fix typo inside Switch class

Corrects setToogleState into setToggleState, slipped from 4dfc869e

Comment on attachment 166403 [details] [review]
Fix typo inside Switch class

Attachment 166403 [details] pushed as 0c38f49 - Fix typo inside Switch class

this got fixed at some point (replaced with SVG switches)