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 705918 - Add gtk_button_new_from_icon_name
Add gtk_button_new_from_icon_name
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-08-13 15:39 UTC by Yosef Or Boczko
Modified: 2013-08-14 07:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add gtk_button_new_from_icon_name (2.96 KB, patch)
2013-08-13 15:49 UTC, Yosef Or Boczko
reviewed Details | Review
example of valgtk_widget_set_valign with and without GTK_ALIGN_CENTER (2.82 KB, text/x-csrc)
2013-08-13 17:49 UTC, Yosef Or Boczko
  Details
Add gtk_button_new_from_icon_name (3.31 KB, patch)
2013-08-13 18:06 UTC, Yosef Or Boczko
needs-work Details | Review
Add gtk_button_new_from_icon_name (2.82 KB, patch)
2013-08-13 21:21 UTC, Yosef Or Boczko
reviewed Details | Review
Add gtk_button_new_from_icon_name (2.83 KB, patch)
2013-08-14 07:51 UTC, Yosef Or Boczko
committed Details | Review

Description Yosef Or Boczko 2013-08-13 15:39:40 UTC
In many applications in GNOME, need to create button with icon name.
For this, always need to create button, create image ans call to
gtk_button_set_image with the image.

This convenience function for this.
Comment 1 Yosef Or Boczko 2013-08-13 15:49:05 UTC
Created attachment 251512 [details] [review]
Add gtk_button_new_from_icon_name
Comment 2 Emmanuele Bassi (:ebassi) 2013-08-13 17:23:49 UTC
Review of attachment 251512 [details] [review]:

thanks for the patch!

naming issue that I want to raise: should it be new_with_icon_name() or new_from_icon_name()?

::: gtk/gtkbutton.c
@@ +1307,3 @@
+ * displayed instead.  If the current icon theme is changed, the icon
+ * will be updated appropriately.
+ * 

an additional note like this would be helpful:

  This function is a convenience wrapper around gtk_button_new() and
  gtk_button_set_image().

@@ +1309,3 @@
+ * 
+ * Returns: a new #GtkButton displaying the themed icon
+ **/

missing "Since: 3.10" annotation
Comment 3 Matthias Clasen 2013-08-13 17:28:00 UTC
Review of attachment 251512 [details] [review]:

Not sure this is entirely right.

In other places where image-only buttons are constructed, we do gtk_style_context_add_class (context, "image-button");

The documentation should be clear about the fact that this api is meant to construct image-only buttons.

::: gtk/gtkbutton.c
@@ +1309,3 @@
+ * 
+ * Returns: a new #GtkButton displaying the themed icon
+ **/

Since: 3.10
Comment 4 Yosef Or Boczko 2013-08-13 17:30:30 UTC
(In reply to comment #2)
> Review of attachment 251512 [details] [review]:
> 
> thanks for the patch!
> 
> naming issue that I want to raise: should it be new_with_icon_name() or
> new_from_icon_name()?

I think should it be new_from_icon_name () for consistency
with gtk_image_new_from_icon_name ().

> 
> ::: gtk/gtkbutton.c
> @@ +1307,3 @@
> + * displayed instead.  If the current icon theme is changed, the icon
> + * will be updated appropriately.
> + * 
> 
> an additional note like this would be helpful:
> 
>   This function is a convenience wrapper around gtk_button_new() and
>   gtk_button_set_image().
> 

OK.

> @@ +1309,3 @@
> + * 
> + * Returns: a new #GtkButton displaying the themed icon
> + **/
> 
> missing "Since: 3.10" annotation

OK.
Comment 5 Yosef Or Boczko 2013-08-13 17:32:28 UTC
(In reply to comment #3)
> Review of attachment 251512 [details] [review]:
> 
> Not sure this is entirely right.
> 
> In other places where image-only buttons are constructed, we do
> gtk_style_context_add_class (context, "image-button");
> 
> The documentation should be clear about the fact that this api is meant to
> construct image-only buttons.
> 

So should also to add gtk_widget_set_valign (button, GTK_ALIGN_CENTER), no?

> ::: gtk/gtkbutton.c
> @@ +1309,3 @@
> + * 
> + * Returns: a new #GtkButton displaying the themed icon
> + **/
> 
> Since: 3.10

OK.
Comment 6 Matthias Clasen 2013-08-13 17:44:09 UTC
(In reply to comment #5)
> 
> So should also to add gtk_widget_set_valign (button, GTK_ALIGN_CENTER), no?
> 

That would align the button inside its parent. I think GtkButton already takes care of centering the image if there is no label.
Comment 7 Yosef Or Boczko 2013-08-13 17:49:13 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > 
> > So should also to add gtk_widget_set_valign (button, GTK_ALIGN_CENTER), no?
> > 
> 
> That would align the button inside its parent. I think GtkButton already takes
> care of centering the image if there is no label.

Not right.
I fixed ot the look in some applications of GNOME,
and always had need gtk_widget_set_valign.

See for example the program an attached.
Comment 8 Yosef Or Boczko 2013-08-13 17:49:46 UTC
Created attachment 251530 [details]
example of valgtk_widget_set_valign with and without GTK_ALIGN_CENTER
Comment 9 Yosef Or Boczko 2013-08-13 18:06:34 UTC
Created attachment 251531 [details] [review]
Add gtk_button_new_from_icon_name
Comment 10 Matthias Clasen 2013-08-13 21:13:29 UTC
Review of attachment 251531 [details] [review]:

::: gtk/gtkbutton.c
@@ +1303,3 @@
+ * @size: (type int): an icon size
+ *
+ * Creates a new #GtkButton containing the an icon from the current icon theme.

"the an"

@@ +1310,3 @@
+ * 
+ * This function is a convenience wrapper around gtk_button_new () and
+ * gtk_button_set_image ().

No space between function name and () here, to make linkification work.

@@ +1327,3 @@
+  button =  g_object_new (GTK_TYPE_BUTTON,
+			  "image", image,
+			  "valign", GTK_ALIGN_CENTER,

The valign doesn't belong here and needs to go.

@@ +1348,3 @@
  * Returns: a new #GtkButton
  *
+ * Deprecated: 3.10: Use gtk_button_new_from_icon_name() instead.

Depending on stock id, new_from_stock may create a button with icon, with label, or both. In most cases, the label is wanted, so the deprecation notice should stay unchanged, or point to both functions.
Comment 11 Yosef Or Boczko 2013-08-13 21:21:46 UTC
Created attachment 251549 [details] [review]
Add gtk_button_new_from_icon_name
Comment 12 Matthias Clasen 2013-08-14 04:43:08 UTC
Review of attachment 251549 [details] [review]:

Looks ok, otherwise. 
Please commit with that correction

::: gtk/gtkbutton.h
@@ +81,3 @@
 GDK_AVAILABLE_IN_ALL
 GtkWidget*     gtk_button_new_with_label    (const gchar    *label);
+GDK_DEPRECATED_IN_3_10

You mean GDK_AVAILABLE_IN_3_10 here, I assume
Comment 13 Yosef Or Boczko 2013-08-14 07:51:17 UTC
Created attachment 251573 [details] [review]
Add gtk_button_new_from_icon_name
Comment 14 Yosef Or Boczko 2013-08-14 07:52:40 UTC
Review of attachment 251573 [details] [review]:

pushed as 2f77a61e61d6eb1d99f74b262c63059726a2ec08 - 'Add gtk_button_new_from_icon_name'
and fix a typo in 89f701ab2ef2cfd6f6db15147006415c9743cefa - 'oopx - fix typo' (would need to be 'oops'...).