Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fluid support for Fl_Flex #523

Merged
merged 9 commits into from
Nov 1, 2022
Merged

Fluid support for Fl_Flex #523

merged 9 commits into from
Nov 1, 2022

Conversation

MatthiasWM
Copy link
Contributor

This PR adds Fl_Flex support for Fluid (90% done). There are two bugs fixes for Fl_Flex::set_size(Fl_Widget*, int) that should be applied to master in any case.

Features:

  • new icon and menu entry
  • code generation, all settings saved in .fl file
  • additional settings in Widget dialog per Fl_Flex and per child of Fl_Flex

Fl_Flex has a new line of parameters for setting the gap and margins:
Screenshot 2022-10-25 at 01 08 19

Children of Fl_Flex will be positioned or sized by Fl_Flex, so the size line is replace by a single Flex Size field. Enter a valid integer, and the Widget will be fixed in that height or width (vertical or horizontal Flex). Enter zero, and Fl_Flex will automatically resize the widget:
Screenshot 2022-10-25 at 01 08 37

Fixes:

  • Fl_Flex::set_size would not move the last entry when deleting a previous entry (off-by-one)
  • Fl_Flex::set_size would continue adding the same child when called multiple times, even if the child was already in the array

@MatthiasWM MatthiasWM added fluid Fast Light User Interface Designer (fluid) active Somebody is working on it labels Oct 24, 2022
@MatthiasWM MatthiasWM added this to the Release FLTK 1.4.0 milestone Oct 24, 2022
@Albrecht-S
Copy link
Member

@MatthiasWM I would like to work further on Fl_Flex and if you don't mind pick your fixes and push them in my next commit. You would have to rebase and merge your own branch then. Would it be OK for you if I committed your fixes?

OTOH, how far is your fluid support? I won't be able to test much today, I'll try to find some time tomorrow. Would you be ready to merge the fluid support part soon (together with your fixes)?

@Albrecht-S
Copy link
Member

@MatthiasWM FYI: I added two comments to the Fl_Flex diffs. I didn't review the fluid part in any way - and I'm not sure if I will understand the code but I'll try to test it to get a feeling how it works.

Thank you very much for adding fluid support of Fl_Flex which covers something I would likely not have been able to do without investing much more time.

@Albrecht-S
Copy link
Member

@MatthiasWM Another question: you moved the resize statement

  // if w is meant to be fixed, set its new size
  w->resize(0, 0, size, size);

(and added the comment) to an if clause where it is only executed "if w is meant to be fixed". The original code did this always, right at the beginning, which I didn't change (I only added the check to prevent size < 0). Is there a specific reason to change this? I'm asking because I'd like to understand this and because there's PR #518 that wants to change this statement (in its original position) to w->size(size, size); because it "could lead to drawing issues when widgets were hidden and shown later".

Maybe these two issues are related?

@Albrecht-S
Copy link
Member

Hi @MatthiasWM, I took the freedom to fix the formatting issue in src/Fl_Flex.cxx (see one of my comments) and pushed it to your branch.

@Albrecht-S
Copy link
Member

@MatthiasWM I also tested the new fluid from your branch and created a working demo layout with one horizontal and one vertical Fl_Flex widget, a few buttons, and a working quit button.

Everything went smoothly except handling of fixed and non-fixed widgets. The tooltip of the "Flex Parent" size field says "... or 0 to make it flexible" children but there's also the check button "fixed" and have a size greater than 0 but still flexible if the check button is off. This is not a bug but a little bit unintuitive.

For the record: here is the example fluid file I created: flex1.fl.

@Albrecht-S
Copy link
Member

Note that I also noticed some new compiler warnings when building fluid:

$ ninja fluid
[206/221] Building CXX object fluid/CMakeFiles/fluid.dir/Fl_Group_Type.cxx.o
../../fluid/Fl_Group_Type.cxx: In member function ‘virtual void Fl_Flex_Type::copy_properties()’:
../../fluid/Fl_Group_Type.cxx:196:23: warning: unused variable ‘gp’ [-Wunused-variable]
  196 |   int lm, tm, rm, bm, gp;
      |                       ^~
../../fluid/Fl_Group_Type.cxx: In member function ‘void Fl_Flex_Type::change_subtype_to(int)’:
../../fluid/Fl_Group_Type.cxx:316:7: warning: unused variable ‘dx’ [-Wunused-variable]
  316 |   int dx = Fl::box_dx(f->box());
      |       ^~
../../fluid/Fl_Group_Type.cxx:317:7: warning: unused variable ‘dy’ [-Wunused-variable]
  317 |   int dy = Fl::box_dy(f->box());
      |       ^~
[221/221] Linking CXX executable bin/fluid

These should be fixed and then I believe that this PR can be merged. Thank you very much for fluid support of Fl_Flex!

@MatthiasWM
Copy link
Contributor Author

The Fluid part uses a lot of trickery because, well, Fluid is just not very clean to begin with. I have done several attempts to clean things up. The funny thing is, the more I clean it up, the more it seems to grow (in number of files), but it's actually just the unravelling of the very densely packed source files. For the time being, Fluid works. I will every once in a while go back into it and clean up a bit here and there.

As for the resize statement: I moved the resize() to the end of set_size(), because set_size(w, 0) is the only way to remove a widget from the lookup array, but if the widget is already deleted, resize() will crash (this can happen when FLTK eagerly delete all children of a widget while deleting the group widget, but I forgot the details). resize() does not serve much of a purpose in this general format here anyway. I will change that, so it also satisfies PR #518:

  // if the child size is meant to be fixed, set its new size
  if (horizontal())
    child->size(size, h()-margin_top_-margin_bottom_-Fl::box_dh(box()));
  else
    child->size(w()-margin_left_-margin_right_-Fl::box_dw(box()), size);

@MatthiasWM
Copy link
Contributor Author

As for Fluid and the size / check button combo: I did change my original code and forgot to update the tooltips. The size always shows the current size of the widget, and the check box indicates if it is fixed. I don't like the association that 0 means not fixed.

Also, if you drag the edge of one of the children of Fl_Flex, the size entry in Fluid changes with it, and the widget is marked as "fixed".

@MatthiasWM
Copy link
Contributor Author

As for the unused variables, I removed them. I guess I should also update the warnings flags for Xcode in CMake.

@MatthiasWM MatthiasWM marked this pull request as ready for review November 1, 2022 09:39
Copy link
Member

@Albrecht-S Albrecht-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, IMHO.
Please rebase (if necessary) and squash to merge. Thanks.

PS: tried to make a formally correct review, still learning "GitHub".

@Albrecht-S
Copy link
Member

Thanks for fixing the remaining issues. Please select "Squash and merge" to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
active Somebody is working on it fluid Fast Light User Interface Designer (fluid)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants