| |
| |
We should really fix this because
value() is non-const which is generally not to be expected by
users. If it had been 'const' in the first place we wouldn't have
had this user report at all!
I've been looking for a fix but we can't easily say which change
would break existing user programs. Backwards compatibility might
be an issue here.
As a first step I used Greg's patch but replaced his call of
value() by a new method first_visible() [1] which is 'const' and
does exactly that: it returns the first visible child or NULL (the
name may be changed) if no child is visible. If we could instead
make value(void) 'const' that would be the preferred solution
though. This is the new (trivial) method:
// Note: Fl_Tabs::value() should be
like this:
Fl_Widget* Fl_Tabs::first_visible() const {
Fl_Widget*const* a = array();
for (int i = children(); i--;) {
Fl_Widget *o = *a++;
if (o->visible())
return o;
}
return 0;
}
Gregs patch becomes:
int Fl_Tabs::value(Fl_Widget *newvalue) {
Fl_Widget*const* a = array();
Fl_Widget *old = first_visible();
for (int i=children(); i--;) {
Fl_Widget* o = *a++;
if (o == newvalue) {
o->show();
} else {
o->hide();
}
}
return (old != newvalue);
}
[1] @Greg: calling the existing value(void) method inside
value(Fl_Widget *) could cause even more unexpected side effects
than we're having now. Other than that: great findings, thanks!
... continued below ...
On 9/3/21 5:59 PM Bill Spitzak wrote:
Also means value() can return on the first visible
child, which is slightly faster.
I agree, and your fix is probably correct.
I think that show() and hide() in value() was because
I was worried that the tabs could get into some kind of
incorrect state where the number that are visible is not
one, and this is trying to fix that. I think now that is
probably a mistake, such states should not happen, and
it would be better to just return the first visible one
or null if none are visible. If this is a problem it
would be better to fix it when there is an attempt to
set the value, rather than when it is read.
I agree that value() should not change states of children, I'd
really like (and would expect!) value() to be a 'const' method.
The problem with this is that it's documented behavior and the user
(program) can at any time add and delete children, show() or hide()
them etc. etc.. Currently value() is called on the FL_SHOW event of
the Fl_Tabs widget (which we could easily replace with another
appropriate method), but we need a way to control the state after
later user program actions (as mentioned above). IMHO the real
problem is event delivery if none or more than one children are
visible(), whereas drawing could always stop after the first child
was drawn. The latter would be an easy change - if and (maybe) only
if all children are drawn.
I was looking for a fix but was interrupted by other private stuff.
I'm still looking for a better solution...
On 9/2/21 10:03 PM, Greg Ercolano wrote:
On 9/2/21 6:38 PM, Bill Spitzak wrote:
It does seem like a bug, calling
value() should not have any side-effects.
Especially because the tabs widget works even if
nobody calls value().
Ah, it is weird that just testing with
Fl_Tabs::value(void) can apparently
end up calling hide() and show().. yikes.
That code goes back to 1999 though, so looks
like you'd know for sure:
49a0693962 (Matthias Melcher 2006-08-17
13:43:07 +0000 308) Fl_Widget* Fl_Tabs::value() {
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 309) Fl_Widget* v = 0;
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 310) Fl_Widget*const* a =
array();
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 311) for (int i=children(); i--;)
{
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 312) Fl_Widget* o = *a++;
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 313) if (v) o->hide();
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 314) else if (o->visible())
v = o;
a7904da09a (Bill Spitzak 1999-10-15
09:01:48 +0000 315) else
if (!i) {o->show(); v = o;}
f9039b2ae2 (Michael R Sweet 1998-10-06
18:21:25 +0000 316) }
f9039b2ae2 (Michael R Sweet 1998-10-06
18:21:25 +0000 317) return v;
f9039b2ae2 (Michael R Sweet 1998-10-06
18:21:25 +0000 318) }
Perhaps that's the cause, because that last line
in the loop that
calls o->show() on the last item seems to be
maybe what's forcing
the last child to be visible if no other one is..
..and by that I mean the loop code in
Fl_Tabs::value(Fl_Widget*) that changes
the selected tab by hide()ing all but the one to
be show()n
gets confused when it reaches the last tab and
finds it visible() before it show()s it,
causing the loop to think there was no change when
there in fact was.
It would appear Fl_Widget* value(void) seems to
enforce that:
> Not more than one group is visible
> That at least one group is visible (the
last, if no others are)
And that sounds like a good thing to enforce at
least /somewhere/,
as it's hard to imagine the Tabs widget with no
group visible, and/or
more than one group visible at a time.
So perhaps the fix I suggested is the right one,
assuming we want to
leave the value(void) code alone, and the show()
FL_HIDE/FL_SHOW event behavior
is correct.
--
You received this message because you are subscribed to the Google Groups "fltk.general" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkgeneral+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/fltkgeneral/92a424c7-6c8d-c60b-7e8d-cb55c53cd1fc%40online.de.
[ Direct Link to Message ] | |
|
| |
