Created attachment 166385 [details] [review]
Add horizontal sliders to PopupMenus

Introduce Cairo-drawn sliders to be used in PopupMenus (for example for
volume). They are stylable to some extent (colors, border width, slider
height) and have the standard behaviour of a slider, except they are
completely modal (once you start dragging, all events are intercepted by
the slider, which thus is kept active and highlighted at all times).
They show numeric values between 0 and 1 (scaling must be performed outside)
and emit value-changed on button release, but no activate, keeping the
menu open.

this will be easier to review if you attach an implementation of the volume menu too :-)

(even if it's not hooked up to the actual volume yet)

The volume menu cannot be implemented yet, cause libgnomevolumecontrol (the library used by gnome-settings-daemon and gnome-volume-control-applet) is not public and/or introspected.
If you just want to test a slider, the code is simple:

const PopupMenu = imports.ui.popupMenu
let item = new PopupMenu.PopupSliderMenuItem(0.5);
item.connect('value-changed', function(item, value) { /* do something */ });
Main.panel._statusmenu.menu.addMenuItem(item);

(In reply to comment #3)
> The volume menu cannot be implemented yet, cause libgnomevolumecontrol (the
> library used by gnome-settings-daemon and gnome-volume-control-applet) is not
> public and/or introspected.

As far as I know, the volume slider is supposed to access pulse directly instead of using libgnomevolumecontrol.


> If you just want to test a slider, the code is simple:

This is what I understand from Dan's comment.

Review of attachment 166385 [details] [review]:

::: js/ui/popupMenu.js
@@ +3,3 @@
 const Cairo = imports.cairo;
+const Lang = imports.lang;
+const Signals = imports.signals;

the imports were right before; the "imports.foo" and "imports.gi.Foo" imports should be mixed together in a single group

@@ +191,3 @@
+
+    _init: function(value) {
+        // HACK: prevent activation behaviour

boo!

it would be better to tweak PopupBaseMenuItem to allow for the behavior you want without hacking around it

Also, I think it seems weird to have the menu item highlight when you move over it. Maybe it would be better to have the "knob" highlight somehow instead?

@@ +199,3 @@
+            // Avoid spreading NaNs around
+            throw TypeError('The slider value must be a number');
+        this._tmpValue = this._value = Math.max(Math.min(value, 0), 1);

tmpValue isn't a very obvious name. How about "displayValue" ?

@@ +225,3 @@
+        let found, handleRadius;
+        [found, handleRadius] = themeNode.get_length('-slider-handle-radius', false);
+        

kill the whitespace on that blank line

@@ +267,3 @@
+    },
+    
+    _startDragging: function(actor, event) {

kill whitespace on the previous line

@@ +277,3 @@
+    },
+    
+    _onEventCapture: function(stage, event) {

kill whitespace on the previous line

@@ +987,3 @@
             } else {
                 this._closeMenu();
+                return false;

this is wrong. it means that other actors *outside the menu structure* will get a chance to receive the button-release-event that caused the menu to close. Eg, if you go into the overview, then press on the user menu, move the mouse out of the menu to a window thumbnail, and then release the mouse button, that window will be selected

Created attachment 167536 [details] [review]
Add horizontal sliders to PopupMenus (fixed)

Removed the hack, now the reactivity of PopupBaseMenuItems is orthogonal to activability.
Also, removed highlight on hover. The knob cannot be highlighted because it is already white.

Comment on attachment 167536 [details] [review]
Add horizontal sliders to PopupMenus (fixed)

The grabbing is still not quite right. Once you click or drag on the slider, you can no longer click outside the menu to dismiss it, and then the keyboard state seems to get really messed up, and Alt-F2 no longer works right, etc, and you have to kill the shell.

>-    _init: function (reactive) {
>+    _init: function (reactive, activatable) {

it looks like it would be simpler to flip these to nonReactive, nonActivatable, and then you could just call PopupBaseMenuItem._init.call(this) for most of the subclasses, and only need to specify values for those args for separators and sliders.

>+PopupSliderMenuItem.prototype = {
>+    __proto__: PopupBaseMenuItem.prototype,
>+
>+    _init: function(value) {
>+        PopupBaseMenuItem.prototype._init.call(this, true, false);
>+ 

that last blank line has a space. (If you do "git config --global color.ui auto", then "git diff" will highlight added trailing whitespace for you.)

>+    _moveHandle: function(absX, absY) {
>+        // FIXME: this fails if you apply transformations more complex than
>+        // just translating

That's true virtually everywhere in the shell. You don't need to call it out here.

(In reply to comment #7)
> (From update of attachment 167536 [details] [review])
> The grabbing is still not quite right. Once you click or drag on the slider,
> you can no longer click outside the menu to dismiss it, and then the keyboard
> state seems to get really messed up, and Alt-F2 no longer works right, etc, and
> you have to kill the shell.

Actually, the grabbing cannot be right in its current form. It relies on a captured-event with a type of BUTTON_RELEASE, but as the name says, the event is captured, so goes from the stage to the destination actors, and first stops at PopupMenu.actor (the listener set by PopupMenuManager).
Unless PopupMenuManager returns true on BUTTON_RELEASE, PopupSliderMenuItem._onEventCapture and thus PopupSliderMenuItem._endDragging won't be called.
OTOH, if true is returned, clicks outside the menu are forwarded normally and thus may focus windows or activate app icons.
I can't see how to resolve this without subgrabbing specially for sliders.

Wouldn't it be better to import MxToggle (http://git.clutter-project.org/mx/tree/mx/mx-toggle.c)?

I guess this comment was meant for bug 621880 (the switches/toggles, which are already in master gnome-shell), not this.

Created attachment 170371 [details] [review]
Add JS based sliders for menus

Rebased against current HEAD and 4e1de26a, also some bugs fixed. Still has problems with grabs, though.

Dan - Can you help Giovanni figure out an approach to deal with the grabbing issue?

OK, I think the issue is that instead of doing Main.pushModal(), the slider should be doing Clutter.grab_pointer(). Main.pushModal() is about "redirect the keyboard and pointer to the shell UI", but that has already been done by the PopupMenuManager, so we don't need to do it again. Calling Clutter.grab_pointer() should allow you to process the events without PopupMenuManager's captured-event handler ever seeing them.

Created attachment 171129 [details] [review]
Add menu sliders

Grabbing fixed, highlighting reintroduced so that keyboard focus can be easily identified, which is useful now that left and right keys can be used to set the slider.

ok, not sure if we always want a fixed delta value for keyboard navigation of sliders, but I don't have a better suggestion, so we'll see how it plays out