Insomnia, Fl_Flex, and resizable

[melcher....@googlemail.com]

tl;dr Proposal to add Fl_Widget::resizable(int width_factor, int height_factor) and merge Fl_Flex into Fl_Pack.

I was looking into how Fl_Flex differs from Fl_Pack, and apart from the margins, Fl_Flex adds the option the make individual children resizable while the rest stays fixed. So instead of introducing a new widget, wouldn't it be better to add this functionality to Fl_Pack?

Which brings me to the second point. Fl_Group::resizable(Fl_Widget*) is a horrible interface. First of all, we reference a child widget, wich may disappear, giving us a dangling pointer. Second of all, we can only have one resizable child, which generated the need for Fl_Flex.

Now, Fl_Flex maintains a list of children that are resizable, and the list is not updated if children are removed because Fl_Group::remove() and clear() are not virtual (delete_child() is, but that is not enough). Again, we get dangling pointers.

My proposal is to remove Fl_Group::resizable_ and instead add either a flag or an integer to Fl_Widget to indicate if the widget is resizable (flag) or even better, by what factor a widget is resizable (int). Adding the functionality of Fl_Flex to Fl_Pack would be ten lines of code. Fl_Group::resizable(Fl_Widget*) could be implemented with the exact same API and functionality, but without the risk of dangling pointers.

Since the resizable() method name is already taken, we need a new method name. We could use Fl_Widget::flexible(bool), but how about Fl_Widget::resizable(int width, int height) where width and height are factors in the given direction. It's a bit of a stretch, as Fl_Group::resizable(child) would be the same as child->resizable(100, 100), but it can't all be perfect. This would add two integers, eight bytes, to every Fl_Widget, and remove one pointer from Fl_Group and more pointers from Fl_Pack/Fl_Flex.

[melcher....@googlemail.com]

I should add that I find it very important that we get a 1.4.0 release out very very soon. I am suggesting this now, because once we added Fl_Flex in 1.4, we will not remove it in 1.5 because Fl_Pack can then do the same.

[Michael Sweet]

FWIW, +1 for me (always seemed strange to only allow one widget to be resizable), but if you do that then you can consolidate Fl_Pack, Fl_Flex, and Fl_Tile - a non-resizable widget just gets stacked/packed while a resizable one fills the remainder. If two resizable widgets border each other then you can drag the border to resize.

> --
> 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/acfad3a8-6dfd-4777-9362-624e71c62361n%40googlegroups.com.

________________________
Michael Sweet

[Albrecht Schlosser]

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

tl;dr Proposal to add Fl_Widget::resizable(int width_factor, int height_factor) and merge Fl_Flex into Fl_Pack.

I was looking into how Fl_Flex differs from Fl_Pack, and apart from the margins, Fl_Flex adds the option the make individual children resizable while the rest stays fixed. So instead of introducing a new widget, wouldn't it be better to add this functionality to Fl_Pack?

No. Fl_Pack has some properties that differ from Fl_Flex and all other widgets and groups: Fl_Pack "shrink-wraps" itself around its children, and it does this during its draw() method which leads to all sorts of unexpected behavior. You can't really say that an Fl_Pack has a pre-determined size because of this "feature".

Fl_Pack also uses the resizable() widget in a very special way that is not used elsewhere and leads to unpredictable behavior, particularly in nested Fl_Pack widgets.

Fl_Flex is a better alternative to Fl_Pack because it doesn't have all these IMHO negative properties. You can give it a specific size and it fills its space with its children. The *standard* behavior is "flexible", i.e. all children adjust their sizes to the available space, not vice versa. The *exception* is to give some widgets a fixed size for special layout options.

If we could choose I would remove Fl_Pack and use Fl_Flex instead as the new "Fl_Pack" widget. However, changing Fl_Pack is not an option, and that's why I "ported" the existing Fl_Flex widget to FLTK as an alternative and Fl_Pack should be considered deprecated. The design and API of Fl_Flex have in parts been chosen such that it can be used as a drop-in replacement of Fl_Pack without Fl_Pack's problems. Hopefully.

Fl_Pack may be used in existing programs, and therefore we should keep it as is to avoid backwards compatibility issues.

Which brings me to the second point. Fl_Group::resizable(Fl_Widget*) is a horrible interface. First of all, we reference a child widget, wich may disappear, giving us a dangling pointer. Second of all, we can only have one resizable child, which generated the need for Fl_Flex.

Yep, maybe. But that's not a bug, it's a "feature" since FLTK 1.0, isn't it? And it works...

Now, Fl_Flex maintains a list of children that are resizable, and the list is not updated if children are removed because Fl_Group::remove() and clear() are not virtual (delete_child() is, but that is not enough). Again, we get dangling pointers.

That's true, as it is now, and it is a known problem. I have been working on a general solution to be applied to Fl_Group and inherited by Fl_Flex and other derived widget to overcome this issue. Unfortunately helping to get the Wayland support working distracted me and this work was not yet applied to Fl_Flex although it's almost ready.

My proposal is to remove Fl_Group::resizable_ and instead add either a flag or an integer to Fl_Widget to indicate if the widget is resizable (flag) or even better, by what factor a widget is resizable (int). Adding the functionality of Fl_Flex to Fl_Pack would be ten lines of code.

I strongly recommend NOT to change Fl_Pack at all. As said above, leave it as is for backwards compatibility and get rid of its odd behavior for new development.

Fl_Group::resizable(Fl_Widget*) could be implemented with the exact same API and functionality, but without the risk of dangling pointers.

Fl_Group does already "know about" deleted widgets and if it doesn't take the deletion of the resizable() widget into account that is probably a bug. It would be easy to do this, however the decision would be harder: what should be done?

Option one: set resizable(0) which would make the Fl_Group widget no longer reiszable.

Option two: set resizable(this) seems the more appropriate option which would leave the group resizable.


I suggest to discuss options to implement a feature to observe widget deletion in a separate thread. I'd like to see your (and others') comments on my proposal and we should find a proper way

Since the resizable() method name is already taken, we need a new method name. We could use Fl_Widget::flexible(bool), but how about Fl_Widget::resizable(int width, int height) where width and height are factors in the given direction. It's a bit of a stretch, as Fl_Group::resizable(child) would be the same as child->resizable(100, 100), but it can't all be perfect. This would add two integers, eight bytes, to every Fl_Widget, and remove one pointer from Fl_Group and more pointers from Fl_Pack/Fl_Flex.

If we had both these features, i.e. Fl_Group::resizable(Fl_Widget *) and Fl_Widget::resizable(...) then we would likely have a contradiction if both is given. Which one would determine the resizing algorithm of the group? I don't see (at a first glance) how this could be resolved - and it would make the resizing algorithm even more complicated. Note that neither Fl_Flex nor Fl_Grid (see below) use the resizable() widget (it's entirely ignored).

That said, besides Fl_Flex I'm still working on Fl_Grid which provides even more flexibility to the resizing of widgets inside the grid container. It's like a two-dimensional Fl_Flex or HTML <table> with options of overlapping widgets like HTML `colspan` and `rowspan` attributes. This Fl_Grid widget is also on the "Release 1.4.0" milestone and almost ready to go.

I'm sure I can resolve the "dangling pointers" issue in both widgets - and in some other core widgets. My proposal would be usable in derived classes as well. @Matthias: I sent you a proposal a while ago but I changed it meanwhile a little bit. The basics are the same though.

Many users have requested better layout widgets, and Fl_Flex has even already been integrated in fltk-rs (Rust bindings for FLTK).

Summary (my conclusion):

Fl_Flex and Fl_Grid provide the requested better layout widgets w/o touching the "standard" FLTK resizing algorithm and should be added because they are really needed. Touching Fl_Pack would open a can of worms and should be avoided.

Everything else can and should be discussed after the release of FLTK 1.4.0.

[Gonzalo Garramuño]

El 26/10/22 a las 07:21, 'melcher....@googlemail.com' via fltk.coredev
escribió:

> tl;dr Proposal to add Fl_Widget::resizable(int width_factor, int
> height_factor) and merge Fl_Flex into Fl_Pack.
>
> I was looking into how Fl_Flex differs from Fl_Pack, and apart from
> the margins, Fl_Flex adds the option the make individual children
> resizable while the rest stays fixed. So instead of introducing a new
> widget, wouldn't it be better to add this functionality to Fl_Pack?

No.  Fl_Pack resizes itself to fit the widgets and pack them together
during the redraw function.  This can be useful when you don't know (or
care for) one of the sizes in one direction.
Thanks to Greg I also have a version of Fl_Pack that has a layout
function to do the packing before hand (adding *that* to fltk1.4 would
be useful).

Fl_Flex, on the other hand, requires you to provide its size before hand
and it fits the widgets proportionally in the available space, except
when one or some of them have had the fixed set_size() set for them.

[melcher....@googlemail.com]

tl;dr : sorry for the long text. Withdrawing my first idea, I suggest we move child related data to the children and not allocate it as part of the parent, and create a standardised interface for that.

Ok, I see the points.

So changing Fl_Pack is not realistic. And since Fl_Flex is in fltk_rs, it's probably wise to keep the method names as is. Fl_Groups seems to avoid successfully to have pointers dangle, or we would know about it.

I am just very much in favour to point at something else only once, because otherwise there are just too many locations to keep track of. For example, if I delete and Widget, it may have a parent that links back to it, but it also may be resizable, so my parent may have a second pointer. Now with Fl_Flex, there may be just another pointer that tells me that my widget has a fixed size. 

This is by no means a drama, but if it is possible to avoid it easily, I would. So here are my ideas:

1. Storing relative width and height was a bad idea by me, because different Groups may want to store very different things per child. 
2. Making the Group handle an array of widget data (Fl_Group::init_size()) or widget pointers (Fl_Group::resizable()) is inherently unsafe, even if we got away with it for 30 years. But that's also part of what kept us from implementing new Groups with better resizing options like Flex and Grid.
3. Making Fl_Group::insert(), remove(), and clear() virtual would make implementing Flex and Grid easier and safe.

So I withdraw my original idea and suggest that we add a group_data pointer to every widget. This pointer is reserved for use by the current parent exclusively, and the parent can store whatever it wants. It should delete that data when removing a widget, but if it doesn't, the widget removes the data when being deleted (no leak). If it is added to a new Group, the new Group will simply replace the group_data with its own data. 

Yes, it adds a pointer to Fl_Widget, but OTOH it removes pointers and code in Fl_Group.

To have a little code, I suggest:

  class _Fl_Group_Data {

  public:

    virtual ~Fl_Group_Data();

  };

  class Fl_Widget {

    ~Fl_Widget() {

      ...

      group_data(NULL);

    }

    void group_data(_Fl_Group_Data*); // replace current group data with new group data

    _Fl_Group_Data *group_data();

  };

class Fl_Fancy_Group {

  class _Fl_Fancy_Group_Data : public _Fl_Group_Data {

    double width_factor;

    int initial_width;

    ...

  };

  void insert(FL_Widget *w) {

    w->group_data(new _Fl_Fancy_Group_Data(50.0, w->w()));

    ...

  }

  void remove(Fl_Widget *w) {

    ...

    w->group_data(NULL); // will call 'delete' for us

  }

};

[Albrecht Schlosser]

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

tl;dr : sorry for the long text. Withdrawing my first idea, I suggest we move child related data to the children and not allocate it as part of the parent, and create a standardised interface for that.

Ok, I see the points.

So changing Fl_Pack is not realistic.

OK, I'm glad you agree.

And since Fl_Flex is in fltk_rs, it's probably wise to keep the method names as is.

I would still want to keep the option to change method names as long as FLTK 1.4 is not yet released. fltk_rs could either keep the old method names in their own implementation or follow the transition. Relying on non-released software must be done on one's own risk. I did already change some method names for FLTK compliance from the original contribution (which had already been adopted before by fltk-rs) and so we can do now too.

Fl_Groups seems to avoid successfully to have pointers dangle, or we would know about it.

Maybe not. The only dangling pointer I can imagine is indeed the resizable() pointer. We might not know about it because users won't delete or remove the resizable() widget that often w/o resetting the resizable() by themselves.

I'll check this because it would be a bug.

I am just very much in favour to point at something else only once, because otherwise there are just too many locations to keep track of. For example, if I delete and Widget, it may have a parent that links back to it, but it also may be resizable, so my parent may have a second pointer.

Yep, but that's not bad by itself. The parent could also store the index of the resizable child but that's probably much more error prone. But somehow the information must be stored.

Now with Fl_Flex, there may be just another pointer that tells me that my widget has a fixed size.

This is IMHO unavoidable unless you keep a structure for each child widget that contains a pointer and more attributes. However, that structure would have to reserve special purpose attributes for every container class that may own the child widget which is obviously not easily doable. I believe that every container (group) widget must store its own per-widget data in its own structure(s).

Hmm, well, after I wrote this and thought more about your group_data approach (below), I think it is possible by subclassing the group_data structure for every clase (as a subclass of its parent's struct/class) but this begins to become more and more complicated.

This is by no means a drama, but if it is possible to avoid it easily, I would. So here are my ideas:

I generally agree (it should be avoided), but emphasis is on both "possible" and "easy".

1. Storing relative width and height was a bad idea by me, because different Groups may want to store very different things per child.

I agree.

1. Making the Group handle an array of widget data (Fl_Group::init_size()) or widget pointers (Fl_Group::resizable()) is inherently unsafe, even if we got away with it for 30 years. But that's also part of what kept us from implementing new Groups with better resizing options like Flex and Grid.

I can't argue against "inherently unsafe". Yes, it needs some rules but I don't see a better and "easy" solution yet.

I would however not agree that this "kept us from implementing new Groups with better resizing options". Fl_Flex and Fl_Grid show that it is possible but it's just a fact that nobody did it.

1. Making Fl_Group::insert(), remove(), and clear() virtual would make implementing Flex and Grid easier and safe.

Unfortunately it's too late to do this as I wrote before. Inconsistent method naming in derived classes prevents this.

I would be glad to see that I'm wrong with this, but that's what I found out so far. I could dig in my notes, maybe I can find some info - although I dropped that idea.

So I withdraw my original idea and suggest that we add a group_data pointer to every widget. This pointer is reserved for use by the current parent exclusively, and the parent can store whatever it wants.

Hmm, this doesn't seem very "easy". Imagine Fl_Flex, a group that stores its own data in its own structure (lets say: _Fl_Flex_Group_Data) similar to your example code below. That's fine.

But what if you derive another class (FancyFlex) from Fl_Flex that wants to store its own data in another structure that is linked by the one and only per-widget group_data pointer? If this derived class used its own structure to store the data it would destroy the data stored by its base class Fl_Flex. OK, the derived class FancyFlex could also subclass _Fl_Flex_Group_Data as _FancyFlex_Group_Data and could use additional members in _FancyFlex_Group_Data etc. etc. But that doesn't sound "easy", does it?

It should delete that data when removing a widget, but if it doesn't, the widget removes the data when being deleted (no leak). If it is added to a new Group, the new Group will simply replace the group_data with its own data. 

Yes, it adds a pointer to Fl_Widget, but OTOH it removes pointers and code in Fl_Group.

I'm not sure that the latter (removing pointers and code) would be the case.

Let's talk about the resizable() widget pointer. It simplifies the standard resizing algorithm by providing direct access to the single resizable() widget that affects the resizing behavior of all its children. If this information was stored in every child's group_data structure, the resizing algorithm would have to search the child array for the resizable child - or _all_ resizable children (?) - before the calculation can begin. This costs CPU time and doesn't help much. Yes, it removes one pointer from Fl_Group but it *adds* code to the standard Fl_Group class and costs likely a performance penalty.

To have a little code, I suggest:

  class _Fl_Group_Data {

  public:

    virtual ~Fl_Group_Data();

  };

  class Fl_Widget {

    ~Fl_Widget() {

      ...

      group_data(NULL);

    }

    void group_data(_Fl_Group_Data*); // replace current group data with new group data

    _Fl_Group_Data *group_data();

  };

class Fl_Fancy_Group {

  class _Fl_Fancy_Group_Data : public _Fl_Group_Data {

    double width_factor;

    int initial_width;

    ...

  };

  void insert(FL_Widget *w) {

    w->group_data(new _Fl_Fancy_Group_Data(50.0, w->w()));

    ...

  }

  void remove(Fl_Widget *w) {

    ...

    w->group_data(NULL); // will call 'delete' for us

  }

};

This looks generally like a good idea.

However, the statement above:

  `w->group_data(new _Fl_Fancy_Group_Data(50.0, w->w()));`

is IMHO an oversimplification. It assumes that the group (class) can allocate a new group data structure of its own and that all parent classes can be initialized with default constructors (in this example only _Fl_Group_Data). But how can this be achieved for all nested classes of deeply subclassed group data structs?

[Potential answer: add the existing group_data pointer of the parent class to the constructor?]


Please don't misunderstand me, these are only some thoughts I had when reading your proposal. I believe it's worth thinking about it and maybe implement it - if we can solve the open questions and issues.


Thank you very much for this proposal, I believe it's worth investigating, but as (you also) said before, we need to get 1.4.0 released soon and such big changes can't be done before that. However, I believe that changes like these are only internal implementation details and could be done in the next minor release (maybe 1.5.0 or whatever) where we can break the ABI w/o issues and lots of ABI guards in the code. Even if Fl_Flex and Fl_Grid do their work differently in 1.4.x.

PS: Matthias, it's great to see you "back" and active again!

[Albrecht Schlosser]

On 10/27/22 18:19 Albrecht Schlosser wrote:

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

Fl_Groups seems to avoid successfully to have pointers dangle, or we would know about it.

Maybe not. The only dangling pointer I can imagine is indeed the resizable() pointer. We might not know about it because users won't delete or remove the resizable() widget that often w/o resetting the resizable() by themselves.

I'll check this because it would be a bug.

For your information: checked, found bug, fixed it.

It was just one statement plus some documentation which I added in commit a91829254.

Known issue: you can set *any* widget as the resizable() of a group and we can't check it because the widget might be added later - as in the FLTK core in FL/Fl_Group.H:

  /**
    Adds a widget to the group and makes it the resizable widget.
  */
  void add_resizable(Fl_Widget& o) {resizable_ = &o; add(o);}

Note that the widget is added after it is set as the resizable. In this case it doesn't matter (internal, atomic operation) but checking it in the user interface, i.e. in Fl_Group::resizable() would potentially break programs that first set the resizable() and then add it to the group as above.

Thus I documented that setting any other (non-child) widget as the resizable() results in undefined behavior.

[Bill Spitzak]

I'm not sure that is a problem. If the resizable is not a child the resizing will screw up because it assumes the resizable's size is changed and it would not be in that case. So the user will have to add the child before the program is usable. The implementation of add_resizable is not really relevant, and I'm pretty certain it was written that way because (at the time) chaining to another function as the last statement was always more efficient than calling it first. It may even be that this could better deal with add() having some strange side-effect such as deleting the widget.

--

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/714c2488-0d0a-0cd4-86b1-58e7ba440f66%40online.de.

[Mohammed]

The current 1.4 implementation of Fl_Flex seems to default to a column. Whereas osen/Fl_Flex defaults to a row, similar to what you would get with the web’s flexbox. I wonder if this was changed for Fl_Flex to be a drop-in replacement for Fl_Pack.

Sent from my iPhone

[Albrecht Schlosser]

On 10/28/22 19:22 Mohammed wrote:
> The current 1.4 implementation of Fl_Flex seems to default to a column. Whereas osen/Fl_Flex defaults to a row, similar to what you would get with the web’s flexbox. I wonder if this was changed for Fl_Flex to be a drop-in replacement for Fl_Pack.

Off the top of my head: the answer is yes, ISTR that this was the
reason. Faint memory, but I'm pretty sure.

[Albrecht Schlosser]

On 10/28/22 19:20 Bill Spitzak wrote:

I'm not sure that is a problem. If the resizable is not a child the resizing will screw up because it assumes the resizable's size is changed and it would not be in that case. So the user will have to add the child before the program is usable.

The problem is if the resizable widget is the child of another group. If it overlaps with the group in question it may affect resizing, if not, resizing may screw up as you wrote. That's what I call "undefined behavior".

Since FLTK does not check that the resizable() widget is a child of the group a faulty user program can set one widget as the resizable() widget of multiple groups.

The implementation of add_resizable is not really relevant, and I'm pretty certain it was written that way because ...

Yes, I agree that this implementation is not relevant. I phrased this: "it doesn't matter (internal, atomic operation)".

OK, that is clear. The problem is: if any widget is deleted it is automatically removed from its parent group and the fix in commit a91829254 makes sure that - if this child was the resizable of its parent - the resizable() pointer of that parent group is reset to the group itself so there is no dangling pointer.

However, if the widget was set as the resizable() of *another group* (not its parent, let's say group A), then we can't reset the resizable() of that group A and a dangling pointer is left. Resizing that group with the invalid resizable() widget may or may not crash the application (it's at least an access to released data). Again: "undefined behavior".

On Fri, Oct 28, 2022 at 9:19 AM Albrecht Schlosser <Albrech...@online.de> wrote:

On 10/27/22 18:19 Albrecht Schlosser wrote:

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

Fl_Groups seems to avoid successfully to have pointers dangle, or we would know about it.

Maybe not. The only dangling pointer I can imagine is indeed the resizable() pointer. We might not know about it because users won't delete or remove the resizable() widget that often w/o resetting the resizable() by themselves.

I'll check this because it would be a bug.

For your information: checked, found bug, fixed it.

It was just one statement plus some documentation which I added in commit a91829254.

Known issue: you can set *any* widget as the resizable() of a group and we can't check it because the widget might be added later - as in the FLTK core in FL/Fl_Group.H:

  /**
    Adds a widget to the group and makes it the resizable widget.
  */
  void add_resizable(Fl_Widget& o) {resizable_ = &o; add(o);}

Note that the widget is added after it is set as the resizable. In this case it doesn't matter (internal, atomic operation) but checking it in the user interface, i.e. in Fl_Group::resizable() would potentially break programs that first set the resizable() and then add it to the group as above.

Thus I documented that setting any other (non-child) widget as the resizable() results in undefined behavior.

--
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/714c2488-0d0a-0cd4-86b1-58e7ba440f66%40online.de.

--
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/CAL-8oAg4%2BU4Q0dch9tcJW5PJ02fLJm3BnFARB0BWrX9Z12_wKg%40mail.gmail.com.

[Albrecht Schlosser]

It's documented: "The default value is Fl_Flex::VERTICAL for consistency
with Fl_Pack but you can use \p type() to assign a row
(Fl_Flex::HORIZONTAL) layout."

[melcher....@googlemail.com]

Wow, thanks for actually checking my suspicion about this. You found an even bigger issue ;-) .