| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #1895
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 2 - Low, e.g. a documentation error or undocumented side-effect |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Fl_Scroll does its scrollbar management only in its draw() |
|---|
| Version: | 1.3-current |
|---|
| Created By: | alvin |
|---|
| Assigned To: | greg.ercolano |
|---|
| Fix Version: | 1.3-current (SVN: v10220) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | alvin
10:49 Mar 12, 2008 |
| First off, I marked this STR as LOW only because I believe this is a undocumented side-effect.
Fl_Scroll only updates the state of its scrollbars in its draw() function. Though this seems to be fine for Fl_Scroll, it does cause some headaches when deriving a new widget based on Fl_Scroll.
For example, I was writing a scroll widget that centres its children within the viewable area. To do this, I override resize() and used Fl_Scroll::bbox() to obtain the new viewable area (after calling Fl_Scroll::resize()). I then use the viewable area to calculate where to reposition each child. This mostly worked but would often break.
The problem was that Fl_Scroll::bbox() uses the state of the scrollbars to calculate and return the viewable bounding box. However, Fl_Scroll only updates its scrollbars during a redraw(). Therefore, it is impossible for a derived class to know the true viewable area during a call to resize().
The workaround is instead, to override the draw() method and do the widget centring after calling the parent Fl_Scroll::draw(). I believe this to be counter-intuitive mostly because, AFAICT, this behaviour is not mentioned in the docs.
One possible solution is to refactor Fl_Scroll::draw() into a (1) drawing component and (2) scrollbar managing component. The scrollbar managing code could then be placed into its own protected function. This would allow derived classes the ability to force Fl_Scroll to manage its scrollbars when it desires. In my example, the widget centring logic could be placed in the overridden resize() method rather than having it executed each time the widget is redrawn. | |
| |
| #2 | matt
12:26 Mar 12, 2008 |
| Yes, your observations are absolutely right. Fl_Scroll is flawed in the way it handles scroll bars as the children of the widget, ropositioning them in the hierarchy, thereby creating sideeffects, including the mix and melt of scrollbar calculations and drawing routines.
But please bear in mind that thises are quite complex and internal conceptual issues that have been around since the beginning of FLTK. I find it much more useful to fix these issues when we advance to the next majo release cycle, because users will be much more aware of possible internal changes.
Up'd to 1.2 . | |
| |
| #3 | alvin
12:33 Mar 12, 2008 |
| I agree. However, would it be possible to add a function such as Fl_Scroll::init_scrollbars() that would do the samething that is in draw()? Basically, it would allow derived classes to force scrollbar managing to happen. And as a new function, would only effect code that calls it and be ignored by older applications? I'm not sure if this would break the ABI though? | |
| |
| #4 | matt
13:00 Mar 12, 2008 |
| Which part of FL_Scroll::draw() exactly would you put in there? | |
| |
| #5 | AlbrechtS
14:58 Mar 12, 2008 |
| If I may answer for Alvin: The part that does the calculation, whether scrollbars are needed or not, and that activates and deactivates the scrollbars.
In short: everything that would be needed to call bbox() and get a correct result depending on the new size of this Fl_Scroll widget in a derived Widget's resize() method (after calling Fl_Scroll::resize()).
Thus, Fl_Scroll::resize() should also call init_scrollbars to manage the appearance and maybe position of the scrollbars before (or after) calling Fl_Widget::resize().
Basically the part from " // accumulate bounding box of children:" over "// turn the scrollbars on and off as necessary:" ... until ... but not including "// draw the scrollbars:".
Albrecht | |
| |
| #6 | alvin
05:12 Mar 13, 2008 |
| I have some time this morning, so I can try to make a patch and a trivial test program. For the test program I was thinking of a derivied class from Fl_Scroll that always centred its children. I believe there are test programs already in test/ that could be used to test the standard behaviour of the patch.
Just a few questions:
1) Is 'diff -u' sufficient for creating the patch file?
2) Would it be better if I were to only implement Fl_Scroll::init_scrollbars() as an additional function OR integrate the use of init_scrollbars() throughout Fl_Scroll?
3) Is there a more appropriate name other than init_scrollbars() the would be better? I based the name off of Fl_Group::init_sizes(). | |
| |
| #7 | alvin
08:43 Mar 13, 2008 |
| I have created Proof of Concept, non-invassive patch for Fl_Scroll. It only adds Fl_Scroll::init_scrollbars() and doesn't touch anything else. If Fl_Scroll::init_scrollbars() is never called, then Fl_Scroll will behave as it always has.
I have also created a test program that can be used to see the effects of the scrollbars with and without the patch applied.
The patch is very trivial. I simply copied the relevant portion that Albrecht identified from Fl_Scroll::draw() and pasted it into its own function, Fl_Scroll::init_scrollbars(). I did have to remove the calls to draw_clip() has they resulted in a segfault and the usage of 'd' was redundant in this context. I believe the segfault occurs because the block of code is not being called in the context of drawing.
If this patch is ok, I am willing to clean it up and fully integrate it into Fl_Scroll. By integrate, I mean that there are places that Fl_Scroll should call this such as:
1. Fl_Scroll::end() [doesn't exist] should call init_scrollbars() so that the scrollbars are adjusted after all the widgets are added.
2. Fl_Scroll::add() should call init_scrollbars() for the same reason.
3. Fl_Scroll::init_sizes() should call init_scrollbars() as well. If I understand what init_sizes() is suppose to do: When widgets are resized outside of the parent Fl_Group, init_sizes() should be called in order to let the parent Fl_Group (Fl_Scroll in this case) know about it? Therefore, calling init_scrollbars() would ensure that Fl_Scroll::bbox() would return the new bbox.
As an aside, fix_scrollbar_order() should be call by Fl_Scroll::end() and Fl_Scroll::add() as well. During my testing of init_scrollbars() there was one bug that the children bbox loop was wrong since after calling end() the scrollbars were actually the first children as opposed to being the last children.
There are two ways to compile the test app:
1) fltk-config --use-images --compile str1895_test.cxx
This will compile the app without the calls to Fl_Scroll::init_scrollbars(). When you launch the app, you will see a gap between tux and the right hand side. This happens because when end() is called, the Fl::bbox() thinks that the scrollbars are still visible (i.e. Fl_Scroll::draw() hasn't been called yet).
Then when you Increase and Decrease the size of Tux (The lower buttons), you will see the effects such as Tux being behind the vertical scrollbar and the gaps.
2) fltk-config -DHAVE_STR1895 --use-images --compile str1895_test.cxx
This will compile the app with the calls to Fl_Scroll::init_scrollbars(). When you start the app, you will see Tux is flush against the right hand side. As you Increase and Decrease Tux, he should always be flush against the right side or flush against the vertical scrollbar when it appears.
Like I said, if this patch is a "good thing", then I am willing to take it to the next step if you like.
Alvin | |
| |
| #8 | alvin
09:01 Mar 14, 2008 |
| I have prepared version 2 of the str1895 patch. This patch does the following to Fl_Scroll:
1) Overrides end(), add() and init_sizes() to call init_scrollbars(). This ensures that Fl_Scroll::bbox() will always return the correct position and dimensions.
2) Added the call to init_scrollbars() in Fl_Scroll::resize() after the base widget is resized.
3) Updated the .html documentation for Fl_Scroll. I added an entry for bbox() and init_sizes().
These allow for Fl_Scroll::bbox() to return the current viewable area which is mainly important for derived widgets. Having these overridden functions make it not necessary for derived widgets to have to call init_scrollbars() they can trust the return values of bbox().
The Fl_Scroll::draw() remains untouched. Ideally, Fl_Scroll::draw() should call init_scrollbars() so that the scrollbar logic is in one place instead of duplicated. But that optimisation would require more debugging an testing.
One question though, in Fl_Scroll::resize(), why is the Fl_Widget::resize() called instead of Fl_Group::resize()? | |
| |
| #9 | AlbrechtS
09:55 Mar 14, 2008 |
| Alvin wrote:
One question though, in Fl_Scroll::resize(), why is the Fl_Widget::resize() called instead of Fl_Group::resize()?
Probably to prevent the scrollbars from resizing. | |
| |
| #10 | matt
13:10 Mar 14, 2008 |
| I will be on a short vacation starting Monday. I will try to look at the patch before that and then decide if we do an RC3 with the patch or a final 1.1.8 without your patch. Thanks for looking into fixing all that. | |
| |
| #11 | alvin
05:37 Mar 15, 2008 |
| I'm no longer fully confident that the patch does what I intended. I'm currently debugging it now.
AFAIK, no one has ever said anything before about this behaviour in Fl_Scroll before. If this is true, then if you want to push out 1.1.8 before I'm done debugging, than I have no problems with that.
Once I am confident that this patch is in fact a good thing, then I will post here. If it takes me too long, then perhaps this patch can be applied after 1.1.8? | |
| |
| #12 | greg.ercolano
18:09 Apr 02, 2009 |
| Revisiting this STR while implementing scrollbar_size() before 1.3.0.
If a public method for scrollbar recalculation would be desirable, I'll see if I can make that part of this change. | |
| |
| #13 | AlbrechtS
09:14 Apr 03, 2009 |
| Maybe not a public method, but protected? I looked at Alvin's patch files, and I think that his init_scrollbars() method in the first patch version (with fix_scrollbar_order) might be a good start.
If it simplifies adding scrollbar_size(), then I suggest to start with it. Separating drawing and scrollbar reordering could be a first step...
BTW.: Here we have a widget that exposes its scrollbars, and therefore it is possible that existing user code modifies the scrollbar widths by accessing the scrollbars directly. I'm not sure if this could have consequences for the scrollbar_size() method. What do you think about this? | |
| |
| #14 | greg.ercolano
11:28 Apr 03, 2009 |
| > BTW.: Here we have a widget that exposes its scrollbars,
> and therefore it is possible that existing user code modifies
> the scrollbar widths by accessing the scrollbars directly.
> I'm not sure if this could have consequences for the
> scrollbar_size() method. What do you think about this?
Yes, it does seem like the scrollbar's size is being preserved,
even though its recomputed during the draw().
Would need some discussion -- I'll move that question over to
fltk.dev so we can discuss it there, and follow up here with results. | |
| |
| #15 | kdiman
03:54 Jun 15, 2014 |
| Hi guys !
I did patch for correct resizing Fl_Scroll,
while the scrollbars are having value() > 0.
After applying the patch drawing children will be
correct, with scrolling them when the (h)scrollbar.value() > 0
{
For reproduction the problem: run test/scroll programm,
and drag handler of scrollbar to the end,
then try enlarge the window ...
}
Have a nice day. | |
| |
| #16 | kdiman
07:56 Jun 29, 2014 |
| I have attached new patch for scroll contents of the Fl_Scroll group (by resizing the group, - hidding a scrollbar ...).
FIXME Sorry but my solution is working for FL_ALIGN_RIGHT and FL_ALIGN_BOTTOM only!
Have a nice day. | |
| |
| #17 | greg.ercolano
10:41 Jun 29, 2014 |
| Perhaps in some way the issues raised by the "Summary"
and comments #3 thru #5 were addressed with svn r6828
which added the "private" method recalc_scrollbars().
Perhaps this can be changed to being 'protected'
to actually solve this STR. Need to check Alvin's patches
to make sure we're not missing anything he was working towards.
kdiman, looks like you've added some interesting patches for a
longstanding resize issue described in comment #15.
But I think that's a different enough issue from this STR
to warrant being a separate STR of its own.
If you agree, could you open a new STR with your patches?
It's good though that your additions here have brought new attention
to this STR, which probably can be solved with a few simple mods to
changes that have already been applied to SVN. | |
| |
| #18 | alvin
07:51 Jun 30, 2014 |
| I agree with Greg in that kdiman's patches should go in a new STR. Seems somewhat out of place for this STR.
I had forgotten all about this STR! I have a few patches that I apply to my build of FLTK and it appears this patch has been removed from that set.
Having reviewed my svn logs from 2008, looks like I worked around this issue by manually determining the viewable size in my derived class.
Having reviewed the Fl_Scroll source, I think, as Greg has said, if recalc_scrollbars() was protected, then it would be the "fix" and this STR can be closed.
Fl_Scroll::draw() calls recalc_scrollbars() which I think is much much better than before. | |
| |
| #19 | greg.ercolano
08:02 Jul 15, 2014 |
| Alvin, thanks for the follow up.
Changing private -> protected would I think break ABI,
(we've had a release since the recalc method was added)
so we'll have to protect this change with FLTK_ABI_VERSION.
Assigning this to me, and changing the software version back down
from 1.4 -> 1.3-current, since we can do this as an ABI feature now. | |
| |
| #20 | greg.ercolano
20:39 Jul 15, 2014 |
| Fixed in Subversion repository.
Added as an ABI feature; to use the new method in the 1.3.x series,
you'll need to set in FL/Enumerations.H:
#define FLTK_ABI_VERSION 10303
..and rebuild FLTK. Then you can access recalc_scrollbars()
I'm attaching a simplified version of your str1895_test program
(see str1895_test_erco_v1.cxx) that demonstrates how to do what you wanted
using the new ScrollInfo structure and recalc_scrollbars() method.
Closing -- if it needs to be re-opened, put a message in fltk.coredev saying why, and I'll reopen. | |
[ Return to Bugs & Features ]
|
| |
