gtk_label_get_text() string duping

[Tim Janik <timj gtk org>]

hi all,

earlier this year, someone on gtk-devel-list pointed out that
gtk_label_get_text() was still missing. back then, irrc, lupus,
me and probably a few others pointed out that the implementation
should basically be:

gchar*
gtk_label_get_text (GtkLabel *label)
{
  g_return_val_if_fail (label != NULL, NULL);
  g_return_val_if_fail (GTK_IS_LABEL (label), NULL);

  return label->label;
}

sometime later, havoc actually implemented that:

2000-04-26  Havoc Pennington  <hp@redhat.com>

        * gtk/gtklabel.c (gtk_label_get_text): Add this function,
        replacing the broken gtk_label_get ()

but with

  return g_strdup (label->label);

i raised that at some point on irc with owen, and had to find
out that while most of the original gtk API tries to return
values by reference (and i was following that scheme with
new functions), he was trying to establish duplication of
return values for new function implementation.

so yes, this is really owen's and my fault for not syncing
us on this issue, but i gues, while we hash out most complicated
things in quite some detail, we both assumed that the other one
would have the same common sense regarding such a supposedly simple
thing as an accessors ;)

now we're in the bad position of having half of our functions
return references and the other half (mostly newly added ones)
returning copies, all using the common <WIDGET>_get_<STUFF>()
naming scheme.

we need to do something about this (volounteers are apprechiated
once we figured the final course of action).

lemme outline first how i think the API _should_ behave:

a) a function returning a reference should be the normal getter
   interface of an object. so say oyu have an object foo, that
   holds a string gchar *bar; you provide an accessor:
   gchar* foo_get_bar (Foo *foo) { return foo->bar; }
   (we don't enforce "const" in return values for a variety
    of reasons that have been discussed before. if we did,
    this function would return const gchar*)
b) accessors that have to dynamically build up the value to
   be returned, such as a string or a list of children, should
   _not_ be named *_get_*, as they don't return references.
   instead, i prefer to name them after an action they perform,
   such as:
   gchar* foo_dup_bar (Foo *foo) {
     return g_strconcat (foo->bar1, foo->bar2, NULL);
   }
   or:
   GSList* foo_list_children (Foo *foo) {
     return g_slist_concat (g_list_copy (foo->pack_start_children),
                            g_list_copy (foo->pack_end_children));
   }

the reasons for this are:

1) we need accessors, often to hide binary incomaptibilities and
   preserve implementation encapsulation. we also need them because
   programmers using foreign widgets (i.e. non self-implemented ones)
   usually don't know what parts of the widget structure is meaningfull
   and public, and what part is not.

3) people using our API, shouldn't need to dig into the source to
   find out whether tehy have to free return values (sometimes very
   hard to figure), or lookup the function in the reference doc.
   instead, the bahviour has to be evident from naming conventions.

2) values should be returned by reference where possible, to avoid
   unneccessary burden for the callers, e.g.
     gchar *tmp_string = gtk_label_get_text (label);
     g_print ("the label says %s\n", tmp_string);
     g_free (tmp_string);
   versus
     g_print ("the label says %s\n", gtk_label_get_text (label));
   that is, accssors that do uneccesary dubplication, inhibit
   convenient nesting of functions which can be quite a saver
   for code portions that deal with lots of widget state information.

4) if we provide accessors for simple object fields, that do nothing
   but encapsulte and add copying burden (such as the current
   gtk_label_get_text()), we defeat the purpose the accessor has
   in the first place, since some programmers will prefer
     g_print ("the label says %s\n", label->label);
  over
     gchar *tmp_string = gtk_label_get_text (label);
     g_print ("the label says %s\n", tmp_string);
     g_free (tmp_string);
  to be able to nest.


what i think we need to do now is this:
1) go over the gtk source to figure what needs fixing, e.g.
   grep 'char.*\*.*_get.*(' *.h reveals quite some candidates
2) change functions newly added in HEAD to return references
3) rename old functions that return copyies and are named
   <WIDGET>_get_<STUFF>() to something like
   <WIDGET>_dup_<STUFF>() and add
   #define <WIDGET>_get_<STUFF> <WIDGET>_dup_<STUFF>
   to gtkcompat.h

that being said, it is not my point to play the hardliner and
enforce reference return values all over the place. instead, i
just want to point out that we have to get the naming right.
here's a counter example (gtk_entry_get_text() and
g_filename_from_utf8() both return newly allocated strings):

gchar*
gtk_file_selection_get_filename (GtkFileSelection *filesel)
{
  static gchar nothing[2] = "";
  static gchar something[MAXPATHLEN*2];
  char *filename;
  char *text;

  g_return_val_if_fail (filesel != NULL, nothing);
  g_return_val_if_fail (GTK_IS_FILE_SELECTION (filesel), nothing);

#ifdef G_WITH_CYGWIN
  translate_win32_path(filesel);
#endif
  text = gtk_entry_get_text (GTK_ENTRY (filesel->selection_entry));
  if (text)
    {
      filename = g_filename_from_utf8 (cmpl_completion_fullname (text, filesel->cmpl_state));
      strncpy (something, filename, sizeof (something));
      g_free (filename);
      return something;
    }

  return nothing;
}

this should instead just have been:

gchar*
gtk_file_selection_dup_filename (GtkFileSelection *filesel)
{
  gchar *filename;
  gchar *text;

  g_return_val_if_fail (filesel != NULL, NULL);
  g_return_val_if_fail (GTK_IS_FILE_SELECTION (filesel), NULL);

#ifdef G_WITH_CYGWIN
  translate_win32_path (filesel);
#endif

  text = gtk_entry_dup_text (GTK_ENTRY (filesel->selection_entry));
  if (text)
    {
      filename = g_filename_from_utf8 (cmpl_completion_fullname (text, filesel->cmpl_state));
      g_free (text);
      text = filename;
    }

  return text ? text : g_strdup (""); /* enforce non-NULL returns */
}

---
ciaoTJ