GNOME Bugzilla – Bug 539944
Add GtkScaleButton API so struct fields can be marked as private
Last modified: 2015-11-30 13:58:42 UTC
2008-06-20 Johan Dahlin <jdahlin@async.com.br> * gtk/gtklayout.c (enum): Remove left-over unused property enum. * gtk/gtk.symbols: * gtk/gtkscalebutton.c (gtk_scale_button_get_plus_button), (gtk_scale_button_get_minus_button): * gtk/gtkscalebutton.h: Add missing accessor for sealed fields GtkScaleButton->plus_button and minus_button. This commit introduced accessors for the plus and minus buttons of GtkScaleButton. However the public API doesn't mention these fields and I do not see a reasonable use case. I suggest these accessors be removed again. If anyone actually has a useful task he thinks he cannot accomplish without these, we can talk about that and see if it makes sense. But then again it might be a better idea to improve the API of GtkScaleButton in another direction.
Either we do: 1) Remove the accessors and mark the fields as /* <private> */ 2) Keep them and add the fields to the documentation I checked on google's codesearch and it seems that they are not used by anything outside of gtk+.
It's used to get to the top-level popup window itself. Eg. in Totem: static GtkWidget * totem_fullscreen_get_volume_popup (TotemFullscreen *fs) { return gtk_widget_get_toplevel (GTK_SCALE_BUTTON (fs->volume)->plus_button); } I'd be happy with making the popup itself available through an accessor. Oh, and it's also used by descendants like the GtkVolumeButton to set the labels and accessible names. I could also see some other descendants hiding those buttons.
Reverted on trunk, changing subject.
We need an accessor for the top-level popup, as well as accessors for the plus and minus buttons. The plus and minus buttons, in descendants of this class, can then: - be hidden - have their labels changed - can have their accessible names changed/set
(In reply to comment #4) > We need an accessor for the top-level popup, as well as accessors for the plus > and minus buttons. > > The plus and minus buttons, in descendants of this class, can then: > - be hidden > - have their labels changed > - can have their accessible names changed/set I'm not sure if that is the right approach. Exposing widgets means that the scale button cannot easily be modified later if new use cases come up. Why do you need to access the raw popup? What about a function that instead displays the popup on the screen? The request to be able to hide the plus and minus buttons came already up in bug 442042, or even move them to the volume button. I suggest that if it is desirable to have custom buttons, the widget should have accessors like _set_plus_button and _set_minus_button instead so subclasses don't need to fiddle with implementation details.
I don't know what the right approach is, but i just committed this to GIMP: 2008-06-26 Michael Natterer <mitch@gimp.org> * app/widgets/gimpscalebutton.c (gimp_scale_button_init): hide the popup's plus and minus buttons, they are completely pointless. Which will stop working when the members are sealed. Hiding the buttons is completely reasonable because they are indeed completely pointless.
(In reply to comment #5) <snip> > I'm not sure if that is the right approach. Exposing widgets means that the > scale button cannot easily be modified later if new use cases come up. We're talking about widgets that can be > Why do you need to access the raw popup? What about a function that instead > displays the popup on the screen? It's used to check whether the popup is visible. When the popup is visible, we don't hide the fullscreen controls in Totem. > The request to be able to hide the plus and minus buttons came already up in > bug 442042, or even move them to the volume button. I suggest that if it is > desirable to have custom buttons, the widget should have accessors like > _set_plus_button and _set_minus_button instead so subclasses don't need to > fiddle with implementation details. That sounds like over-engineering for something as simple as that. (In reply to comment #6) > I don't know what the right approach is, but i just committed this > to GIMP: > > 2008-06-26 Michael Natterer <mitch@gimp.org> > > * app/widgets/gimpscalebutton.c (gimp_scale_button_init): hide the > popup's plus and minus buttons, they are completely pointless. > > Which will stop working when the members are sealed. Hiding the buttons > is completely reasonable because they are indeed completely pointless. I hope you mean they're not useful for your use case. They're still useful for a11y.
(In reply to comment #7) > (In reply to comment #5) > <snip> > > I'm not sure if that is the right approach. Exposing widgets means that the > > scale button cannot easily be modified later if new use cases come up. > > We're talking about widgets that can be After thinking about this some more I do see why the buttons should always be present (a11y). I still don't like the idea that subclasses are supposed to modify the buttons directly, but I don't see a good way around either except for adding properties for everything that should be manipulated. > > Why do you need to access the raw popup? What about a function that instead > > displays the popup on the screen? > > It's used to check whether the popup is visible. When the popup is visible, we > don't hide the fullscreen controls in Totem. What about an accessors that actually returns the popup, if available? > > The request to be able to hide the plus and minus buttons came already up in > > bug 442042, or even move them to the volume button. I suggest that if it is > > desirable to have custom buttons, the widget should have accessors like > > _set_plus_button and _set_minus_button instead so subclasses don't need to > > fiddle with implementation details. > > That sounds like over-engineering for something as simple as that. I don't think setters for the buttons are overengineering. The subclass could just provide their own buttons with label, image, name and the scale button connects the events.
(In reply to comment #8) <snip> > > > Why do you need to access the raw popup? What about a function that instead > > > displays the popup on the screen? > > > > It's used to check whether the popup is visible. When the popup is visible, we > > don't hide the fullscreen controls in Totem. > > What about an accessors that actually returns the popup, if available? Do you read the comments? In comment 4, I said: > We need an accessor for the top-level popup > > > The request to be able to hide the plus and minus buttons came already up in > > > bug 442042, or even move them to the volume button. I suggest that if it is > > > desirable to have custom buttons, the widget should have accessors like > > > _set_plus_button and _set_minus_button instead so subclasses don't need to > > > fiddle with implementation details. > > > > That sounds like over-engineering for something as simple as that. > > I don't think setters for the buttons are overengineering. The subclass could > just provide their own buttons with label, image, name and the scale button > connects the events. It would require quite a lot of changes in the widget, for an API that's only marginally better.
Created attachment 113515 [details] [review] Implement gtk_scale_button_get_popup (In reply to comment #9) > (In reply to comment #8) > <snip> > > What about an accessor that actually returns the popup, if available? > > Do you read the comments? I'm sorry, due to your list style formatting I missed that part when checking again if this was already suggested. Do you accept an excuse in the form of a patch? ;)
Looks alright to me.
Sure, looks alright. But it doesn't solve the 'access to the button' problem for Mitch and for GtkVolumeButton.
* gtk/gtk.symbols: * gtk/gtkscalebutton.[hc] (gtk_scale_button_get_popup): Add an accessor for the popup. Patch by Christian Dywan Leaving open for dealing with plus and minus somehow.
Created attachment 113960 [details] [review] Implement _get_plus_button and _get_minus_button Following the discussion in this bug, it seems the best idea is to add accessors for the button widgets afterall. This is a patch that adds them.
Fixed in SVN: 2008-07-04 Michael Natterer <mitch@imendio.com> Bug 539944 – Add GtkScaleButton API so struct fields can be marked as private * gtk/gtk.symbols * gtk/gtkscalebutton.[ch]: add gtk_scale_button_get_plus_button() and _get_minus_button(). Patch by Christian Dywan.
Reopening so we can improve the documentation. Do gtk_scale_button_get_plus_button() and gtk_scale_button_get_minus_button() always return GtkButton* instances, or can they really be any GtkWidget*? If they are always GtkButton* then that return type should be used. If not then there should be some explanation of that so it doesn't just look like the return type is wrong.
(In reply to comment #16) > Reopening so we can improve the documentation. > > Do gtk_scale_button_get_plus_button() and gtk_scale_button_get_minus_button() > always return GtkButton* instances, or can they really be any GtkWidget*? If > they are always GtkButton* then that return type should be used. If not then > there should be some explanation of that so it doesn't just look like the > return type is wrong. In fact the docs for the two accessors lack a 'Returns' line, I agree that should be fixed. The actual return type must of course remain GtkWidget*. We might want to document that the widgets are always GtkButton* widgets, which would be unusual but essentially helpful in the mentioned use cases.
> The actual return type must of course remain GtkWidget*. Why?
Other get_*() functions that return widgets really do seem to return GtkWidget*. I guess there just aren't so many that I have noticed. How ugly. This does indeed make it very important to document the actual type.
(In reply to comment #16) > Reopening so we can improve the documentation. Please also add a documentation hint like: The icons are "named icons" which can be registered via gtk_icon_theme_append_search_path() Took me a ridiculous inappropriate amount of time to figure that out, following the "menus_and_toolbars" example from your book first..
Created attachment 142464 [details] [review] Say that _get_plus/minus_button returns a #GtkButton
the docs look ok to me now
No, it still needs to say that it's a GtkButton, I think: http://developer.gnome.org/gtk3/unstable/GtkScaleButton.html#gtk-scale-button-get-plus-button The patch in comment #21 would be fine. May I push it?
Review of attachment 142464 [details] [review]: I think it would be nicer to add the type information without losing the existing docs
Created attachment 316046 [details] [review] Mention that _get_minus_button/get_plus_button return a GtkButton Maybe we can close this now?
Review of attachment 316046 [details] [review]: sure
Ok, pushed. Thanks.