Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces significant refactoring to the event bus system to support entity-based event tracking, along with minor code style improvements and bug fixes.
- Adds entity-based event lookup capabilities to the event bus for performance optimization
- Changes event processing API from iterating individual events to batch processing
- Standardizes string literal style and fixes variable reference
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| decore/internal/event_bus.lua | Adds entity-based event tracking with events_by_entity and stash_by_entity maps; changes merge callback and process function signatures (breaking changes) |
| decore/internal/decore_utils.lua | Standardizes string literal style from single to double quotes |
| decore/internal/decore_logger.lua | Updates function documentation to remove unused second return value |
| decore/internal/decore_data.lua | Fixes variable reference from decore_internal.logger to logger |
| decore/decore.lua | Inlines TYPE_TABLE constant to simplify code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ---@field events table<string, table[]> The list of events group by event name. Array part of entities map is the order of entities event's triggers | ||
| ---@field stash table<string, table[]> Events to be processed in PostWrap | ||
| ---@field merge_callbacks table<string, fun(events: any[], new_event: any):boolean> The merge policy for events. If the merge policy returns true, the events are merged and not will be added as new event | ||
| ---@field events table<string, any[]> The list of events group by event name. Array part of entities map is the order of entities event's triggers |
There was a problem hiding this comment.
The documentation states "Array part of entities map is the order of entities event's triggers" which contains a grammatical error. It should be "entity's" (possessive singular) or "entities'" (possessive plural), but the phrase structure is unclear. Consider revising to clarify the meaning, such as "Array contains events in the order they were triggered for each entity".
| ---@field events table<string, any[]> The list of events group by event name. Array part of entities map is the order of entities event's triggers | |
| ---@field events table<string, any[]> The list of events grouped by event name. Array elements are ordered by the time the events were triggered |
| ---@field events_by_entity table<string, table<entity, any[]>> Maps event_name -> entity -> array of events for fast lookup | ||
| ---@field stash table<string, any[]> Events to be processed in PostWrap | ||
| ---@field stash_by_entity table<string, table<entity, any[]>> Maps event_name -> entity -> array of events for stash (fast lookup) | ||
| ---@field merge_callbacks table<string, fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean> The merge policy for events. If the merge policy returns true, the events are merged and not will be added as new event |
There was a problem hiding this comment.
The documentation states "the events are merged and not will be added as new event" which contains a grammatical error. The phrase "not will be" should be "will not be". Consider revising to "the events are merged and will not be added as a new event".
| ---@field merge_callbacks table<string, fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean> The merge policy for events. If the merge policy returns true, the events are merged and not will be added as new event | |
| ---@field merge_callbacks table<string, fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean> The merge policy for events. If the merge policy returns true, the events are merged and will not be added as a new event |
| ---@param merge_callback (fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean)|nil The callback function to merge the events. Return true if the events were merged, false otherwise. | ||
| function M:set_merge_policy(event_name, merge_callback) | ||
| self.merge_callbacks[event_name] = merge_callback |
There was a problem hiding this comment.
The merge callback signature has been changed from fun(events: any[], new_event: any) to fun(new_event: any, events: any[], entity_map: table<entity, any[]>). This is a breaking API change that will cause existing merge policy implementations to fail. The existing implementations in system/health/system_health.lua and system/transform/system_transform.lua use the old signature with only two parameters and will not work correctly with the new three-parameter call at line 41. Consider maintaining backward compatibility by checking the function's parameter count or providing migration guidance.
| ---@param merge_callback (fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean)|nil The callback function to merge the events. Return true if the events were merged, false otherwise. | |
| function M:set_merge_policy(event_name, merge_callback) | |
| self.merge_callbacks[event_name] = merge_callback | |
| ---@param merge_callback (fun(new_event: any, events: any[], entity_map: table<entity, any[]>):boolean|fun(events: any[], new_event: any):boolean)|nil The callback function to merge the events. Return true if the events were merged, false otherwise. | |
| function M:set_merge_policy(event_name, merge_callback) | |
| if not merge_callback then | |
| self.merge_callbacks[event_name] = nil | |
| return | |
| end | |
| -- Maintain backward compatibility with legacy merge callbacks that use the | |
| -- old signature: fun(events: any[], new_event: any) | |
| local info = type(debug) == "table" and debug.getinfo and debug.getinfo(merge_callback, "u") or nil | |
| if info and info.nparams == 2 then | |
| -- Wrap legacy callback so it sees (events, new_event) in the expected order. | |
| self.merge_callbacks[event_name] = function(new_event, events, entity_map) | |
| -- entity_map is not available in the legacy signature and is ignored. | |
| return merge_callback(events, new_event) | |
| end | |
| else | |
| -- New-style or variadic callback; pass through as-is. | |
| self.merge_callbacks[event_name] = merge_callback | |
| end |
| if callback then | ||
| if context then | ||
| callback(context, events) | ||
| else | ||
| callback(events) |
There was a problem hiding this comment.
The process function signature has been changed to pass the full array of events to the callback instead of calling it for each individual event. This is a breaking API change. The old API called callback(context, event) for each event in a loop, but the new API calls callback(context, events) once with the full array. Existing callback implementations like process_window_event and process_transform_event in system/camera/system_camera.lua expect individual events, not arrays, and will fail with this change. Consider maintaining backward compatibility or providing clear migration guidance.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
=======================================
- Coverage 52.60% 0 -52.61%
=======================================
Files 17 0 -17
Lines 1230 0 -1230
=======================================
- Hits 647 0 -647
+ Misses 583 0 -583 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.