Re: Patch for bug 55767

[Owen Taylor <otaylor redhat com>]

Comments on the patch:

 * We need inline doc comments for all new functions. In most cases
   it can be really brief, something like:

    Gets the shadow type for the handle box. See gtk_handle_box_set_shadow_type().

 * All functions returning a non-strdup'ed string should be 
   G_CONST_RETURN gchar *function (...)

  +gdouble
  +gtk_adjustment_get_value (GtkAdjustment	       *adjustment)
  +{
  +  g_return_val_if_fail (adjustment != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_ADJUSTMENT (adjustment), 0);

We no longer add the adjustment != 0 check - GTK_IS_ADJUSTMENT() will catch NULL anyways.
[ Others of the same type are elsewhere in the patch - search for != NULL ]

  +gboolean
  +gtk_box_get_homogeneous (GtkBox *box)
  +{
  +  g_return_val_if_fail (GTK_IS_BOX (box), 0);

This should be ', FALSE' - even though we are just returning a junk value,
the type should match. Many other of these as well - enums should use
some value from the enumeration.

  +gboolean         gtk_cell_renderer_toggle_get_radio (GtkCellRendererToggle *toggle);

Did you forget the function for this prototype?

  +gboolean
  +gtk_container_get_reallocate_redraws (GtkContainer *container)
  +{
  +  g_return_val_if_fail (GTK_IS_CONTAINER (container), 0);
  +
  +  return container->reallocate_redraws;
  +}

I'd say omit this - gtk_container_set_reallocate_redraws() is
protected and rather obscure. 

  +GtkWidget *
  +gtk_container_get_focus_child (GtkContainer *container)
  +{
  +  g_return_val_if_fail (container != NULL, NULL);
  +  g_return_val_if_fail (GTK_IS_CONTAINER (container), NULL);
  +
  +  return container->focus_child;
  +}

Omit this - container->focus_child is not publically interesting 
and gtk_container_set_focus_child() is really private/protected.

  +GList *
  +gtk_container_get_focus_chain (GtkContainer *container)
  +{
  +  g_return_val_if_fail (container != NULL, NULL);
  +  g_return_val_if_fail (GTK_IS_CONTAINER (container), NULL);
  +
  +  if (container->has_focus_chain)
  +    {
  +      GList *chain;
  +
  +      chain = get_focus_chain (container);
  +
  +      return chain;
  +    }

How about simply:

 if (container->has_focus_chain)
   return get_focus_chain (container);

===

  +PangoAttrList *
  +gtk_label_get_attributes (GtkLabel *label)
  +{
  +  g_return_val_if_fail (GTK_IS_LABEL (label), NULL);
  +
  +  return label->attrs;
  +}

See my last mail - I think some more radical changes are needed 
in attribute handling. I think it would be best to implement
plan 1) described there:

===
 1) ->attrs is only affected by set/get_attributes, ::attributes.
    The effective attributes are 

     pango_layout_get_attributes (gtk_label_get_layout (label));

    Caching layout->effective_attrs also possible but doesn't 
    affect the API. This is cleanest, but maybe a little
    less efficient. (probably doesn't matter.)
==

To make this easy to do, why don't we, for now, simply to add
an extra field to the GtkLabel structure, "effective_attrs".

  +GtkSubmenuPlacement
  +gtk_menu_item_get_placement (GtkMenuItem *menu_item)
  +{
  +  g_return_val_if_fail (menu_item != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_MENU_ITEM (menu_item), 0);
  +
  +  return menu_item->submenu_placement;
   }

I'd consider gtk_menu_item_set_placement() private and not add this getter().

  +gboolean
  +gtk_notebook_get_homogeneous_tabs (GtkNotebook *notebook)
  +{
  +  g_return_val_if_fail (notebook != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_NOTEBOOK (notebook), 0);
  +
  +  return notebook->homogeneous;
  +}

  +guint
  +gtk_notebook_get_tab_border (GtkNotebook *notebook)
  +{
  +  g_return_val_if_fail (notebook != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_NOTEBOOK (notebook), 0);
  +
  +  return notebook->tab_hborder;
  +}

  +guint
  +gtk_notebook_get_tab_hborder (GtkNotebook *notebook)
  +{
  +  g_return_val_if_fail (notebook != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_NOTEBOOK (notebook), 0);
  +
  +  return notebook->tab_hborder;
  +}
  +
  +guint
  +gtk_notebook_get_tab_vborder (GtkNotebook *notebook)
  +{
  +  g_return_val_if_fail (notebook != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_NOTEBOOK (notebook), 0);
  +
  +  return notebook->tab_vborder;
  +}

Please omit these, and if you don't mind, put:

 #ifndef GTK_DISABLE_DEPRECATED
 #endif /* GTK_DISABLE_DEPRECATED */

Around the corresponding getters.

  +GtkMetricType
  +gtk_ruler_get_metric (GtkRuler *ruler)
  +{
  +  int i;
  +
  +  g_return_val_if_fail (ruler != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_RULER (ruler), 0);
  +
  +  for (i = 0; i < G_N_ELEMENTS (ruler_metrics); i++)
  +    if (ruler->metric == &ruler_metrics[i])
  +      return i;
  +  
  +  return 0;
  +}

I'd add a g_assert_not_reached() before the return here.

   void	   gtk_table_set_row_spacings (GtkTable	       *table,
                                         guint		spacing);
  +guint      gtk_table_get_row_spacings (GtkTable        *table);
   void	   gtk_table_set_col_spacings (GtkTable	       *table,
                                         guint		spacing);
  +guint      gtk_table_get_col_spacings (GtkTable        *table);

These are somewhat problematical because:

 gtk_table_set_row_spacing (table, 0, 5);
 gtk_table_set_row_spacings (table, 0, 10);

Is not the same as:

 gtk_table_set_row_spacings (table, 0, 10);
 gtk_table_set_row_spacing (table, 0, 5);

I think I'd name the getter something different, like
"gtk_table_get_default_row_spacing(table)", to make it clear it isn't
an inverse of gtk_table_set_row_spacings () - it doesn't "get the
spacing of all rows in the table".

  +gint
  +gtk_text_view_get_border_window_size (GtkTextView       *text_view,
  +				      GtkTextWindowType  type)
  +{
  +  g_return_val_if_fail (GTK_IS_TEXT_VIEW (text_view), 0);
  +  g_return_val_if_fail (type != GTK_TEXT_WINDOW_WIDGET, 0);
  +  g_return_val_if_fail (type != GTK_TEXT_WINDOW_TEXT, 0);
  +  
  +  switch (type)
  +    {
  +    case GTK_TEXT_WINDOW_LEFT:
  +      return text_view->left_window->requisition.width;
  +      
  +    case GTK_TEXT_WINDOW_RIGHT:
  +      return text_view->right_window->requisition.width;

  +      
  +    case GTK_TEXT_WINDOW_TOP:
  +      return text_view->top_window->requisition.width;
  +
  +    case GTK_TEXT_WINDOW_BOTTOM:
  +      return text_view->bottom_window->requisition.width;

These need to be height not width.

  +      
  +    default:
  +      g_warning ("Can't get size of center or widget or private GtkTextWindowType in %s\n", G_STRLOC);

This warning is hard to understand. Havoc suggests:

 "can only set size of left/right/top/bottom border windows with
  gtk_text_view_set_border_window_size"

Here and in gtk_text_view_set_border_window_size().

  +GtkStateType
  +gtk_widget_get_state (GtkWidget *widget)
  +{
  +  g_return_val_if_fail (widget != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_WIDGET (widget), 0);
  +
  +  return GTK_WIDGET_STATE (widget);
  +}
  +gboolean
  +gtk_widget_get_app_paintable (GtkWidget *widget)
  +{
  +  g_return_val_if_fail (widget != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_WIDGET (widget), 0);
  +
  +  return GTK_WIDGET_APP_PAINTABLE (widget);
  +}
  +gtk_widget_get_double_buffered (GtkWidget *widget)
  +{
  +  g_return_val_if_fail (widget != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_WIDGET (widget), 0);
  +
  +  return GTK_WIDGET_DOUBLE_BUFFERED (widget);
  +gboolean
  +gtk_widget_get_sensitive (GtkWidget *widget)
  +{
  +  g_return_val_if_fail (widget != NULL, 0);
  +  g_return_val_if_fail (GTK_IS_WIDGET (widget), 0);
  +
  +  return GTK_WIDGET_SENSITIVE (widget);
  +}

These macros are all public. Let's omit the getters.

  +void
  +gtk_widget_get_uposition (GtkWidget *widget,
  +			  gint      *x,
  +			  gint      *y)
  +{
  +  GtkWidgetAuxInfo *aux_info;
  +
  +  g_return_if_fail (widget != NULL);
  +  g_return_if_fail (GTK_IS_WIDGET (widget));
  +
  +  aux_info = _gtk_widget_get_aux_info (widget, TRUE);
  +
  +  if (x)
  +    *x = aux_info->x;
  +
  +  if (y)
  +    *y = aux_info->y;
  +}

You need to take account x_set and y_set here. Probably best to just
add 2 more gboolean parameters.

  +void
  +gtk_widget_get_usize (GtkWidget *widget,
  +		      gint      *width,
  +		      gint      *height)
  +{
  +  GtkWidgetAuxInfo *aux_info;
  +
  +  g_return_if_fail (widget != NULL);
  +  g_return_if_fail (GTK_IS_WIDGET (widget));
  +
  +  aux_info = _gtk_widget_get_aux_info (widget, TRUE);
  +
  +  if (width)
  +    *width = aux_info->width;
  +
  +  if (height)
  +    *height = aux_info->height;
   }

I'd probably do _get_aux_info (widget, FALSE), then 
*width = aux_info ? aux_info->width : -1;

Regards,
                                        Owen