GNOME Bugzilla – Bug 685739
Can't remove every widget correctly from a Gtk::ScrolledWindow
Last modified: 2016-06-26 11:15:31 UTC
If you add some widget without native ability to scroll to a ScrolledWindow, it adds a Viewport to the ScrolledWindow in which it stores the widget. If you now use the remove() function on that ScrolledWindow, it removes the viewport from the ScrolledWindow, so you don't have any access to the Viewport anymore and can't re-add your widget to the ScrolledWindow. I already filed a bug in launchpad: https://bugs.launchpad.net/ubuntu/+source/gtkmm3.0/+bug/1055744 It also seems that this bug persists since 2006 or longer (probably since there is the class ScrolledWindow): https://bugs.launchpad.net/ubuntu/+source/gtkmm2.4/+bug/30345 The discussion about this started in the gtkmm mailing list: https://mail.gnome.org/archives/gtkmm-list/2012-October/msg00002.html I also added an example code as attachment.
Created attachment 226059 [details] Code example
Created attachment 227710 [details] [review] patch: ScrolledWindow: Add remove() that removes both added widget and Viewport This patch fixes the bug without breaking ABI. But it does break API, I think! Consider code like this, that works at present: Gtk::Widget* button_in_scrolled_window = &m_Button1; while (something_to_do) { m_ScrolledWindow.add(*button_in_scrolled_window); button_in_scrolled_window = m_ScrolledWindow.get_child(); do_something1(); m_ScrolledWindow.remove(); m_ScrolledWindow.add(m_TextView); do_something2(); m_ScrolledWindow.remove(); } With the patch applied, this code would break, because the first m_ScrolledWindow.remove() would delete the Viewport that button_in_scrolled_window points to. This patch can be added at the next API/ABI break, preferably with the further fixes indicated by TODO comments in it.
Created attachment 227711 [details] [review] patch: ScrolledWindow::add(): Check if the widget has a viewport as parent. This patch is better for gtkmm 3. As far as I can see, it won't break any existing and working code.
Created attachment 227712 [details] [review] patch: Bin, Container::remove(): Restore the floating ref of a managed widget. I think this patch is also needed. It's not closely related to the ScrolledWindow bug. Perhaps it doesn't fit in this bug, but it is a problem that will occur if a managed widget is repeatedly added and removed from a container. Its ref count will be increased by 1 each time it's added, except the first time, when the ref count is floating. The floating status of the ref should be restored when the widget is removed from the container.
Jonas, have you got an opinion about my patches? From your posts on the gtkmm-list it's not quite clear if you planned to suggest a solution yourself.
No I didn't plan to suggest an own solution. I understand the logic of your patches, but I couldn't do that myself. I just don't want read all that tutorials to start fixing bugs here. And I never made a git patch before. I'm just fine writing SMO, my first bigger project, first GUI program with sence and first open source project :D Now back to topic: I think your first solution would not remove a Viewport that's manually added, if that one doesn't have a child, but it's generally the way I would do it too. But the second one is not really good because it would not work if you'd place a widget onto something else than a ScrolledWindow after removing it from one. The first solution is quite good, just look for what I wrote about a manually added Viewport ;)
Created attachment 228579 [details] [review] patch: ScrolledWindow: Add remove_with_viewport(). (In reply to comment #6) > I think your first solution would not remove a Viewport that's manually > added, if that one doesn't have a child, Yes, it would. "gtk_container_remove(Container::gobj(), child)" removes the ScrolledWindow's child whatever that child is. But the child of a manually added Viewport is not removed. I don't think it should be. I agree with you that the first solution (in comment 2) is the best one, but I don't want to add it in gtkmm3. As I explain in comment 2, it can break existing working code. The addition of a Gtk::Viewport is documented in the description of ScrolledWindow::add() and Bin::remove(), and it's possible that some users of gtkmm have taken that into account in their code. The patch in this comment adds a non-virtual method to ScrolledWindow. It breaks neither ABI nor API. It can be added in gtkmm3. It's an alternative to the solution in comment 3. What do you think? If you can suggest a better name for the new method, please do.
I don't see the problem in your example code but I understand how it could break existing code. So I think your latest patch makes sense.
Created attachment 228865 [details] Test case for ref counts of managed widgets I've pushed a patch similar to the one in comment 4. It fixes a ref count bug that can cause a memory leak. The attached test case shows that without the pushed patch the ref count will be wrong after some add() and remove(). What can I do with the patch in comment 7? It adds a method in ScrolledWindow. Is gtkmm 3.8 the next version where it's allowed to add new API? There is no gtkmm-3-6 branch in the git master. Does that mean that the master branch shall only be used for updates to gtkmm 3.6.1 at present? I once again forgot to include @newin. I'll attach an updated version of the patch where I've fixed that, and where I've also fixed the ref count bug in the new ScrolledWindow::remove_with_viewport() method.
(In reply to comment #9) > Is gtkmm 3.8 the next version where it's allowed to add new API? Yes. > There is no gtkmm-3-6 branch in the git master. Does that mean that the master > branch shall only be used for updates to gtkmm 3.6.1 at present? Yes, but feel free to create the branch yourself. Thanks.
I've created the gtkmm-3-6 branch, and added a better version of ScrolledWindow::remove_with_viewport() to the master branch (better than the one in comment 7). This bug is now fixed. As mentioned in comment 2 and comment 7, a better solution can be applied at the next ABI/API break. That's mentioned in a TODO comment in scrolledwindow.hg.
Does this need any update for this change that I just made?: http://git.gnome.org/browse/gtkmm/commit/?id=b919f1e7c7297b40027132d5387054b04f6723bb
No, the remove methods don't need an update. The local function gtk_scrolled_window_remove() has not been changed.
Now that it's GTK+ that adds the extra GtkViewport, maybe it should be GTK+ that has this extra function to remove it.
I have filed the gtk+ bug 710471.
Created attachment 329813 [details] [review] patch: Gtk::ScrolledWindow: Deprecate remove_with_viewport() The gtk+ bug 710471 has now been fixed. An unplanned side-effect of that fix is that the code snippet in comment 2 now crashes. Let's hope that no one has written such unintuitive code. I once added remove_with_viewport() because I didn't want to break code such as the one in comment 2. Now it's broken anyway by a fix in gtk+. Then there is no reason to keep remove_with_viewport(). The call to gtk_container_remove() in remove() destroys an automatically created and added GtkViewport.
Review of attachment 227712 [details] [review]: A patch very similar to this one was committed long ago.
Review of attachment 228579 [details] [review]: A patch very similar to this one was committed long ago.
Created attachment 330397 [details] Updated test case for ref counts of managed widgets This version tests more ways of removing a widget from a ScrolledWindow.
Closing this bug once again. I have committed the patch in comment 16.