| | [ Return to Bugs & Features | Roadmap 1.1 | SVN ⇄ GIT ]
STR #834
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 4 - High, e.g. key functionality not working |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | flaw in Fl_Browser_ callback logic: two concurrent calls of the callback. |
|---|
| Version: | 1.1-current |
|---|
| Created By: | a.rburgers.quicknet |
|---|
| Assigned To: | mike |
|---|
| Fix Version: | 1.1-current (SVN: v4484) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | a.rburgers.quicknet
11:49 Apr 26, 2005 |
| Fl_Browser_::select currently looks as follows:
int Fl_Browser_::select(void* l, int i, int docallbacks) {
if (type() == FL_MULTI_BROWSER) {
...
...
if (docallbacks) {
set_changed();
do_callback();
}
return 1;
}
I think it should look something like this:
void Fl_Browser_::select(void* l, int i, int docallbacks, char *change) {
if (type() == FL_MULTI_BROWSER) {
...
...
*change = 1;
if (docallbacks) {
set_changed();
do_callback();
}
}
invocations of select functions such as this one:
change = select_only(l, when() & FL_WHEN_CHANGED)
should then be changed to
select_only(l, when() & FL_WHEN_CHANGED, &change)
The reason is that I think the setting of static char change to
1 in Fl_Browser_::handle(int event) comes too late.
I had a problem when the callback does an fl_alert(). When
there is an FL_PUSH followed by an FL_LEAVE, the FL_PUSH results
in a first call of the callback through Fl_Browser_::select.
The wait() in the fl_alert in the first call of the callback allows
the FL_LEAVE event to be handled. At that moment the first callback
has not returned yet, so static char change is still 0. This leads to a
second - concurrent - invocation in this piece of code in
Fl_Browser_::handle in the false branch of the if (change):
case FL_RELEASE:
if (type() == FL_SELECT_BROWSER) {
void* t = selection_; deselect(); selection_ = t;
}
if (change) {
set_changed();
if (when() & FL_WHEN_RELEASE) do_callback();
} else {
if (when() & FL_WHEN_NOT_CHANGED) do_callback();
}
return 1;
After my proposed modification, change will already be set
to 1 when the FL_LEAVE event is handled.
The second callback can also be prevented by de-activating the
Fl_Browser widget at the start of the callback, but I think
this should not be necessary.
| |
| |
| #2 | a.rburgers.quicknet
10:40 Apr 27, 2005 |
| Mike wrote:
> Clearly we can't make this change in 1.1.x, as it will break binary
> compatibility.
> However, IMHO Fl_Browser_ should only be doing a callback on FL_PUSH
> *or* FL_RELEASE, not both.
Perhaps modifications such as these should be made:
--- Fl_Browser_.cxx (revision 4300)
+++ Fl_Browser_.cxx (working copy)
@@ -610,7 +610,11 @@
if (type() == FL_NORMAL_BROWSER || !top_)
;
else if (type() != FL_MULTI_BROWSER) {
- change = select_only(find_item(my), when() & FL_WHEN_CHANGED);
+ change = select_only(find_item(my), 0);
+ if (change && (when() & FL_WHEN_CHANGED)) {
+ set_changed();
+ do_callback();
+ }
} else {
void* l = find_item(my);
whichway = 1;
| |
| |
| #3 | mike
06:34 May 12, 2005 |
| Fixed in Subversion repository.
Suggested patch accepted... | |
| |
| #4 | mike
06:25 May 19, 2005 |
| [reopened]
The suggested patch in the STR was not complete, it only served to illustrate the idea.
See the patch attached in this e-mail for a complete patch. Please
review carefully. I post it here since the STR #834 has already been closed.
Teun | |
| |
| #5 | mike
19:59 Aug 07, 2005 |
| Fixed in Subversion repository. | |
[ Return to Bugs & Features ]
|
| |
