After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 625029 - Sliders in menus
Sliders in menus
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-07-22 12:40 UTC by Giovanni Campagna
Modified: 2010-10-01 19:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add horizontal sliders to PopupMenus (8.07 KB, patch)
2010-07-22 12:42 UTC, Giovanni Campagna
needs-work Details | Review
Add horizontal sliders to PopupMenus (fixed) (10.07 KB, patch)
2010-08-10 18:08 UTC, Giovanni Campagna
needs-work Details | Review
Add JS based sliders for menus (7.76 KB, patch)
2010-09-15 20:22 UTC, Giovanni Campagna
none Details | Review
Add menu sliders (7.57 KB, patch)
2010-09-26 14:48 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2010-07-22 12:40:39 UTC
One of the widgets missing from the PopupMenu set is the slider, used for example by audio volume.
We need an horizontal slider, instead of a vertical one, to keep the overall look consistent (it becomes mostly a standard menu item then).
Comment 1 Giovanni Campagna 2010-07-22 12:42:37 UTC
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.
Comment 2 Dan Winship 2010-08-02 16:34:56 UTC
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)
Comment 3 Giovanni Campagna 2010-08-07 12:31:01 UTC
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);
Comment 4 Florian Müllner 2010-08-07 12:43:36 UTC
(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.
Comment 5 Dan Winship 2010-08-09 19:11:18 UTC
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
Comment 6 Giovanni Campagna 2010-08-10 18:08:13 UTC
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 7 Dan Winship 2010-08-30 16:46:31 UTC
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.
Comment 8 Giovanni Campagna 2010-08-30 21:26:53 UTC
(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.
Comment 9 Florian Müllner 2010-09-07 09:50:58 UTC
Wouldn't it be better to import MxToggle (http://git.clutter-project.org/mx/tree/mx/mx-toggle.c)?
Comment 10 Giovanni Campagna 2010-09-07 09:55:29 UTC
I guess this comment was meant for bug 621880 (the switches/toggles, which are already in master gnome-shell), not this.
Comment 11 Giovanni Campagna 2010-09-15 20:22:44 UTC
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.
Comment 12 Owen Taylor 2010-09-16 15:59:28 UTC
Dan - Can you help Giovanni figure out an approach to deal with the grabbing issue?
Comment 13 Dan Winship 2010-09-21 17:45:17 UTC
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.
Comment 14 Giovanni Campagna 2010-09-26 14:48:05 UTC
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.
Comment 15 Dan Winship 2010-10-01 19:23:21 UTC
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