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 710471 - Make gtk_scrolled_window_remove() smart
Make gtk_scrolled_window_remove() smart
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkScrolledWindow
3.10.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-10-18 17:14 UTC by Kjell Ahlstedt
Modified: 2016-06-08 08:35 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
patch: Make gtk_scrolled_window_remove() smart (3.95 KB, patch)
2013-10-18 17:24 UTC, Kjell Ahlstedt 		none Details | Review
A test case (2.65 KB, text/plain)
2013-10-18 17:35 UTC, Kjell Ahlstedt 		  Details
patch: Make gtk_scrolled_window_remove() smart (4.11 KB, patch)
2016-06-07 12:36 UTC, Kjell Ahlstedt 		committed Details | Review
An updated test case (3.15 KB, text/plain)
2016-06-07 12:37 UTC, Kjell Ahlstedt 		  Details

Description Kjell Ahlstedt 2013-10-18 17:14:37 UTC 
Now that gtk_scrolled_window_add() is smart enough to automatically add a
GtkViewport if necessary (bug 693015), it would be appropriate to make
gtk_scrolled_window_remove() as smart. I'll attach a patch to show what I
mean.
Comment 1 Kjell Ahlstedt 2013-10-18 17:24:22 UTC 
Created attachment 257654 [details] [review]
patch: Make gtk_scrolled_window_remove() smart

This patch makes it possible to execute code such as

  g_object_ref_sink(child_widget);
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_remove(GTK_CONTAINER(scrolled_window),
                       gtk_bin_get_child(GTK_BIN(scrolled_window)));
  /* ...... */
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);

even if child_widget does not implement the GtkScrollable interface.
gtk_container_remove() first removes child_widget from the viewport,
then the viewport from scrolled_window.

It would perhaps have been nicer to accept code such as

  g_object_ref_sink(child_widget);
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_remove(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);

but that would require a complication of a test in gtk_container_remove(),
which is probably not wanted. The proposed patch also better suits gtkmm.
Comment 2 Kjell Ahlstedt 2013-10-18 17:35:22 UTC 
Created attachment 257656 [details]
A test case
Comment 3 Matthias Clasen 2016-06-06 02:35:43 UTC 
(In reply to Kjell Ahlstedt from comment #1)

> It would perhaps have been nicer to accept code such as

Yes, I think that would be much nicer.
Comment 4 Matthias Clasen 2016-06-06 02:36:33 UTC 
Review of attachment 257654 [details] [review]:

.
Comment 5 Kjell Ahlstedt 2016-06-07 12:36:52 UTC 
Created attachment 329266 [details] [review]
patch: Make gtk_scrolled_window_remove() smart

The test in gtk_container_remove() that worried me in comment 1 was removed by
https://git.gnome.org/browse/gtk+/commit/?id=a3805333361fee37a3b1a974cfa6452a85169f08

With this patch both

  g_object_ref_sink(child_widget);
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_remove(GTK_CONTAINER(scrolled_window),
                       gtk_bin_get_child(GTK_BIN(scrolled_window)));
  /* ...... */
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);

and

  g_object_ref_sink(child_widget);
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_remove(GTK_CONTAINER(scrolled_window), child_widget);
  /* ...... */
  gtk_container_add(GTK_CONTAINER(scrolled_window), child_widget);

can be used. The result is the same in both cases.
Comment 6 Kjell Ahlstedt 2016-06-07 12:37:52 UTC 
Created attachment 329267 [details]
An updated test case
Comment 7 Matthias Clasen 2016-06-07 20:32:30 UTC 
Review of attachment 329266 [details] [review]:

alright then!