Implements a debounced notification listener in MeshCoreConnector#239
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a debounced notification listener mechanism in MeshCoreConnector to batch UI rebuilds and improve performance. The implementation adds a 50ms debounce to notifyListeners() calls, which helps group rapid state changes into single UI updates.
Changes:
- Added debouncing infrastructure with a timer and dirty flag to batch
notifyListeners()calls - Override
notifyListeners()to use the debouncing mechanism instead of immediate notifications - Added proper cleanup of the debounce timer in
dispose()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void markNotifyDirty() { | ||
| if (_notifyListenersDirty && _notifyListenersTimer != null) { | ||
| return; | ||
| } | ||
|
|
||
| _notifyListenersDirty = true; | ||
| _notifyListenersTimer ??= Timer( | ||
| _notifyListenersDebounce, | ||
| _flushBatchedNotify, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The markNotifyDirty() method and the debouncing mechanism lack documentation explaining their purpose and behavior. Consider adding documentation to explain that this implements a 50ms debounce for notifyListeners() calls to group UI rebuilds, how the dirty flag and timer interact, and that notifications occurring during a flush will be batched into the next flush cycle. This will help future maintainers understand the intended behavior and avoid accidentally breaking the debouncing logic.
| void markNotifyDirty() { | ||
| if (_notifyListenersDirty && _notifyListenersTimer != null) { | ||
| return; | ||
| } | ||
|
|
||
| _notifyListenersDirty = true; | ||
| _notifyListenersTimer ??= Timer( | ||
| _notifyListenersDebounce, | ||
| _flushBatchedNotify, | ||
| ); | ||
| } | ||
|
|
||
| void _flushBatchedNotify() { | ||
| _notifyListenersTimer = null; | ||
| if (!_notifyListenersDirty) { | ||
| return; | ||
| } | ||
|
|
||
| _notifyListenersDirty = false; | ||
| super.notifyListeners(); | ||
|
|
||
| if (_notifyListenersDirty && _notifyListenersTimer == null) { | ||
| _notifyListenersTimer = Timer( | ||
| _notifyListenersDebounce, | ||
| _flushBatchedNotify, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The codebase already has a pattern for safely scheduling notifications using Flutter's SchedulerBinding, as seen in BleDebugLogService._notifyListenersSafely() at lib/services/ble_debug_log_service.dart:92-108. That approach uses SchedulerBinding.instance.addPostFrameCallback which is more framework-aware and ensures notifications happen at safe points in the Flutter frame lifecycle. Consider whether this Timer-based debouncing approach is appropriate for MeshCoreConnector, or whether a SchedulerBinding-based approach would be more consistent with existing patterns and potentially avoid the race conditions inherent in the current implementation.
|
Wasn't this already a thing? |
|
Not unless there is a system in place already that I didn't know about. |
Implement debounced notification listener updates in MeshCoreConnector.
This help with grouping UI rebuilds.