Tree & delete node (as button), 1.3.6, linux

[basiliscos]

Hi,

I have to following program:

#include <stdio.h>
#include <FL/Fl.H>
#include <FL/Fl_Double_Window.H>
#include <FL/Fl_Tree.H>
#include <FL/Fl_Button.H>

int main(int argc, char *argv[]) {
  Fl::scheme("gtk+");
  Fl_Double_Window *win = new Fl_Double_Window(250, 400, "Simple Tree");
  win->begin();
  {
    Fl_Tree *tree = new Fl_Tree(10, 10, win->w()-20, win->h()-20);
    tree->showroot(0);                          // don't show root of tree

    // Add some items
    tree->begin();
    auto item = tree->add("add");
    auto add = new Fl_Button(0,0, 40, 0, "add");
    add->callback([](auto*, void* data) {
        auto tree = reinterpret_cast<Fl_Tree*>(data);
        tree->begin();
        static int i = 0;
        char buff[32];
        sprintf(buff, "del #%d", ++i);
        auto item = tree->add(buff); 
        auto d = new Fl_Button(0, 0, 40, 0, "del");
        item->widget(d);
        d->callback([](auto* w, void* data){
            auto it = reinterpret_cast<Fl_Tree_Item*>(data);
            auto t = it->tree();
            auto pos = it->parent()->find_child(it);
            fprintf(stderr, "widget = %p, pos = %d\n", (void*)w, pos);
            if (pos >= 0) {
                t->remove(it);
                t->redraw();
            }
        }, item);
        tree->end();
        tree->redraw();
    }, tree);
    item->widget(add);
    tree->end();
  }
  win->end();
  win->resizable(win);
  win->show(argc, argv);
  return(Fl::run());
}

When I add a few several nodes, and then quicly delete each one (by pressing on "del" button), I notice that callback is invoked twice for the same node which leads to segfault.

As a workaround, I have to checkk whether the current node can be found in it's parent, and only then delete it. This works, but I found it a bit clumpsy. I think it would be nice to add some internal "removed" flag into node itself, and when it is enablend then the call tree->remove(node) should just ignore the operation.

What do you think?

[Albrecht-S]

Your program fails because it accesses deleted data. Try to run it with valgrind and you'll see...

A much shorter test to see the effect goes like this (no need to click the button quickly):

• start the program
• add exactly one item
• hover the mouse over the del button
• click once, don't move the mouse after clicking
• click again in the "empty space" where the button has been before

Valgrind shows:

==525456==    at 0x11CBCE: Fl_Tree_Item::tree() (Fl_Tree_Item.H:283)
==525456==    by 0x11C671: _ZZZ4mainENKUlPT_PvE_clI9Fl_WidgetEEDaS0_S1_ENKUlS0_S1_E_clIS4_EEDaS0_S1_ (in /tmp/x)
==525456==    by 0x11CA97: _ZZZ4mainENKUlPT_PvE_clI9Fl_WidgetEEDaS0_S1_ENUlS0_S1_E_4_FUNIS4_EEDTcldtdeLKPKS5_0EonclIS_EscOS0_fp_scOS1_fp0_EES0_S1_ (in /tmp/x)
==525456==    by 0x1415F9: Fl_Widget::do_callback(Fl_Widget*, void*) (Fl_Widget.cxx:328)
==525456==    by 0x120548: Fl_Widget::do_callback() (Fl_Widget.H:881)
==525456==    by 0x120B6C: Fl_Button::handle(int) (Fl_Button.cxx:123)
==525456==    by 0x11E175: send_event(int, Fl_Widget*, Fl_Window*) (Fl.cxx:1044)
==525456==    by 0x11E568: Fl::handle_(int, Fl_Window*) (Fl.cxx:1227)
==525456==    by 0x11E20C: Fl::handle(int, Fl_Window*) (Fl.cxx:1124)
==525456==    by 0x166B4C: fl_handle(_XEvent const&) (Fl_x.cxx:2290)
==525456==    by 0x160545: do_queued_events() (Fl_x.cxx:214)
==525456==    by 0x161164: fd_callback(int, void*) (Fl_x.cxx:470)
==525456==  Address 0x699fca8 is 8 bytes inside a block of size 176 free'd
==525456==    at 0x483D1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==525456==    by 0x13BE48: Fl_Tree_Item::~Fl_Tree_Item() (Fl_Tree_Item.cxx:103)
==525456==    by 0x13B519: Fl_Tree_Item_Array::remove(int) (Fl_Tree_Item_Array.cxx:168)
==525456==    by 0x13D1CB: Fl_Tree_Item::remove_child(Fl_Tree_Item*) (Fl_Tree_Item.cxx:640)
==525456==    by 0x137933: Fl_Tree::remove(Fl_Tree_Item*) (Fl_Tree.cxx:926)
==525456==    by 0x11C6D1: _ZZZ4mainENKUlPT_PvE_clI9Fl_WidgetEEDaS0_S1_ENKUlS0_S1_E_clIS4_EEDaS0_S1_ (in /tmp/x)
==525456==    by 0x11CA97: _ZZZ4mainENKUlPT_PvE_clI9Fl_WidgetEEDaS0_S1_ENUlS0_S1_E_4_FUNIS4_EEDTcldtdeLKPKS5_0EonclIS_EscOS0_fp_scOS1_fp0_EES0_S1_ (in /tmp/x)
==525456==    by 0x1415F9: Fl_Widget::do_callback(Fl_Widget*, void*) (Fl_Widget.cxx:328)
==525456==    by 0x120548: Fl_Widget::do_callback() (Fl_Widget.H:881)
==525456==    by 0x120B6C: Fl_Button::handle(int) (Fl_Button.cxx:123)
==525456==    by 0x11E175: send_event(int, Fl_Widget*, Fl_Window*) (Fl.cxx:1044)
==525456==    by 0x11E568: Fl::handle_(int, Fl_Window*) (Fl.cxx:1227)

The first line shows that auto t = it->tree(); in the button callback is the problematic statement.

The reason why this happens is because the button is not deleted when the tree item is removed. It's invisible, but it's still there. Clicking on it again executes the callback which accesses the deleted tree item. Even your test accesses deleted data.

It's disputable whether t->remove(it); should delete the associated widget, and ISTR that this was once discussed, but my test clearly shows that it is not deleted. Greg (@erco77) could probably explain the details. The following replacement of the del button callback will resolve the issue:

        d->callback([](auto* w, void* data){
            auto it = reinterpret_cast<Fl_Tree_Item*>(data);
            auto t = it->tree();
            auto wid = dynamic_cast<Fl_Button *>(w);
            delete wid; // this is the key: delete the button
            fprintf(stderr, "widget = %p\n", (void*)w);
            t->remove(it);
            t->redraw();
        }, item);

Note that wid will always be non-NULL because it's used in a widget (button) callback, but even if it's NULL the delete statement would be valid. Since the button is deleted there's no chance that a button callback is called twice.

Your suggestion

I think it would be nice to add some internal "removed" flag into node itself, ...

wouldn't help because the tree item itself is deleted and accessing it after deletion would be forbidden anyway. You must make sure not to access deleted data in the first place.

[Albrecht-S]

One more note: you may want to add t->widget(0); before you delete the widget just in case a future version of FLTK (Fl_Tree_Item) would delete the associated widget. Deleting the widget would remove it from Fl_Tree's widget group automatically, hence this should be clean.

[basiliscos]

Yeah, auto-deleting underlying widget when tree item is removed, seems expected/reasonable behavior by default for me. I'm a bit surprised, that it is not.

(So, when the whole tree is deleted, the custom widgets in tree items still live... ??? ).

Will try your solution, seems a bit better workaround...

The reason why this happens is because the button is not deleted when the tree item is removed. It's invisible, but it's still there.

Why so? OK, I understand that the button is not deleted, however I'd not expect, that it will receive clicks, when it's container (tree-item) is deleted/removed.

[erco77]

When you create a custom button/custom widget to be used in Fl_Tree, it becomes a child of the Fl_Group which then 'owns' it. The widget's ownership is then managed with the inherited Fl_Group::xxx() methods (such as Fl_Group::clear(w), Fl_Group::remove(w), or Fl::delete_widget(w), etc)

When you assign the widget to a particular Fl_Tree_Item with item->widget(your_widget), it's just associating the widget pointer to that particular item, not re-parenting it. (Fl_Tree_Item is not derived from Fl_Widget or Fl_Group). So the widget is still 'owned' by Fl_Group.

This allows the widget to be unassigned from one Fl_Tree_Item (with item->remove(w)), and reassigned to another (with item->widget(w)), without needing to destroy + recreate the widget.

To actually destroy the widget, first un-assign it from the item (using item->widget(0)) and then use the usual techniques to either remove or delete a child widget from an Fl_Group, e.g. Fl_Tree::Fl_Group::remove(w) to remove from the tree's group without deleting, or Fl::delete_widget(w) to remove + destroy, etc.

[basiliscos]

Would it be possible together with assigning widget to TreeItem assign also ownership of it? (i.e. item->widget(widget_ptr, true) ? To have API-compatibility, the ownership flag will be false by default... ?

[erco77]

I didn't do this because in general its bad design to have two "owners" of an instance, and here Fl_Group really is the owner at all times; it has to be for events and such to operate properly. Also, arguably for consistency it would be necessary to provide Fl_Tree_Item::begin()/end()/add()/etc etc which takes things off the rails.

If Fl_Tree_Item were itself an Fl_Group/Fl_Widget derived widget, then it would be OK, but Fl_Tree_Item is purposefully designed not to be for efficiency, so there can be hundreds or thousands of items without the overhead of an Fl_Group in each item, esp. when the items are not hosts of Fl_Widgets.

I'd offer to solve the issues you're running into, the docs for Fl_Tree and Fl_Tree_Item::widget() should be fleshed out more about how to manage child widgets assigned to Fl_Tree_Items properly, esp. with regards to 'ownership' and the recommended way to delete/destroy.

I'd also offer Fl_Group's docs should cover this too, as it's not clear from anything in Fl_Group how to properly destroy a child widget. Perhaps Fl_Group should have a virtual delete(w) (that simply calls Fl::delete_widget()) so it's easy to discern from the docs how to do this, and then by extension Fl_Tree could extend this method to ensure the widget is not only deleted from the group, but also un-assigns it from any tree item it's been associated with.

I'd suggest opening a discussion on fltkcoredev to discuss API/doc improvements (so other devs can weigh in), as such conversations are pretty hidden within issues.

[Albrecht-S]

FTR: deleting a widget and removing it from its parent (Fl_Group) is as simple as delete w;. This is documented, for instance in the destructor (~Fl_Widget) docs.

There are some details in the docs of Fl::delete_widget() as well. The automatic removal from its parent group was obviously (as documented) introduced in 1.3.0.

Side note: Fl::delete_widget() should no longer be necessary since FLTK 1.3.x (which exact x I do not know). It has always only be recommended if you wanted to delete a widget in its own callback (directly or indirectly [1]), but I hope that this is no longer necessary because we fixed the access-after-delete issues we had in 1.1.x. However, there's no official guarantee. Personally I would always call delete w in FLTK 1.4 (and 1.3) rather than using Fl::delete_widget(w);.

[1] with indirectly I mean deleting one of the parents of a widget which would recursively delete the widget itself, e.g. delete button->parent() or something like that.

[Albrecht-S]

@basiliscos wrote:

auto-deleting underlying widget when tree item is removed, seems expected/reasonable behavior ...
(So, when the whole tree is deleted, the custom widgets in tree items still live... ??? ).

When the whole Fl_Tree widget is deleted, this also deletes all child widgets. However, when a single Fl_Tree_Item is deleted, its associated widget (if any) is not deleted, as Greg explained.

@erco77 I'm wondering if Fl_Tree::remove(item) is maybe a bad (name) choice. The docs say the item is "removed", but what does this mean? Is it also destroyed (deleted)? I believe yes, particularly because it works recursively. Note that the analog method Fl_Group::remove() (same name) does not delete the removed widget (this is documented).

[erco77]

I used Fl_Browser for reference for the Fl_Tree API, as their needs are similar in terms of being a collection of items that are not derived from Fl_Widget.

In the case of Fl_Tree::remove(), its docs read much like Fl_Browser::remove(), and neither elaborates on destruction but probably should, as both do destroy the item. Elaborating would prevent confusion with Fl_Group::remove(), which does /not/ destroy.

Fl_Browser::remove(int)

    Remove entry for given line number, making the browser one line shorter.
    You must call redraw() to make any changes visible. 

Fl_Tree::remove(..)

    Remove the specified 'item' from the tree.
    'item' may not be NULL. If it has children, all those are removed too.
    If item being removed has focus, no item will have focus.

[Albrecht-S]

Ah, ok, I see. History and docs... ;)

I don't want to go further into details, but anyway: Some time ago I thought I could make some Fl_Group methods virtual (e.g. Fl_Group::clear(), add(), remove()) but it turned out it's impossible because these method names had been overloaded by methods with the same name but sometimes very different semantics in derived classes. One example is obviously Fl_Group::remove() vs. Fl_Tree::remove() but there are a lot more of those. History... I gave up. ;-(

[erco77]

Well, at very least the docs can be improved. I like showing examples in the widget class descriptions for doing things that are non-obvious or are common use patterns.

An example added to Fl_Group's description showing how to properly delete a child widget in various contexts, both from normal methods and callbacks/event handlers could also be useful, as well as citing historical artifacts (like the changes in 1.3.0 that you mentioned), and techniques to avoid.

[erco77]

I've opened a thread in fltkcoredev:

Subject: Fl_Group/Fl_Tree/etc: document how to properly delete widgets/items from container widgets

..so we can better hash this out, as one can't use text colors and such in issues, where I think it would be useful to explore doc changes.

[Albrecht-S]

I've opened a thread in fltkcoredev ...

@erco77 Good move. We're gone way off-topic here and should move all further discussion to fltk.coredev

@basiliscos I believe your issue is clear now and there's nothing we can do directly in FLTK for now. See also the discussion in fltk.coredev if you're interested.

@basiliscos Can we close this issue?

[basiliscos]

Yes, thanks a lot!