| | [ Return to Bugs & Features | SVN ⇄ GIT ]
STR #3385
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 1 - Request for Enhancement, e.g. asking for a feature |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Introduce new classes Fl_Rect and Fl_Rect2 |
|---|
| Version: | 1.4-feature |
|---|
| Created By: | AlbrechtS |
|---|
| Assigned To: | AlbrechtS |
|---|
| Fix Version: | 1.4.0 (SVN: v12302) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | AlbrechtS
10:57 May 31, 2017 |
| I propose to add one or two new classes to FLTK:
(1) class Fl_Rect
(2) class Fl_Rect2 (less important).
Class Fl_Rect may be used for overloaded widget constructors and other overloaded methods like fl_measure(), fl_text_extents(), fl_rect(), fl_rectf(), and maybe more internal functions. There are many internal functions that could be simplified if only one struct was passed instead of four arguments (X, Y, W, H). See also documentation in attached file Fl_Rect.H (not yet ready).
Fl_Rect.H may be incomplete and contains class Fl_Rect2 as well which may be considered optional (see below).
Class Fl_Rect2 (its name may be changed, I didn't find a better one) describes the layout of the internal sizes() array of Fl_Group. The protected method sizes() returns a currently undocumented array of int's (int*) that describes the stored sizes of the Fl_Group widget itself, its resizable() widget, and of all children. Since derived classes can access this array it should IMHO be documented. I propose to create new methods to access the stored array in a documented way for developers of derived classes. Currently it is possible to cast the returned array (int*) to an Fl_Rect2* and access the array through this pointer as done in the attached demo program rect.cxx. I believe that this is standard conformant, although there are alignment questions to deal with.
Class Fl_Rect contains x, y, w, h (standard FLTK coordinates and sizes).
Class Fl_Rect2 (unfortunately; as defined by Fl_Group) contains left, right, top, and bottom coordinates (in this order). I suggest not to change this order and contents for backwards compatibility (derived classes), but we can use Fl_Rect2 for accessor methods and documentation. | |
| |
| #2 | AlbrechtS
11:05 May 31, 2017 |
| Attached file rect.cxx can be used to test the proposed classes and access to the stored sizes() array using class Fl_Rect2.
Two classes MyWindow and MyButton show overloaded constructors that use class Fl_Rect to simplify argument lists. This is just for convenience, but such overloaded constructors might also be added to the FLTK core widgets.
Instructions:
(1) save Fl_Rect.H as FL/Fl_Rect.H in your FLTK working directory
(2) `fltk-config --compile rect.cxx'
(3) ./rect
The class implementation is entirely contained in FL/Fl_Rect.H (this may be changed later). | |
| |
| #3 | AlbrechtS
11:22 May 31, 2017 |
| Please see also discussion in fltk.coredev, thread "Introduce new classes Fl_Rect and Fl_Rect2", opened today (2017-05-31), and vote. Thanks. | |
| |
| #4 | manolo
02:04 Jun 12, 2017 |
| About improving the int* Fl_Group::sizes() member function:
an approach would be to define a new, preferred, API
Fl_Rect* Fl_Group::bounds()
that would be used by FLTK internally, that is, in
Fl_Group.cxx and Fl_Tile.cxx
and to maintain int* Fl_Group::sizes() just for
backward compatibility with putative existing source code.
This is implemented in attached group_bounds.patch | |
| |
| #5 | AlbrechtS
05:01 Jun 12, 2017 |
| Thanks for the comment and the patch.
Re #4: I'd really like to do it with this new internal Fl_Rect array instead of the int array used by sizes(). Code readability would be improved, and some of the code would also be shorter. I had already implemented a similar patch for Fl_Tile with the new bounds() method.
Unfortunately the patch, as given, would change the API in a way that would open memory leaks in existing programs that are just recompiled for new FLTK versions (1.4) because they won't know of the necessary delete[] after using sizes().
Programs "corrected" for FLTK 1.4 might even crash if recompiled for FLTK 1.3, unless the necessary delete[] operator would be guarded by FLTK version checks. The latter could be documented, so this wouldn't be a real issue.
A possible way out would be to _keep_ the sizes_ pointer additional to Fl_Rect *bounds_ in Fl_Group so we can control the deletion of the (old) sizes_ array as we do with bounds_, but that would increase Fl_Group's size by one pointer size, i.e. 4 or 8 bytes depending on the platform.
If we went this way we could deprecate sizes() and remove the method and the additional pointer in a later FLTK version. We could even remove the sizes() method and pointer with a configure/CMake option in FLTK 1.4 (not default, of course) for those that compile FLTK for their own use only. Linux distros would use the default and still have sizes() etc.
Question: should we also rename init_sizes() to init_bounds() for consistency and keep (add) init_sizes() as an alias for compatibility? Note that init_sizes() is public and certainly used frequently in existing code.
The longer I think about it, the more appealing this seems to be to me. If we'd vote I'd likely vote +1 for this approach.
Technical notes (just to not forget in case the patch would be used):
(1) The patch calls bounds() repeatedly in the new implementation of sizes(). I suggest to call bounds() only once and use bounds_ (or a local variable) later for optimization.
(2) The patch adds:
+ //Fl_Rect* p = bounds_ = new Fl_Rect[children_+2]; // this repeatedly calls the Fl_Rect constructor
+ Fl_Rect* p = bounds_ = (Fl_Rect*)new char[(children_+2)*sizeof(Fl_Rect)];
I'm not sure if the proposed 'new char[(children_+2)*sizeof(Fl_Rect)]' might potentially produce alignment issues. Admittedly, the default constructor would initialize four int's to zero, but I believe this is neglectible, so I'd rather suggest to use 'new Fl_Rect[children_+2]'. | |
| |
| #6 | manolo
00:50 Jun 13, 2017 |
| Re: int* Fl_Group::sizes()
Yes, I believe this partially undocumented member function
(because the content of the returned array was not documented)
could be deprecated, and removed in a later step.
This will oblige to keep two pointers in each Fl_Group object,
sizes_ and bounds_, to avoid memory leaks.
I see now there are 2 other calls to Fl_Group::sizes() in
Fl_Tile.cxx, which should be similarly converted to calls
to Fl_Group::bounds().
Re: how to construct an array of Fl_Rect objects
This statement
Fl_Rect* p = bounds_ = new Fl_Rect[children_+2];
calls (children_+2) times the Fl_Rect constructor which sets each
object member variable to 0.
Defining two Fl_Rect constructors, with and without arguments
Fl_Rect(int x, int y, int w, int h) : x_(x), y_(y), w_(w), h_(h) {}
Fl_Rect() {}
may be the way to go. | |
| |
| #7 | manolo
01:05 Jun 13, 2017 |
| I had not seen the new FL/Fl_Rect.H file
My proposal for this constructor
Fl_Rect() {}
is not possible. I agree that it's safer to construct
an array of Fl_Rect objects with new Fl_Rect[number]
I would suggest to remove the copy constructor for the following reason,
taken from http://www.cplusplus.com/articles/y8hv0pDG/ :
When do I need to write a copy constructor?
First, you should understand that if you do not declare a copy
constructor, the compiler gives you one implicitly. The implicit
copy constructor does a member-wise copy of the source object.
Thus, the implicit copy constructor is enough for Fl_Rect objects. | |
| |
| #8 | AlbrechtS
03:43 Jun 13, 2017 |
| Copy constructor removed as suggested (svn r12260).
I was working on a complete patch yesterday, but I did not yet post it here because it was not yet ready. I saw and fixed the other calls of Fl_Group::sizes() in Fl_Tile.cxx and made some more changes. This is a proposal based on your changes (Fl_Group::bounds()) and my comments.
I'll post the complete patch later today (I'm busy with something else right now), so please wait and don't waste your time updating your patch. | |
| |
| #9 | AlbrechtS
09:34 Jun 13, 2017 |
| Uploaded file group_bounds_v2.patch is my modified patch proposal (use patch -p1 with svn r12260 or later).
Changes:
- both bounds_ and sizes_ variables, no need to delete[] array
returned by sizes()
- sizes() always delete[]'s the array and creates a new one [1]
- Fl_Tile patch completed (all occurrences of sizes()
replaced with bounds())
- some minor other changes, e.g. replaced 'type()<FL_WINDOW'
with '!as_window()' etc.
[1] Do we really need to delete[] sizes_ always if it is not NULL? Maybe we can do the same as we do with bounds_, i.e. don't allocate a new array if it exists, but I'm not sure. Needs code review to decide.
I tested the code with test/tile and it seems to work, but maybe we need more testing with 'old' code that uses sizes()...
Looking forward to your comments and test results... | |
| |
| #10 | AlbrechtS
08:44 Jun 21, 2017 |
| The new file group_bounds_v3.patch is (AFAICT) an update to the previous patch:
- use "better" Fl_Rect constructors in Fl_Group::bounds()
- do not delete[] sizes_ in sizes() if it is not NULL
- simplify some code
- complete documentation
This is tested well, even with old code that uses sizes() [1]. It is not necessary to delete[] the sizes_ array and recreated it if sizes() is called and the sizes_ array exists (sizes_ != NULL).
[1] I used the old version of Fl_Tile that used to call sizes() and work with the old int* array. | |
| |
| #11 | AlbrechtS
08:46 Jun 21, 2017 |
| Summary / Conclusion:
I propose to commit this update. It adds the method Fl_Group::bounds() and is otherwise backwards compatible: Fl_Group::sizes() is retained, but now deprecated. The new class Fl_Rect has already been committed previously.
The only drawback is that we need one additional pointer in Fl_Group until Fl_Group::sizes() can be removed (likely in 1.6.0 or higher). If requested we could add a configure/CMake option to remove Fl_Group::sizes() and the additional pointer as an ABI breaking feature for users that compile their own library version and want to save some address space. | |
| |
| #12 | AlbrechtS
12:22 Jul 07, 2017 |
| Fixed in Subversion repository.
Contrary to the title of this RFE only one new class (Fl_Rect) was added.
Fl_Rect2 or Fl_Bounds, as discussed, were not necessary. | |
[ Return to Bugs & Features ]
|
| |
