Re: [gtk-list] Re: memory allocation/freeing

["Mattias.Gronlund" <Mattias Gronlund sa erisoft se>]

Owen Taylor wrote:
> 
> Jay Painter <jpaint@serv.net> writes:
> 
> > > Just a near by question.
> > > The function gtk_clist_set_row_data(..., gpointer data) just stores
> > > the pointer in the clist, and if I understand you right it should
> > > copy the "unknown" object (would need the length) or it should
> > > take a reference counted object as parameter?
> > >
> > > There exists a memory leak in the fileselection dialog because this
> > > odd behavior (the clists dir_list and file_list will be populated
> > > with data from gtk_file_selection_populate but the clist-destroy
> > > functionality nor the file_selection-destroy functionality will
> > > free the data in the list.
> >
> > This problem needs to be addressed in the filesel dialog; clist should
> > never try to do anything with its data.  It might be true there needs to
> > be a easier way to run a function on all the data pointers in a clist to
> > help solve this problem.
> 
> The correct solution is probably that gtk_clist_set_row_data
> should have a _full() variant:
> 
> gtk_clist_set_row_data_full (..., gpointer data, GtkDestroyNotify *notify)
> 
> Where 'notify' is a function that will be called when 'data' is
> no longer stored.
> 
> i.e., it could be used like
> 
>   gtk_clist_set_row_data_full (...,
>                                g_strdup ("Some Text"),
>                                (GtkDestroyNotify *)g_free);
> 
> This is the standard way the GTK deals with opaque pointers. It
> could be argued that the row_data in a CList is generally
> homogeneous, so memory could be saved by only having a single
> DestroyNotify function for the whole list, but I think the
> per-row solution is more consistent with the rest of GTK, and
> slightly more flexible.
> 

OK, I have made a patch which implements gtk_clist_set_row_data_full()
and
a patch to gtkfilesel.c so that it start using the new functionality.

/Mattias

PS. The diff is against 0.99.4
--- gtkclist.h.orig	Thu Mar  5 23:24:01 1998
+++ gtkclist.h	Thu Mar  5 23:27:02 1998
@@ -190,6 +190,7 @@
   GdkColor background;
 
   gpointer data;
+  GtkDestroyNotify destroy;
 
   gint fg_set : 1;
   gint bg_set : 1;
@@ -442,6 +443,12 @@
 void gtk_clist_set_row_data (GtkCList * clist,
 			     gint row,
 			     gpointer data);
+
+/* sets a arbitrary data pointer for a given row */
+void gtk_clist_set_row_data_full (GtkCList * clist,
+			          gint row,
+			          gpointer data,
+				  GtkDestroyNotify destroy);
 
 /* returns the data set for a row */
 gpointer gtk_clist_get_row_data (GtkCList * clist,
--- gtkclist.c.orig	Thu Mar  5 23:20:17 1998
+++ gtkclist.c	Thu Mar  5 23:26:55 1998
@@ -1285,6 +1285,12 @@
   list = g_list_nth (clist->row_list, row);
   clist_row = list->data;
 
+  if (clist_row->destroy && clist_row->data) {
+    clist_row->destroy (clist_row->data);
+    clist_row->data = NULL;
+    clist_row->destroy = NULL;
+  }
+
   /* reset the row end pointer if we're removing at the
    * end of the list */
   if (row == clist->rows - 1)
@@ -1338,6 +1344,11 @@
       clist_row = list->data;
       list = list->next;
 
+      if (clist_row->destroy && clist_row->data) {
+        clist_row->destroy (clist_row->data);
+	clist_row->data=NULL;
+	clist_row->destroy=NULL;
+      }
       row_delete (clist, clist_row);
     }
   g_list_free (clist->row_list);
@@ -1370,6 +1381,15 @@
 			gint row,
 			gpointer data)
 {
+  gtk_clist_set_row_data_full (clist, row, data, NULL);
+}
+
+void
+gtk_clist_set_row_data_full (GtkCList * clist,
+			     gint row,
+			     gpointer data,
+			     GtkDestroyNotify destroy)
+{
   GtkCListRow *clist_row;
 
   g_return_if_fail (clist != NULL);
@@ -1379,6 +1399,7 @@
 
   clist_row = (g_list_nth (clist->row_list, row))->data;
   clist_row->data = data;
+  clist_row->destroy = destroy;
 
   /*
    * re-send the selected signal if data is changed/added
@@ -3475,6 +3496,7 @@
   clist_row->bg_set = FALSE;
   clist_row->state = GTK_STATE_NORMAL;
   clist_row->data = NULL;
+  clist_row->destroy = NULL;
 
   return clist_row;
 }
@@ -3487,6 +3509,12 @@
 
   for (i = 0; i < clist->columns; i++)
     cell_empty (clist, clist_row, i);
+
+  if (clist_row->destroy && clist_row->data) {
+    clist_row->destroy (clist_row->data);
+    clist_row->data=NULL;
+    clist_row->destroy=NULL;
+  }
 
   g_mem_chunk_free (clist->cell_mem_chunk, clist_row->cell);
   g_mem_chunk_free (clist->row_mem_chunk, clist_row);
--- gtkfilesel.c.orig	Thu Mar  5 23:20:46 1998
+++ gtkfilesel.c	Thu Mar  5 23:27:12 1998
@@ -1224,15 +1224,15 @@
                   strcmp (filename, "../") != 0)
 		{
 		  row = gtk_clist_append (GTK_CLIST (fs->dir_list), text);
-		  gtk_clist_set_row_data (GTK_CLIST (fs->dir_list), row, 
-					  filename);
+		  gtk_clist_set_row_data_full (GTK_CLIST (fs->dir_list), row,
+					       filename, (GtkDestroyNotify )g_free);
  		}
 	    }
           else
 	    {
 	      row = gtk_clist_append (GTK_CLIST (fs->file_list), text);
-	      gtk_clist_set_row_data (GTK_CLIST (fs->file_list), row, 
-				      filename);
+	      gtk_clist_set_row_data_full (GTK_CLIST (fs->file_list), row,
+					   filename ,(GtkDestroyNotify )g_free);
             }
 	}