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 415775 - Add a volume widget
Add a volume widget
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 439532 439799 439800
 
 
Reported: 2007-03-07 17:03 UTC by Bastien Nocera
Modified: 2007-06-30 08:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk-volume-button-1.patch (33.58 KB, patch)
2007-03-07 17:07 UTC, Bastien Nocera
needs-work Details | Review
gtk-volume-button-2.patch (35.03 KB, patch)
2007-03-08 12:01 UTC, Bastien Nocera
needs-work Details | Review
gtk-volume-button-3.patch (33.69 KB, patch)
2007-03-09 17:24 UTC, Bastien Nocera
needs-work Details | Review
gtk-volume-button-4.patch (36.24 KB, patch)
2007-03-16 16:01 UTC, Bastien Nocera
needs-work Details | Review
gtk-volume-button-grab-test.patch (37.56 KB, patch)
2007-03-16 18:25 UTC, Bastien Nocera
rejected Details | Review
gtk-volume-button-5.patch (38.96 KB, patch)
2007-05-11 12:58 UTC, Bastien Nocera
none Details | Review
gtk-volume-button-7.patch (39.47 KB, patch)
2007-05-11 16:00 UTC, Bastien Nocera
none Details | Review
gtk-volume-button-8.patch (40.65 KB, patch)
2007-05-12 15:07 UTC, Bastien Nocera
none Details | Review
gtk-volume-button-9.patch (49.19 KB, patch)
2007-05-12 21:06 UTC, Bastien Nocera
reviewed Details | Review
gtk-volume-button-10.patch (53.20 KB, patch)
2007-05-14 17:37 UTC, Bastien Nocera
reviewed Details | Review
gtk-volume-button-11.patch (54.56 KB, patch)
2007-05-19 11:57 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2007-03-07 17:03:09 UTC
This would be used by multimedia applications in their droves.

First patch attached.

The widget is called GtkVolumeButton, and is a rework of the BaconVolumeButton used by Totem, Rhythmbox, Muine, Sound-Juicer, and others.
Comment 1 Bastien Nocera 2007-03-07 17:07:47 UTC
Created attachment 84184 [details] [review]
gtk-volume-button-1.patch

- all the apps use either a range between 0-100, or between 0 and 1, should we remove the ability to set the range altogether?
- finish documenting the few public functions
- allow the GtkIconSize to be changed as a property
- more code cleanups/comments, and change coding style.
- make the percentage tooltip translatable
- optimise the icon loading (ie. size not really changing)
Comment 2 Havoc Pennington 2007-03-07 17:17:43 UTC
single precision floats are pretty much nothing but evil, I'd nuke those puppies...

have you guys considered a library just for media apps?
Comment 3 Bastien Nocera 2007-03-07 17:23:45 UTC
(In reply to comment #2)
> single precision floats are pretty much nothing but evil, I'd nuke those
> puppies...

Percentage as an int it would be then.

> have you guys considered a library just for media apps?

There wouldn't be many widgets in there. Apart from the volume button, and a better seek slider to replace scales, there's pretty much nothing in media apps that normal apps wouldn't want to use as well.

Comment 4 Aaron Bockover 2007-03-07 18:09:56 UTC
Bastien, I've got some ideas with this. I think this widget can be transformed slightly, and renamed, so it's not tied to "volume." It'd be more generic and probably more suitable for GTK inclusion. 

I think this same widget could be very useful in applications like EOG, F-Spot, Evince, etc - anything that has a notion of "zoom". I've tried it in F-Spot and it feels very natural.
Comment 5 Bastien Nocera 2007-03-07 18:40:25 UTC
I have no problems making the widget more generic.
But:
- how would you set the icon set to be used?
- what would the default range be?
- what would you call it?
Comment 6 Ronald Bultje 2007-03-07 18:47:31 UTC
Just keep the default range configurable (and keep the doubles, we use a GtkScale/GtkAdjustment, which uses doubles as well, so there is no reason to do differently - it will just cause issues).
Comment 7 Aaron Bockover 2007-03-07 19:32:41 UTC
I'll start playing with the code to make it more generic this evening, so I've not settled on anything for setting the icon set. However, in my port I changed how the icons were picked using a simple one or two line formula vs. a bunch of conditionals with hard coded values. Thus the icon set can be of any size and the right icon will be chosen based on the scale value.

Essentially an array of icon names is all that'd be needed.

I agree with Ronald - I don't really see why the range support needs to change, and again because the GtkScale/Adjustment is used internally, which uses doubles, I don't see why we'd need to change that at a higher level either. If the range does change, it should be normalized as a percentage, like the progress bar.

As for the name, I'm completely unoriginal:

GtkScaleButton
GtkButtonScale
GtkPopupScale
GtkScaleButtonPopup
GtkButtonWithAPopupScale
...

Comment 8 Aaron Bockover 2007-03-07 19:35:41 UTC
Also, in the case of volume, a [0,100] interval makes perfect sense. If we get more generic, this interval doesn't make much sense anymore. For example, the whole point of zooming is to go beyond 100%.
Comment 9 Havoc Pennington 2007-03-07 19:43:31 UTC
If the two use cases are volume and zoom you could have two precanned "modes" that set up the right image and a default interval. Not sure what the API would look like, a _new(PopupScaleType type) convenience function doesn't language bind very well.
Comment 10 Ronald Bultje 2007-03-07 19:51:07 UTC
The 0,100 only makes sense because we use that. GStreamer uses a float property, and other systems will use something else. Please just make it settable, it'll prevent a whole lot of stupidity later on that could've been prevented, and is no effort or burden whatsoever, not for the app developer and not those maintaining the widget (who would that be anyway?).

_scale_new_for_volume(0.0, 1.0) and _scale_new_for_zoom(-10, 10) as shortcuts for _scale_new (volume_icon_array, 0.0 1.0) and so on sounds good to me

As for the media lib that Havoc proposed, how about sharing a playlist widget between apps? I'm pretty sure our playlist widgets are only slightly different (RB/Totem share one; I wrote my own for something else, but I'm 99% sure it's conceptually the same).
Comment 11 Aaron Bockover 2007-03-07 20:11:49 UTC
Ronald: I meant the [0,100] interval to demonstrate that volume is much more limited than zoom. Zoom is theoretically more like [0,+inf). I don't really care what the scale is, and agree with you - it makes more sense for volume to technically be [0.0,1.0].

Default intervals (can be changed with a later call) should probably be provided with each "profile". For volume, [0.0,1.0] and for zoom, [0.0,8.0]. Something like that.
Comment 12 Rui Matos 2007-03-07 23:45:48 UTC
[I'm assuming this widget is the one used to control volume on rhythmbox's top right corner.]

From a usability POV I think this widget should work like a simple Scale, either vertical or horizontal.

I mean: I know that both this widget and a plain Scale can be actuated upon with a sigle click and a drag. But, for a new user, he sees this widget as a button and will click once on the button and then click again to drag (I've seen this and used it like that myself). Well, clicking once is better than clicking twice. Besides, visually, I think it is much easier to have a grasp of the volume by looking at a simple Scale widget. It's just much more directly manipulatable.

So, I vote for this widget to be like F-Spot's zoom widget. I know that it would then use more space on screen but many of the apps that are using this now, like rhythmbox and totem, *do* have that space available.
Comment 13 Aaron Bockover 2007-03-08 00:08:45 UTC
The widget does allow for clicking and then adjusting the volume, like a normal scale. It also allows click+drag+release to adjust. It has a tooltip to show the actual volume level and the theme icons do a decent job of showing relative volume level (mute, min, med, max).

If this widget were like F-Spot's zoom widget, there would be no widget - you'd just use a regular slider. Part of the point of this widget is to be more compact and it also looks nicer than a constantly exposed slider. I do not think there's room for showing a standard slider in Rhythmbox, Banshee, or Totem. That area should be consumed (at least in RB and Banshee) by metadata, etc.

I don't think this widget is any less usable than a regular slider.
Comment 14 Ronald Bultje 2007-03-08 00:19:31 UTC
It's actually more usable. B/c of the icon, you get instant information on what the button does. Since all (media) apps share it, it's a learn-once-use-always thing for users, too.

A slider is just a slider, like all the million other sliders on GNOME, e.g. the brightness slider in gimp, the quality slider in banshee's vorbis encoder, etc. I just looked at the f-spot screenshots to see what you're talking about and I still haven't been able to figure out which slider in f-spot is supposed to be the zoom slider, and I'm a programmer!

Anyway, this discussion has been had before in the totem bug report, please re-read that and let's leave this bug report for the potential inclusion of this widget in gtk+.
Comment 15 Bastien Nocera 2007-03-08 12:01:04 UTC
Created attachment 84237 [details] [review]
gtk-volume-button-2.patch

> - all the apps use either a range between 0-100, or between 0 and 1, should we
> remove the ability to set the range altogether?

Won't do then. The ranges have been changed to use gdoubles instead.

> - allow the GtkIconSize to be changed as a property

Done.

> - optimise the icon loading (ie. size not really changing)

Done.

Also done:
- avoid the popup getting outside of the screen if we reach the top or bottom of it (place the widget at the top of the screen, set the volume to 0, click on the button, the popup would be partly offscreen)
- make the adjustment of the range gettable, so it can be modified (for bindings)

Still to do:
> - finish documenting the few public functions
> - more code cleanups/comments, and change coding style.
> - make the percentage tooltip translatable

A popup bug:
- Make the button big, click and release on the outside of the button (so the scale doesn't popup underneath the cursor), click once on the scale, it doesn't pop down.
- Make gtk_volume_button_update_icon() use the number of icons in our list to update its status, rather than hardcode 4 states
Comment 16 Matthias Clasen 2007-03-08 17:03:28 UTC
In general, I think such a widget would be an ok addition to GTK+.

Some comments after reading the patch:

The widget is not quite ready to be used for other things, like zoom, since
"Volume" is used in the widget name, property name and in various user-visible strings, like a11y names.+							


P_("The GtkAdjustment that contains the current value of this range object"),

Should not talk about a range object here, I think.


+  button->plus = gtk_button_new_with_label (_("+"));

When making short strings like this translatable, please use Q_() and
a prefix. Does it make any sense to translate "+" and "-" here ? Are they
actually translated for any languages in the current users of this widget ?


Use gdk_display_ functions instead of gdk_pointer_grab and gdk_keyboard_grab.

The widget should handle grabs being broken (e.g by a modal dialog popping up).

Instead of the current manual pixbuf-loading and -setting, it would be much 
nicer to use gtk_image_set_from_icon_name() and let GtkImage handle theme 
changes...


Should there be a toolitem version, for use in toolbars ? 
 
Comment 17 Bastien Nocera 2007-03-09 01:34:38 UTC
(In reply to comment #16)
> In general, I think such a widget would be an ok addition to GTK+.
> 
> Some comments after reading the patch:
> 
> The widget is not quite ready to be used for other things, like zoom, since
> "Volume" is used in the widget name, property name and in various user-visible
> strings, like a11y names.+                                                      

The a11y names, and other strings would need to be changed. Is "GtkScaleButton" be a good name? Then we could have a GtkVolumeButton that would implement the specifics of a volume button (a11y name, tooltip, properties)

> P_("The GtkAdjustment that contains the current value of this range object"),
> 
> Should not talk about a range object here, I think.

Agreed.

> +  button->plus = gtk_button_new_with_label (_("+"));
> 
> When making short strings like this translatable, please use Q_() and
> a prefix. Does it make any sense to translate "+" and "-" here ? Are they
> actually translated for any languages in the current users of this widget ?

I didn't find it translated into any languages for Totem, or Rhythmbox, so I removed the translations.

> Use gdk_display_ functions instead of gdk_pointer_grab and gdk_keyboard_grab.

There's a gdk_display_pointer_ungrab, but no grab. Same for keyboard. Which functions were you talking about?

> The widget should handle grabs being broken (e.g by a modal dialog popping up).

How does one do that? Do you have any examples?

> Instead of the current manual pixbuf-loading and -setting, it would be much 
> nicer to use gtk_image_set_from_icon_name() and let GtkImage handle theme 
> changes...

Should we implement fallback for icon names in that case, or stick with the newer names in the Volume side of the widget?

> Should there be a toolitem version, for use in toolbars ? 

There should be.
Comment 18 Matthias Clasen 2007-03-09 02:57:35 UTC
> > Use gdk_display_ functions instead of gdk_pointer_grab and gdk_keyboard_grab.
> 
> There's a gdk_display_pointer_ungrab, but no grab. Same for keyboard. Which
> functions were you talking about?

Sorry, I meant the multihead-safe ungrab functions.


> > The widget should handle grabs being broken (e.g by a modal dialog popping up).
> 
> How does one do that? Do you have any examples?

Look for grab_broken implementations, e.g. in gtkrange.c

> > Instead of the current manual pixbuf-loading and -setting, it would be much 
> > nicer to use gtk_image_set_from_icon_name() and let GtkImage handle theme 
> > changes...
>
> Should we implement fallback for icon names in that case, or stick with the
> newer names in the Volume side of the widget?

I'd go with one set of names; we can put icons for that in the builtin icon theme, so they will always be available. To allow zoom and other uses, the icon names have to be made configurable anyway, right ?
Comment 19 Bastien Nocera 2007-03-09 17:24:40 UTC
Created attachment 84318 [details] [review]
gtk-volume-button-3.patch

Remove all mentions of volume, remove hard-coded icons/number of icons.

The tests/testvolumebutton.c shows how to implement a volume widget (which I'd add to GTK+ as well once the GtkScaleButton gets added). The descendants would be the ones adding the a11y names, and tooltips.
Comment 20 Bastien Nocera 2007-03-16 15:56:38 UTC
(In reply to comment #18)
> > > Use gdk_display_ functions instead of gdk_pointer_grab and gdk_keyboard_grab.
> > 
> > There's a gdk_display_pointer_ungrab, but no grab. Same for keyboard. Which
> > functions were you talking about?
> 
> Sorry, I meant the multihead-safe ungrab functions.

We already use those:
$ grep ungrab gtkscalebutton.c 
          gdk_display_pointer_ungrab (display, event->time);
          gdk_display_pointer_ungrab (display, event->time);
  /* ungrab focus */
  gdk_display_keyboard_ungrab (display, event->time);
  gdk_display_pointer_ungrab (display, event->time);
      /* ungrab focus */
      gdk_display_keyboard_ungrab (display, event->time);
      gdk_display_pointer_ungrab (display, event->time);
  gdk_display_keyboard_ungrab (display, GDK_CURRENT_TIME);
  gdk_display_pointer_ungrab (display, GDK_CURRENT_TIME);

> > > The widget should handle grabs being broken (e.g by a modal dialog popping up).
> > 
> > How does one do that? Do you have any examples?
> 
> Look for grab_broken implementations, e.g. in gtkrange.c

I don't seem to receive any "grab-notify" event, or "grab-broken-event" when the  dialog pops up. I'll upload a test case.

> > > Instead of the current manual pixbuf-loading and -setting, it would be much 
> > > nicer to use gtk_image_set_from_icon_name() and let GtkImage handle theme 
> > > changes...
> >
> > Should we implement fallback for icon names in that case, or stick with the
> > newer names in the Volume side of the widget?
> 
> I'd go with one set of names; we can put icons for that in the builtin icon
> theme, so they will always be available. To allow zoom and other uses, the icon
> names have to be made configurable anyway, right ?

Done.
Comment 21 Bastien Nocera 2007-03-16 16:01:50 UTC
Created attachment 84723 [details] [review]
gtk-volume-button-4.patch

The only problems missing are the grab problems mentioned above, and the behavioural bug mentioned earlier.

Docs are in (although they're not being built, figuring out why now), we also use the double-click time as the base for the click timeout rather than hard coding a value.
Comment 22 Bastien Nocera 2007-03-16 18:25:49 UTC
Created attachment 84734 [details] [review]
gtk-volume-button-grab-test.patch

A test patch.
Compile, and start "tests/testvolumebutton", then press "Space" to get a popup (using the mouse will make the grab yo-yo, and the popup never shows up). The dialog popping up seconds afterwards doesn't trigger a cb_dock_grab_broken_event, but does trigger cb_dock_grab_notify.
Comment 23 Alex L. Mauer 2007-05-09 22:07:42 UTC
I'd like to see this widget allow a middle-click to mute and unmute the volume as well.
Comment 24 Matthias Clasen 2007-05-10 17:30:04 UTC
First of all, the grab-broken thing is a misunderstanding. a modal dialog by itself does not grab the pointer or keyboard, thus you don't get a grab-broken 
event. It does add a gtk grab though, so you do get a a grab-notify.

Looks like the button_press and key_release implementations share 90% of their code. Can that be factored out ?
Comment 25 Matthias Clasen 2007-05-10 17:50:52 UTC
I believe most of your grab problems are due to the fact that the scale adds a grab on a child of the dock.

Adding

  if (gtk_widget_is_ancestor (gtk_grab_get_current (), button->dock))
    return;

to the grab_notify implementation should filter that out.


I also notice that the keep-on-screen logic does not seem to work for the bottom of the screen.
Comment 26 Matthias Clasen 2007-05-10 18:01:00 UTC
further notes on the keep-on-screen logic:
- needs to ensure horizontal on-screen-ness, too
- must work in terms of monitor dimensions, not screen (think xinerama)
Comment 27 Bastien Nocera 2007-05-11 10:20:36 UTC
(In reply to comment #24)
> First of all, the grab-broken thing is a misunderstanding. a modal dialog by
> itself does not grab the pointer or keyboard, thus you don't get a grab-broken 
> event. It does add a gtk grab though, so you do get a a grab-notify.

I don't get a grab-notify with this implementation unless I click on the popup again.


Comment 28 Bastien Nocera 2007-05-11 12:58:13 UTC
Created attachment 88008 [details] [review]
gtk-volume-button-5.patch

- Make sure the scale stays inside the monitor and the screen, both horizontally and vertically (the old code was definitely broken, as it used the width/height from the button, not the popup)
- Ignore grab notifies when it's a child of the dock that's doing the grab
- Factored the button press and key release code

TODO:
- implement the volume widget in a separate widget
- implement a GtkToolScaleButton
- Figure out why, when a dialogue pops up, and we've clicked and kept the mouse button pressed on the scale, the scale doesn't disappear (we don't get a grab notify, or a grab broken event)
Comment 29 Bastien Nocera 2007-05-11 16:00:24 UTC
Created attachment 88023 [details] [review]
gtk-volume-button-7.patch

- Fixed the dock not popping down when the mouse button was being held on the scale (thanks Matthias)
- Fixed copyright assignments (let me know if they don't match)
Comment 30 Matthias Clasen 2007-05-12 01:01:46 UTC
Should be Red Hat, Inc. in the copyright line.

The header could be moved a tiny bit close to gtk coding
style, by separating the typedef from the structure declaration
for the class struct, aligning the parameters and using
gchar.

Can we do better than hardcoding the size of the scale ?

All doc comments should have a "Since: 2.12"  line, and 
references to functions should use foo(), not #foo. Please
break the doc comments into reasonably sized lines.

Some more coding style trivia: We put { after if or for
on a line by itself, with an indent of 2.

g_return_if_fail checks in public entry points should 
check GTK_IS_SCALE_BUTTON(), not != NULL

Is there any technical reason to demand >= 3 icons ? I mean,
we could just do the right thing with 1 or 2 icons, too.

This
  for (l = button->icon_list; l != NULL; l = l->next)
    g_free (l->data);
could just be g_list_foreach()


Comment 31 Bastien Nocera 2007-05-12 15:03:09 UTC
(In reply to comment #30)
> Should be Red Hat, Inc. in the copyright line.

Fixed.

> The header could be moved a tiny bit close to gtk coding
> style, by separating the typedef from the structure declaration
> for the class struct, aligning the parameters and using
> gchar.

Done.

> Can we do better than hardcoding the size of the scale ?

Can do. How about making the scale size a multiplier of the icon size?

> All doc comments should have a "Since: 2.12"  line, and 
> references to functions should use foo(), not #foo. Please
> break the doc comments into reasonably sized lines.

Done.

> Some more coding style trivia: We put { after if or for
> on a line by itself, with an indent of 2.

Done.

> g_return_if_fail checks in public entry points should 
> check GTK_IS_SCALE_BUTTON(), not != NULL

Done.

> Is there any technical reason to demand >= 3 icons ? I mean,
> we could just do the right thing with 1 or 2 icons, too.

It's just not very useful with 1 or 2 icons, as you don't have any feedback from the icon as to what the value could be. I added special-casing for the 1 and 2 icons cases, and modified the documentation to say that 3 icons or more is better.

> This
>   for (l = button->icon_list; l != NULL; l = l->next)
>     g_free (l->data);
> could just be g_list_foreach()

Duh. Fixed.

Comment 32 Bastien Nocera 2007-05-12 15:07:36 UTC
Created attachment 88080 [details] [review]
gtk-volume-button-8.patch

I also fixed the >=3 icons case where the docs didn't match the behaviour. First icon name is the lowest value, 2nd is the highest, 3rd and beyond spread evenly over the rest of the values.
Comment 33 Ronald Bultje 2007-05-12 15:25:32 UTC
I'm not sure if this is relevant, but the widget doesn't work under the Quartz-port of Gtk+ on OS X. When popped up, clicking on part of the main window or another window hides it. However, clicking on the desktop, menubar, statusbar or other parts of the desktop doesn't. This looks like a bug caused by the fact that X has a root window over the whole screen, and Quartz apparently doesn't.

If quartz is considered a supported platform, this should probably be fixed or worked around before this is committed.
Comment 34 Bastien Nocera 2007-05-12 21:06:06 UTC
Created attachment 88098 [details] [review]
gtk-volume-button-9.patch

- Fix PROP_SIZE's set_property missing a break statement
- Order GtkScaleButton properly in the alphabetical lists
- Use GtkScaleButtonPrivate so that we could potentially export some objects in the widget
- Add GtkVolumeButton
- Plenty of little indentation to make it look better
Comment 35 Christian Persch 2007-05-13 12:14:45 UTC
+ * Copyright (C) 2005 Ronald S. Bultje
+ * Copyright (C) 2007 Red Hat, Inc.
+ *
+ * Authors:
+ * - Ronald S. Bultje <rbultje@ronald.bitfreak.net>
+ * - Bastien Nocera <bnocera@redhat.com>

I'm not sure which version of bacon-volume this was forked from; but if it contains the patches, or portions of the patches from bug 369136, bug 370028 or bug 419571 then please add myself as (C) 2006, 2007 too. And looking at the bacon-volume.c svn history, probably Jan Arne Petersen holds (C) too.


+typedef struct {
[...]
+  GList *icon_list;

The icon list is set as a strv array, and all accesses are by g_list_nth; maybe it just should be stored as an array too?

+GtkWidget *
+gtk_scale_button_new (GtkIconSize   size,
[...]

This does more than just g_object_new, which is bad for language bindings.

+gtk_scale_button_init (GtkScaleButton *button)
+{
+  GtkWidget *frame, *box;
+  GtkScaleButtonPrivate *priv;
+
+  priv = GET_PRIVATE (button);
[and in more places]

GET_PRIVATE uses G_TYPE_INSTANCE_GET_PRIVATE; can't you just store the priv pointer in the GtkScaleButton struct as usual?

+  if (event->keyval != GDK_space && event->keyval != GDK_Return)
+    return FALSE;

What about GDK_ISO_Enter and GDK_KP_Enter? And should it also popup for Shift/Ctrl/Whatever+one of those keys?
Comment 36 Bastien Nocera 2007-05-13 13:17:57 UTC
(In reply to comment #35)
> + * Copyright (C) 2005 Ronald S. Bultje
> + * Copyright (C) 2007 Red Hat, Inc.
> + *
> + * Authors:
> + * - Ronald S. Bultje <rbultje@ronald.bitfreak.net>
> + * - Bastien Nocera <bnocera@redhat.com>
> 
> I'm not sure which version of bacon-volume this was forked from; but if it
> contains the patches, or portions of the patches from bug 369136, bug 370028 or
> bug 419571 then please add myself as (C) 2006, 2007 too. And looking at the
> bacon-volume.c svn history, probably Jan Arne Petersen holds (C) too.

I'll double-check and fix this, thanks for the eagle-eye.

> +typedef struct {
> [...]
> +  GList *icon_list;
> 
> The icon list is set as a strv array, and all accesses are by g_list_nth; maybe
> it just should be stored as an array too?

I prefer GList's but I could be tempted using a GSList instead. I'm not sure it buys us much.

> +GtkWidget *
> +gtk_scale_button_new (GtkIconSize   size,
> [...]
> 
> This does more than just g_object_new, which is bad for language bindings.

It's just a helper function, all the rest can be done with:
- gtk_scale_button_set_icons()
- the adjustment property
- the size property
which is what the GtkVolumeButton uses.

> +gtk_scale_button_init (GtkScaleButton *button)
> +{
> +  GtkWidget *frame, *box;
> +  GtkScaleButtonPrivate *priv;
> +
> +  priv = GET_PRIVATE (button);
> [and in more places]
> 
> GET_PRIVATE uses G_TYPE_INSTANCE_GET_PRIVATE; can't you just store the priv
> pointer in the GtkScaleButton struct as usual?

I guess this is only useful for older objects that can't be extended without breaking ABI.

> +  if (event->keyval != GDK_space && event->keyval != GDK_Return)
> +    return FALSE;
> 
> What about GDK_ISO_Enter and GDK_KP_Enter? And should it also popup for
> Shift/Ctrl/Whatever+one of those keys?

Nod. Will fix.

I also noticed that I dropped the ATK names for some widgets, and I'm missing tooltips support in the volume button, will fix as well.
Comment 37 Emmanuele Bassi (:ebassi) 2007-05-13 13:57:23 UTC
(In reply to comment #36)

> > GET_PRIVATE uses G_TYPE_INSTANCE_GET_PRIVATE; can't you just store the priv
> > pointer in the GtkScaleButton struct as usual?
> 
> I guess this is only useful for older objects that can't be extended without
> breaking ABI.

G_TYPE_INSTANCE_GET_PRIVATE() is a type look-up and check (which is constant time, but it's expensive anyway), while using a priv pointer is a pointer dereference.

Comment 38 Bastien Nocera 2007-05-14 17:37:09 UTC
Created attachment 88167 [details] [review]
gtk-volume-button-10.patch

- Export helper _get_adjustment and _set_adjustment, for the benefit of the descendants (makes it easy to get to adj->lower/adj->upper, not used by the volume widget)
- Export the plus and minus widgets in the scale (so that descendants can set nice names for them)
- popup the scale on GDK_ISO_Enter and GDK_KP_Enter as well, but don't when Ctrl or shift is pressed at the same time
- add tooltips and atk names to the volume button
- Use char** internally for the icon list
- Use a GtkScaleButtonPrivate struct member rather than g_type_class_add_private
- Add Jan and Christian as authors of the scale button widget

I'll punt using the icon size to calculate the scale size to another bug when this is committed.
Comment 39 Matthias Clasen 2007-05-18 18:27:12 UTC
I hate to bring up more things, but in the light of bug 433593, I believe it would be prudent to add keybindings for popup/popdown of the popup.


Also, if we are adding the volume widget too, we should have builtin audio icons, 
I think.


+<!ENTITY GtkScaleButton SYSTEM "xml/gtkscalebutton.xml">

it is not enough to define those entities, you must also use them in the document.


Other than that, it looks really good now.
Comment 40 Ronald Bultje 2007-05-18 18:50:34 UTC
Can I please again bring up the fact that this thing doesn't work at all under gtk-quartz? Is that a supported platform or not yet?
Comment 41 Havoc Pennington 2007-05-18 19:04:47 UTC
I don't see anything weird or platform-specific in the patch so presumably it's a quartz backend bug, not a problem with this patch, right?
Comment 42 Ronald Bultje 2007-05-18 19:46:22 UTC
True, I was more hinting towards fixing whtever bug causes this in gtk-quartz. :-).
Comment 43 Matthias Clasen 2007-05-18 19:58:24 UTC
We are usually not holding up generally useful features due to non-workingness on some platforms. Last i heard, dnd is still largely broken on win32...
Comment 44 Ronald Bultje 2007-05-18 20:47:34 UTC
Mind if I open a new bug for that then?
Comment 45 Matthias Clasen 2007-05-18 21:35:34 UTC
Not at all.
Comment 46 Bastien Nocera 2007-05-19 11:57:39 UTC
Created attachment 88437 [details] [review]
gtk-volume-button-11.patch

- Use GtkBindingSet to popup and popdown the dock
- Fix gtk-docs.sgml to mention the 2 added buttons

Can I punt the "add icons to gtk-stock" to another bug? I'd like to give the opportunity to the Tango hackers to add their versions of the volume icons, with the .xcf files
Comment 47 Matthias Clasen 2007-05-19 14:21:18 UTC
Thanks for staying with me so far; I'm fine with deferring the icon addition to a  followup bug. One thing noticed in passing:

+ * either the dock, or the scale itself */

Should move the closing */ to the next line. Please commit to trunk. 
Please split it into two commits, one for the scale widget, and one for the 
volume widget. Thanks.
Comment 48 Bastien Nocera 2007-05-19 23:18:13 UTC
Committed the GtkScaleButton

2007-05-19  Bastien Nocera  <hadess@hadess.net>

        reviewed by: Matthias Clasen <mclasen@redhat.com>

        * gtk/Makefile.am:
        * gtk/gtk.h:
        * gtk/gtk.symbols:
        * gtk/gtkscalebutton.[ch]: Add the GtkScaleButton widget,
        a button that pops up a scale when pressed

2007-05-19  Bastien Nocera  <hadess@hadess.net>

        * POTFILES.in: Add scale button to the list

2007-05-20  Bastien Nocera  <hadess@hadess.net>

        * gtk/gtk-docs.sgml:
        * gtk/gtk-sections.txt: add the GtkScaleButton widget
        to the docs

Some bits of related code were already committed though:
http://svn.gnome.org/viewcvs/gtk%2B/trunk/gtk/gtk.symbols?view=diff&r1=17867&r2=17868
http://svn.gnome.org/viewcvs/gtk%2B/trunk/docs/reference/gtk/gtk-sections.txt?r1=17746&r2=17868
Comment 49 Bastien Nocera 2007-05-19 23:36:02 UTC
And the GtkVolumeButton

2007-05-20  Bastien Nocera  <hadess@hadess.net>

        * gtk/Makefile.am:
        * gtk/gtk.h:
        * gtk/gtk.symbols:
        * gtk/gtkvolumebutton.[ch]: Add the GtkVolumeButton widget,
        a button that pops up a scale when clicked (Closes: #415775)
        * tests/Makefile.am:
        * tests/testvolumebutton.c: Add a test program for the
        volume button

2007-05-20  Bastien Nocera  <hadess@hadess.net>

        * POTFILES.in: Add volume button to the list

2007-05-20  Bastien Nocera  <hadess@hadess.net>

        * gtk/gtk-sections.txt: Add the GtkVolumeButton widget
        to the docs
Comment 50 Richard Hult 2007-05-29 08:12:28 UTC
Does this support showing the scale horizontally and vertically like the volume control applet in the gnome panel? (depending on if it sits in a vertical or horizontal panel)
Comment 51 Bastien Nocera 2007-05-29 09:47:39 UTC
No, it only supports up/down. Please file a bug if you had a particular use in mind.
Comment 52 Richard Hult 2007-06-30 08:47:58 UTC
The use case would be any use except a volume control (which is the only thing I can think of where a vertical slider makes sense). Anyway, Mitch filed a bug for that and some other limitations a while ago: bug #442042.