Re: [fltk.coredev] Proposal: adding FL_WHEN_DELETING and the event FL_DELETING

[Michael Sweet]

+1

> On Oct 26, 2022, at 6:29 AM, 'melcher....@googlemail.com' via fltk.coredev <fltkc...@googlegroups.com> wrote:
>
> Problem:
>
> When adding user_data in callbacks, it is difficult to make sure that the user_data is deleted when the widget is deleted, especially when deleting a group auto deletes all children.
>
> Solution:
>
> Add FL_WHEN_DELETING to Fl_Widget::when(), so before deleting a widget, the user can receive a callback and get a chance to clean up. The reason for the callback would be indicated by setting Fl::event() to FL_DELETING, which would also be sent to Fl_Widget::handle(int).
>
> --
> You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/1d37d011-1554-4295-9fac-34b7379ba85bn%40googlegroups.com.

________________________
Michael Sweet

[Albrecht Schlosser]

On 10/26/22 12:29 'melcher....@googlemail.com' via fltk.coredev wrote:

Problem:

When adding user_data in callbacks, it is difficult to make sure that the user_data is deleted when the widget is deleted, especially when deleting a group auto deletes all children.

Solution:

Add FL_WHEN_DELETING to Fl_Widget::when(), so before deleting a widget, the user can receive a callback and get a chance to clean up. The reason for the callback would be indicated by setting Fl::event() to FL_DELETING, which would also be sent to Fl_Widget::handle(int).

Interesting.

Thinking about it ... Hmm, I don't think that sending a callback to the *widget* that is about to be deleted is enough (see below). Generally this is already done by calling its destructor. Sure, using the destructor involves subclassing whereas sending a callback could be handled w/o subclassing. Maybe it's useful to add for cases where the widget itself "knows" what to do.

But how does the (child) *widget* "know" what its parent class needs to do, i.e. what to clean up if it's a child of a group? The parent can be any type of group, e.g. Fl_Pack, Fl_Flex, Fl_Grid, or Fl_Scroll and the child widget doesn't necessarily know what subclass of Fl_Group its parent is. This would need to be hardcoded in the callback (which might work). The removal of the widget from its parent group is done anyway in the destructor of the widget, and this notifies the Fl_Group widget.

The latter is where my new proposal kicks in: container classes like Fl_Scroll, Fl_Flex, Fl_Grid etc. can opt in for notification that a child is about to be added, moved inside the group, or removed by implementing a new virtual method and can do whatever they need. Hence the "callback" (virtual method) is delivered to the parent class directly. This is what I implemented but not yet published because I was still working on it.

Conclusion:

Maybe both approaches can have a value in different contexts of a program. A user (program) may want to know when a widget is deleted to clean up specific widget related data but my current problem is more about the parent of the widget that needs to know when a child is deleted because it (the parent) stores additional information about some or all children (Fl_Flex, Fl_Grid). This is independent of the widget's callback proposed above because it can't be "delegated" to the child to call the proper cleanup function of the parent group.

So, maybe "yes" to both approaches.

[melcher....@googlemail.com]

Ah, sorry, my message were coincidendtal, but this has nothing to do with Groups. This is purely a callback thing that is meant to give callback users a chance to deallocate user data that they may have allocated for just this widget. The Group thing is quite easy to resolve: we just need to make Fl_Group::insert(), remove(), and clear() virtual.

But now to explain this case. Assuming your app allocates and deletes widgets dynamically, let's say you need more data in a callback than just a single pointer. So you allocate a dataset when allocating the widget, but you can't make sure that the dataset is deleted whenever the widget is deleted by the system.

For example (not compilable code, grossly simplified):

  struct {
    int x;
    int y;
  } CallbackData;


  void button_cb(Fl_Button *w, void *u) {
    CallbackData *d = (CallbackData*)u;
    if (Fl::event()==FL_DELETING)
      delete d;
    else
      printf("I got %d and %d\n", d->x, d->y);
  }

  myButton = new Fl_Button(...)
  myButton->callback(button_cb, new CallbackData(42, 128) );

  myButton->when(FL_WHEN_CHANGED|FL_WHEN_DELETING);

This needs just a few lines of code to be implemented:

in Fl_Widget at the top:

  if (when() & FL_WHEN_DELETING) {

    int old_event = Fl::event();

    Fl::event(FL_DELETING);

    do_callback();

    Fl::event(old_event);

  }

plus the declaration of those constants and some Fluid changes.

There is an alternative: just as in Fl_Widget::label_copy(char*), we could add Fl_Widget::callback_data_copy(void*), set a flag, and call free(_user_data) when deleting, or call delete _user_data, so it would be a set-and-forget with less flexibility.

[Albrecht Schlosser]

On 10/27/22 14:12 'melcher....@googlemail.com' via fltk.coredev wrote:

Ah, sorry, my message were coincidendtal, but this has nothing to do with Groups. This is purely a callback thing that is meant to give callback users a chance to deallocate user data that they may have allocated for just this widget.

Yes, I understood. That's why I concluded that we may want to have both.

The Group thing is quite easy to resolve: we just need to make Fl_Group::insert(), remove(), and clear() virtual.

Unfortunately not. Been there, "tried" that, but it can't work because we have too many overloaded add(), remove() and clear() and maybe also insert() methods with entirely unrelated semantics. Making the existing Fl_Group:: methods virtual doesn't play together with these conflicting methods. I thought about adding new virtual methods with non-conflicting names instead but this looked too convoluted. Maybe it could work somehow but I stopped investigating and this is OT here anyway.

But now to explain this case. Assuming your app allocates and deletes widgets dynamically, let's say you need more data in a callback than just a single pointer. So you allocate a dataset when allocating the widget, but you can't make sure that the dataset is deleted whenever the widget is deleted by the system.

For example (not compilable code, grossly simplified):

  struct {
    int x;
    int y;
  } CallbackData;


  void button_cb(Fl_Button *w, void *u) {
    CallbackData *d = (CallbackData*)u;
    if (Fl::event()==FL_DELETING)
      delete d;
    else
      printf("I got %d and %d\n", d->x, d->y);
  }

  myButton = new Fl_Button(...)
  myButton->callback(button_cb, new CallbackData(42, 128) );

  myButton->when(FL_WHEN_CHANGED|FL_WHEN_DELETING);

Good example. Thanks.

This needs just a few lines of code to be implemented:

in Fl_Widget at the top:

  if (when() & FL_WHEN_DELETING) {

    int old_event = Fl::event();

    Fl::event(FL_DELETING);

    do_callback();

    Fl::event(old_event);

  }

plus the declaration of those constants and some Fluid changes.

Where "in Fl_Widget" would this be implemented? I assume you mean in ~Fl_Widget(), i.e. in the destructor. Or did you mean the handle() method? Anyway, assuming correct implementation:

+1

There's one remaining problem though, if I'm not missing anything. Your example seems perfect, but I believe at the time the destructor of Fl_Widget is called, the derived class would already be partially destroyed. Hence we need to document clearly that the callback would only be allowed to deallocate the related (user) data but not access the widget, i.e. any parts of the derived class. Or whatever.

There is an alternative: just as in Fl_Widget::label_copy(char*), we could add Fl_Widget::callback_data_copy(void*), set a flag, and call free(_user_data) when deleting, or call delete _user_data, so it would be a set-and-forget with less flexibility.

Fl_Widget::copy_label() does indeed make a copy of a string and the previous owner of the string can free it immediately. Hence the name `copy...`.

Fl_Widget::callback_data_copy(void*) can't make a copy of the data (because we don't know its size). The only thing we can do is to "transfer ownership" of existing data (and set a flag). Hence `callback_data_copy` would be a misnomer, and the lack of flexibility makes me vote -1 for this in favor of the original proposal above.

[melcher....@googlemail.com]

Albrecht Schlosser schrieb am Donnerstag, 27. Oktober 2022 um 14:58:20 UTC+2:

The Group thing is quite easy to resolve: we just need to make Fl_Group::insert(), remove(), and clear() virtual.

Unfortunately not. Been there, "tried" that, but it can't work because we have too many overloaded add(), remove() and clear() and maybe also insert() methods with entirely unrelated semantics.

I just checked. The union of child1_ and array_ is only written in Fl_Group::insert(Fl_Widget*, int index) and Fl_Group::remove(int index) and nowhere else. Fl_Group::erase() accesses it via array() in the most heinous of ways. Fl_Scroll and Fl_Tree reshuffle children, but at least do not add or remove any. Fl_Table does its own thing anyway (sigh). Everything else just reads array(). So I think insert, remove, and clear are good enough. Fl_Group::array() should be deprecated, disable, curried, and removed.

This needs just a few lines of code to be implemented:

in Fl_Widget at the top:

  if (when() & FL_WHEN_DELETING) {

    int old_event = Fl::event();

    Fl::event(FL_DELETING);

    do_callback();

    Fl::event(old_event);

  }

plus the declaration of those constants and some Fluid changes.

Where "in Fl_Widget" would this be implemented? I assume you mean in ~Fl_Widget(), i.e. in the destructor. Or did you mean the handle() method? Anyway, assuming correct implementation:

In ~Fl_Widget as one of the first lines to keep much of Fl_Widget in tact. But it doesn't;t really matter as it is really only there to avoid deriving a new class. If a new class is derived, this code can go into the destructor anyway. Also, doe we really need thin in ::handle()? Because, if a new widget is created anyway to have a new handle() method, why not again just put that code in the destructor. It doesn't really do any harm either, so, eh?

 

+1

There's one remaining problem though, if I'm not missing anything. Your example seems perfect, but I believe at the time the destructor of Fl_Widget is called, the derived class would already be partially destroyed. Hence we need to document clearly that the callback would only be allowed to deallocate the related (user) data but not access the widget, i.e. any parts of the derived class. Or whatever.

Yes, that's correct, and it's one reason why it should only be available in the callback, to make clear that that is really the main (and only?) purpose. It could bused to clear external links *to* that widget, but that's a bit of a stretch.

 

Fl_Widget::copy_label() does indeed make a copy of a string and the previous owner of the string can free it immediately. Hence the name `copy...`.

Yes, my example was not a good match. I just wanted to illustrate that we can have a label that is automatically free'd when the widget goes, and we could also have user_data that can be marked to be automatically free'd or deleted. 

[Albrecht Schlosser]

On 10/27/22 21:05 'melcher....@googlemail.com' via fltk.coredev wrote:

Albrecht Schlosser schrieb am Donnerstag, 27. Oktober 2022 um 14:58:20 UTC+2:

The Group thing is quite easy to resolve: we just need to make Fl_Group::insert(), remove(), and clear() virtual.

Unfortunately not. Been there, "tried" that, but it can't work because we have too many overloaded add(), remove() and clear() and maybe also insert() methods with entirely unrelated semantics.

I just checked. The union of child1_ and array_ is only written in Fl_Group::insert(Fl_Widget*, int index) and Fl_Group::remove(int index) and nowhere else. Fl_Group::erase() accesses it via array() in the most heinous of ways.

OK, we're drifting even more OT, but yes: that's something I would really like to remove. It's IMHO useless anyway since a group is something that should hold more than one child in most cases. The special case 1 (child) is an intermediate status, not something we need to save memory. So why not allocate the array with a certain number of children already for the first child and manage it dynamically as with other arrays? The advantage is that we wouldn't need to alloc/realloc/free (off the top of my head, not checked) for every single child that is added or removed. The drawback would be that we need another variable for the allocated array size. But this would (a) simplify the code and (b) remove many allocations and deallocations when children are added or removed.

Fl_Scroll and Fl_Tree reshuffle children, but at least do not add or remove any.

I don't understand. Do you mean that their code does not explicitly add or remove children? This should always be the case because this is implemented in Fl_Group. So what are you saying here?

Fl_Table does its own thing anyway (sigh). Everything else just reads array(). So I think insert, remove, and clear are good enough. Fl_Group::array() should be deprecated, disable, curried, and removed.

Fl_Group::array() is again just another topic and off-topic here. It has nothing to do with the point I was making. What I wanted to say is that we have several methods in derived classes that have the same *names* as one of the methods in question but different semantics, arguments, and/or return types. You can't make a method of the base class like remove() virtual and expect it to work in a derived class which already has a method of the same name but with totally different semantics. Let's look for an example ...

OK, found one: try to make all (three) variants of Fl_Group::remove(...) virtual in Fl_Group.H:

  virtual void remove(int index);
  virtual void remove(Fl_Widget&);
  virtual void remove(Fl_Widget* o) {remove(*o);}

Trying to compile:

[44/483] Building CXX object src/CMakeFiles/fltk.dir/Fl_Check_Browser.cxx.o
FAILED: src/CMakeFiles/fltk.dir/Fl_Check_Browser.cxx.o 
/usr/bin/c++  -DFL_LIBRARY -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -I. -I../../ -I/usr/include/freetype2 -I/usr/include/pango-1.0 -I/usr/include/fribidi -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/uuid -I/usr/include/libpng16 -g   -D_THREAD_SAFE -D_REENTRANT -MD -MT src/CMakeFiles/fltk.dir/Fl_Check_Browser.cxx.o -MF src/CMakeFiles/fltk.dir/Fl_Check_Browser.cxx.o.d -o src/CMakeFiles/fltk.dir/Fl_Check_Browser.cxx.o -c ../../src/Fl_Check_Browser.cxx
In file included from ../../src/Fl_Check_Browser.cxx:22:
../../FL/Fl_Check_Browser.H:81:7: error: conflicting return type specified for ‘virtual int Fl_Check_Browser::remove(int)’
   81 |   int remove(int item);           // delete an item. Returns nitems()
      |       ^~~~~~
In file included from ../../FL/Fl_Browser_.H:26,
                 from ../../FL/Fl_Check_Browser.H:24,
                 from ../../src/Fl_Check_Browser.cxx:22:
../../FL/Fl_Group.H:111:15: note: overridden function is ‘virtual void Fl_Group::remove(int)’
  111 | virtual  void remove(int index);
      |               ^~~~~~
[240/483] Building CXX object fluid/CMakeFiles/fluid.dir/factory.cxx.o
FAILED: fluid/CMakeFiles/fluid.dir/factory.cxx.o 
/usr/bin/c++  -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -I. -I../../ -I/usr/include/freetype2 -I/usr/include/pango-1.0 -I/usr/include/fribidi -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/uuid -I/usr/include/libpng16 -g   -D_THREAD_SAFE -D_REENTRANT -MD -MT fluid/CMakeFiles/fluid.dir/factory.cxx.o -MF fluid/CMakeFiles/fluid.dir/factory.cxx.o.d -o fluid/CMakeFiles/fluid.dir/factory.cxx.o -c ../../fluid/factory.cxx
In file included from ../../fluid/factory.cxx:174:
../../FL/Fl_Check_Browser.H:81:7: error: conflicting return type specified for ‘virtual int Fl_Check_Browser::remove(int)’
   81 |   int remove(int item);           // delete an item. Returns nitems()
      |       ^~~~~~
In file included from ../../FL/Fl_Window.H:24,
                 from ../../fluid/factory.cxx:33:
../../FL/Fl_Group.H:111:15: note: overridden function is ‘virtual void Fl_Group::remove(int)’
  111 | virtual  void remove(int index);
      |               ^~~~~~
[353/483] Building CXX object test/CMakeFiles/sudoku.dir/sudoku.cxx.o
ninja: build stopped: cannot make progress due to previous errors.

This is only one example where different argument and return types prevent to make Fl_Group::remove() virtual. I believe that I found more but it's not necessary to find all these conflicts, one conflict is enough.

I'm not (yet) talking about all those Fl_Group:: methods like clear() which you can make virtual and that would pass the compiler but still change semantics unnoticed and/or break existing programs. I remember when Fl_Window::hide() and show() were made virtual and even code in FLTK was suddenly broken in an unpredicted way. Yes, that's true.

I know that I tried making all these children handling methods virtual but I gave up. That's the point I was making: we just can't make some methods virtual because we have some conflicting methods of the same names in derived classes.

Can we work around these conflicts w/o breaking existing programs? I doubt that we can but I'd like to learn that we can - because that was what I initially wanted to do but couldn't.

This needs just a few lines of code to be implemented:

in Fl_Widget at the top:

  if (when() & FL_WHEN_DELETING) {

    int old_event = Fl::event();

    Fl::event(FL_DELETING);

    do_callback();

    Fl::event(old_event);

  }

plus the declaration of those constants and some Fluid changes.

Where "in Fl_Widget" would this be implemented? I assume you mean in ~Fl_Widget(), i.e. in the destructor. Or did you mean the handle() method? Anyway, assuming correct implementation:

In ~Fl_Widget as one of the first lines to keep much of Fl_Widget in tact.

Yes, OK, that's what I thought too. But it keeps only those parts of the base class Fl_Widget intact and not those of the derived class that is being destroyed - as far as I understand the C++ mechanics. And that is something the user programmer *needs* to know: at the time the callback is called the widget is already partially destroyed (dangerous!). I think you can still call methods of Fl_Widget but none of any subclass or access member variables of the subclass (although in practice, you can, but that violates C++ rules).

But it doesn't;t really matter as it is really only there to avoid deriving a new class. If a new class is derived, this code can go into the destructor anyway. Also, doe we really need thin in ::handle()? Because, if a new widget is created anyway to have a new handle() method, why not again just put that code in the destructor. It doesn't really do any harm either, so, eh?

I would opt to not send an event in the destructor - for the same reasons as said before. Calling the callback should be enough. The usual way to call a callback is in the handle() method when an event is processed and a condition is fulfilled to call the callback. If we'd send an event to a (partially destroyed) widget *and* call the callback then the callback could be called twice under certain circumstances (we can't avoid programmer errors). It's also usual to call the event handling procedure of each parent class, but what would all these parent classes add to this callback processing? I can't imagine what this could be good for.

If we'd do this, then I believe that calling the callback from ~Fl_Widget() would be enough and sending an event would make things only unnecessarily complicated and error prone. I may be wrong, but that's why we are discussing this. To learn what to do and how...

 

+1

There's one remaining problem though, if I'm not missing anything. Your example seems perfect, but I believe at the time the destructor of Fl_Widget is called, the derived class would already be partially destroyed. Hence we need to document clearly that the callback would only be allowed to deallocate the related (user) data but not access the widget, i.e. any parts of the derived class. Or whatever.

Yes, that's correct, and it's one reason why it should only be available in the callback, to make clear that that is really the main (and only?) purpose. It could bused to clear external links *to* that widget, but that's a bit of a stretch.

Well put. I agree.

 

Fl_Widget::copy_label() does indeed make a copy of a string and the previous owner of the string can free it immediately. Hence the name `copy...`.

Yes, my example was not a good match. I just wanted to illustrate that we can have a label that is automatically free'd when the widget goes, and we could also have user_data that can be marked to be automatically free'd or deleted.

OK, then I believe we can drop this idea which might be problematic API-wise anyway and keep the callback idea as proposed.

[melcher....@googlemail.com]

Albrecht Schlosser schrieb am Donnerstag, 27. Oktober 2022 um 23:38:03 UTC+2:

Fl_Scroll and Fl_Tree reshuffle children, but at least do not add or remove any.

I don't understand. Do you mean that their code does not explicitly add or remove children? This should always be the case because this is implemented in Fl_Group. So what are you saying here?

Fl_Scroll uses Fl_Scroll::fix_scrollbar_order() to check regularly during draw() handle() and resize() if widgets were added and rearranges them so that the two scrollbars are the last widgets in the array.

Fl_Table "overrides" the non-virtual functions add() and remove() to forward the functionality to an internal Fl_Scroll. 

Fl_Tree does not allow adding Widgets as children, only Fl_Tree_Item which it manages in its own arrays and may or may not contain an Fl_Widget. Also see Fl_Tree::fix_scrollbar_order()

 

(...) we have several methods in derived classes that have the same *names* as one of the methods in question but different semantics, arguments, and/or return types. You can't make a method of the base class like remove() virtual and expect it to work in a derived class which already has a method of the same name but with totally different semantics. Let's look for an example ...

In file included from ../../src/Fl_Check_Browser.cxx:22:
../../FL/Fl_Check_Browser.H:81:7: error: conflicting return type specified for ‘virtual int Fl_Check_Browser::remove(int)’
   81 |   int remove(int item);           // delete an item. Returns nitems()
      |       ^~~~~~
In file included from ../../FL/Fl_Browser_.H:26,
                 from ../../FL/Fl_Check_Browser.H:24,
                 from ../../src/Fl_Check_Browser.cxx:22:
../../FL/Fl_Group.H:111:15: note: overridden function is ‘virtual void Fl_Group::remove(int)’
  111 | virtual  void remove(int index);
      |               ^~~~~~

Yes, sorry, I remember that we even talked about that a while ago. I made a PR at https://github.com/fltk/fltk/pull/526 and added another post here that I would like to offer as a solution for these collected issues.

Continued at 

https://groups.google.com/g/fltkcoredev/c/k_o3LlBZ4mQ

[melcher....@googlemail.com]

Albrecht Schlosser schrieb am Donnerstag, 27. Oktober 2022 um 23:38:03 UTC+2:

 

in ~Fl_Widget at the top:

  if (when() & FL_WHEN_DELETING) {

    int old_event = Fl::event();

    Fl::event(FL_DELETING);

    do_callback();

    Fl::event(old_event);

  }

plus the declaration of those constants and some Fluid changes.

+1

There's one remaining problem though, if I'm not missing anything. Your example seems perfect, but I believe at the time the destructor of Fl_Widget is called, the derived class would already be partially destroyed. Hence we need to document clearly that the callback would only be allowed to deallocate the related (user) data but not access the widget, i.e. any parts of the derived class. Or whatever.

Yes, that's correct, and it's one reason why it should only be available in the callback, to make clear that that is really the main (and only?) purpose. It could bused to clear external links *to* that widget, but that's a bit of a stretch.

Well put. I agree.

 

Fl_Widget::copy_label() does indeed make a copy of a string and the previous owner of the string can free it immediately. Hence the name `copy...`.

Yes, my example was not a good match. I just wanted to illustrate that we can have a label that is automatically free'd when the widget goes, and we could also have user_data that can be marked to be automatically free'd or deleted.

OK, then I believe we can drop this idea which might be problematic API-wise anyway and keep the callback idea as proposed.

Thanks for all the great feedback. I will wrap this into a Pull Request, so you guys can have an additional say on the implementation and documentation.

 - Matthias

[melcher....@googlemail.com]

tl;dr: Ok, trying to implement things sometimes reveals bad thinking quite well. FL_WHEN_DELETING was a bad idea that I withdraw.

There are several reasons. Mainly, Fl_Widget->when() is not always used as a group of flags, but user code may do things like if (when()==FL_WHEN_CHANGED), and adding FL_WHEN_DELETING will break user code.

It won't work for Fl_Menu_Item, so it's inconsistent.

Also, the only case where I thought I needed this was easily solved in another way.

And lastly, using two bits in the Fl_widget::flags_ filed to request a :free(), delete, or delete[] on ~Fl_Widget is a much simpler set-and-forget way to do this, if we ever feel the need.

Sorry for all the noise!