-
Notifications
You must be signed in to change notification settings - Fork 23
Unify widget callback mechanism with closure #144
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 14 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="src/widget.c">
<violation number="1" location="src/widget.c:91">
Forward the closure you received when calling the copy widget’s handler; otherwise its user data is lost and the new closure-based API breaks for geometry-copy scenarios.</violation>
<violation number="2" location="src/widget.c:354">
Pass the closure argument to the user’s custom widget handler; dropping it to NULL means their registered user data never reaches the handler in the destroy path.</violation>
<violation number="3" location="src/widget.c:378">
Return the user dispatch with the closure you were passed; otherwise custom widget handlers cannot access their user data during normal event handling.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
e239453 to
925b966
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 14 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="src/box.c">
<violation number="1" location="src/box.c:44">
Passing NULL as the closure when dispatching to a child widget discards its callback_data, so widgets relying on per-instance state stop receiving their context. Please forward the child’s stored closure instead.</violation>
<violation number="2" location="src/box.c:191">
Pointer events forwarded to the pressed child must pass its callback_data; returning with a NULL closure prevents handlers from seeing their per-instance context.</violation>
<violation number="3" location="src/box.c:198">
Keyboard events sent to the focused child need to include its callback_data; passing NULL strips away the closure context and breaks stateful handlers.</violation>
<violation number="4" location="src/box.c:217">
Paint callbacks must receive the widget’s callback_data closure; forwarding NULL causes stateful renderers to lose their context.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 issues found across 14 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name="src/button.c">
<violation number="1" location="src/button.c:69">
The new click dispatch emits TwinEventButtonSignalDown from the TwinEventButtonUp handler, so button release events are misreported and the matching TwinEventButtonSignalUp is never sent. This breaks release handlers and mislabels clicks.</violation>
</file>
<file name="src/widget.c">
<violation number="1" location="src/widget.c:91">
Forward the incoming closure when delegating QueryGeometry to the copy widget; replacing it with callback_data loses the caller-provided context and breaks custom handlers.</violation>
</file>
<file name="src/box.c">
<violation number="1" location="src/box.c:44">
Forward the widget’s closure when dispatching to children; passing NULL drops application state for QueryGeometry handlers.</violation>
<violation number="2" location="src/box.c:191">
Propagate the child widget’s closure when sending pointer events; NULL strips the handler’s state.</violation>
<violation number="3" location="src/box.c:198">
Return the focused child with its closure so key handlers can access state; passing NULL breaks them.</violation>
<violation number="4" location="src/box.c:217">
Forward the child’s closure when invoking its paint handler; NULL removes the required state.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| twin_widget_t *copy = widget->copy_geom; | ||
| if (copy->layout) | ||
| (*copy->dispatch)(copy, event); | ||
| copy->handler(copy, event, copy->callback_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forward the incoming closure when delegating QueryGeometry to the copy widget; replacing it with callback_data loses the caller-provided context and breaks custom handlers.
Prompt for AI agents
Address the following comment on src/widget.c at line 91:
<comment>Forward the incoming closure when delegating QueryGeometry to the copy widget; replacing it with callback_data loses the caller-provided context and breaks custom handlers.</comment>
<file context>
@@ -75,16 +75,20 @@ static void _twin_widget_paint(twin_widget_t *widget)
twin_widget_t *copy = widget->copy_geom;
if (copy->layout)
- (*copy->dispatch)(copy, event);
+ copy->handler(copy, event, copy->callback_data);
widget->preferred = copy->preferred;
return TwinDispatchDone;
</file context>
| copy->handler(copy, event, copy->callback_data); | |
| copy->handler(copy, event, closure); |
925b966 to
92b0b67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 14 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="src/box.c">
<violation number="1" location="src/box.c:132">
Passing NULL here discards the widget's callback_data, so configure handlers that rely on their closure will now be called with NULL and malfunction. Forward the stored closure instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Previous design had widget-specific callbacks (button signals, scroll signals) scattered across different widgets, each managing its own closure storage. Now all widgets use a unified callback mechanism: - Single callback signature: (widget, event, closure) → result - Closure stored in twin_widget_t.callback_data - Button clicks emit TwinEventButtonSignalDown event - Applications register callbacks via twin_widget_set_callback() Removed obsolete scroll signal typedefs and widget-specific callback fields. Custom widgets and button handlers now use the same dispatch pattern. Button event types now correctly reflect their semantics: - TwinEventButtonSignalDown: emitted on button press - TwinEventButtonSignalUp: emitted on button release (click completion) Applications updated to listen for ButtonSignalUp instead of Down, matching the GTK convention where click callbacks trigger on release.
92b0b67 to
3718a8a
Compare
Previous design had widget-specific callback mechanisms (button signals, scroll signals) with duplicated closure management code.
This commit introduces two complementary callback patterns:
twin_widget_proc_twith (widget, event, closure) signaturetwin_widget_ttwin_button_on_clicked(button, callback, data)void (*)(twin_button_t*, void*)Button signals promoted to standard events (TwinEventButtonSignal*). Removed obsolete scroll signal typedefs and button-specific fields.
Summary by cubic
Unifies widget event handling with a closure-based API and adds a simple button click callback to replace widget-specific signal plumbing. This standardizes callbacks across widgets and simplifies demo apps.
New Features
Migration