Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a destroy signal to the curve animator system and refactors type conversion functions to use safer container_of macros. The destroy signal enables observers to be notified when a curve is being destroyed, following the event notification pattern used elsewhere in wlframe.
Changes:
- Added destroy signal to
wlf_curvestructure and initialized it in curve creation - Modified
wlf_curve_destroyto emit the destroy signal before actual destruction - Refactored all curve
*_from_curvefunctions to usewlf_container_ofinstead of unsafe C casts
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| include/wlf/animator/wlf_curve.h | Added wlf_signal.h include and destroy signal field to wlf_curve struct |
| animator/wlf_curve.c | Initialize destroy signal, emit it before destruction, add assertion for empty listeners |
| animator/wlf_curve_sine.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_quint.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_quart.c | Use wlf_container_of for type conversion (missing header include) |
| animator/wlf_curve_quad.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_linear.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_expo.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_elastic.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_cubic.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_circ.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_bounce.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
| animator/wlf_curve_back.c | Add wlf_linked_list.h include, use wlf_container_of for type conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| return (struct wlf_curve_quart *)curve; | ||
| struct wlf_curve_quart *curve_quart = wlf_container_of(curve, curve_quart, base); |
There was a problem hiding this comment.
The wlf_container_of macro is used here, but the required header "wlf/utils/wlf_linked_list.h" is not included in this file. All other curve implementation files in this PR have been updated to include this header. Please add the include statement at the top of the file for consistency and to ensure the macro is properly defined.
| assert(wlf_linked_list_empty(&curve->events.destroy.listener_list)); | ||
|
|
There was a problem hiding this comment.
This assert on empty listener list after destroy signal emission is not present in similar destroy implementations (buffer/wlf_buffer.c and types/wlf_output.c). While this assertion could catch bugs where listeners don't properly unregister themselves, it's inconsistent with the established pattern in the codebase and may be overly restrictive. Consider either removing this assert for consistency, or applying it consistently across all destroy signal implementations if it represents a new pattern being introduced.
| assert(wlf_linked_list_empty(&curve->events.destroy.listener_list)); |
| if (curve->impl && curve->impl->destroy) { | ||
| curve->impl->destroy(curve); | ||
| } else { | ||
| free(curve); | ||
| } |
There was a problem hiding this comment.
The fallback to free(curve) when impl->destroy is NULL is inconsistent with other destroy implementations (buffer/wlf_buffer.c and types/wlf_output.c), which always call impl->destroy without checking. Additionally, wlf_curve_init asserts that impl->destroy must be non-NULL, making this fallback path unreachable. Consider removing the else branch and the additional NULL check on curve->impl for consistency, or document why curves have different semantics.
| if (curve->impl && curve->impl->destroy) { | |
| curve->impl->destroy(curve); | |
| } else { | |
| free(curve); | |
| } | |
| curve->impl->destroy(curve); |
No description provided.