fix: make announce filter dialog scrollable on short displays#483
fix: make announce filter dialog scrollable on short displays#483torlando-tech merged 3 commits intomainfrom
Conversation
CompanionDeviceService (API 31) superclass causes class verification crash on Samsung S8 (API 26-28) even though runtime code is guarded. Use resource-qualified android:enabled to gate the service at the manifest level so it's never loaded on older devices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oggle fix: Oboe SIGSEGV on OpenSL ES + speaker toggle for native stream
The NodeTypeFilterDialog had 9 checkbox rows plus headers that could overflow on shorter phone screens. Wraps the dialog Column in verticalScroll so all filter options are accessible. Closes #480 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR combines three separate changes: 1. UI Fix (Issue #480): Makes the announce filter dialog scrollable on short-display devices by adding 2. Companion Service Fix: Disables 3. Audio Call Optimization: Implements TX batching in Issues Found:
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[Kotlin: Audio frame ready] --> B[receive_audio_packet called]
B --> C{Is this first frame<br/>in batch?}
C -->|Yes| D[Set _tx_batch_start = now]
C -->|No| E[Add frame to _tx_batch]
D --> E
E --> F{Batch size >= 3<br/>OR timeout >= 150ms?}
F -->|No| G[Wait for more frames]
F -->|Yes| H[_flush_tx_batch]
H --> I[Pack frames as msgpack]
I --> J[RNS.Packet.send]
J --> K[Reset _tx_batch = empty]
K --> G
L[hangup called] --> M[Flush remaining batch]
M --> N[Reset batch state]
N --> O[Teardown link]
Last reviewed commit: 238c7f9 |
| # Reset batch state | ||
| self._tx_batch = [] | ||
| self._tx_batch_count = 0 | ||
| self._tx_send_time_sum = 0.0 |
There was a problem hiding this comment.
_tx_batch_start should also be reset here (currently only _tx_batch, _tx_batch_count, and _tx_send_time_sum are reset)
| # Reset batch state | |
| self._tx_batch = [] | |
| self._tx_batch_count = 0 | |
| self._tx_send_time_sum = 0.0 | |
| # Reset batch state | |
| self._tx_batch = [] | |
| self._tx_batch_start = 0.0 | |
| self._tx_batch_count = 0 | |
| self._tx_send_time_sum = 0.0 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: python/lxst_modules/call_manager.py
Line: 442:445
Comment:
`_tx_batch_start` should also be reset here (currently only `_tx_batch`, `_tx_batch_count`, and `_tx_send_time_sum` are reset)
```suggestion
# Reset batch state
self._tx_batch = []
self._tx_batch_start = 0.0
self._tx_batch_count = 0
self._tx_send_time_sum = 0.0
```
How can I resolve this? If you propose a fix, please make it concise.| if link is not None and self._tx_batch: | ||
| try: | ||
| self._flush_tx_batch(link) |
There was a problem hiding this comment.
Bug: The hangup() and receive_audio_packet() methods access the shared _tx_batch list concurrently without a lock, creating a race condition that can cause audio frames to be lost.
Severity: MEDIUM
Suggested Fix
Wrap the code block in hangup() that accesses and flushes _tx_batch (lines 436-440) within a with self._call_handler_lock: block to ensure thread-safe access to the shared batch state.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: python/lxst_modules/call_manager.py#L436-L438
Potential issue: A race condition exists because the `hangup()` and
`receive_audio_packet()` methods can be executed on different threads and both access
the shared `_tx_batch` list without acquiring the `_call_handler_lock`. If an audio
packet arrives while a call is being hung up, this concurrent access to the shared
mutable state can lead to lost audio frames or potential list corruption errors. The
code acknowledges the risk of a race condition with `hangup()` but only protects the
`active_call` reference, not the shared `_tx_batch` state.
Did we get this right? 👍 / 👎 to inform future reviews.
| should_notify = True | ||
|
|
||
| # Notify Kotlin OUTSIDE lock (same pattern as hangup()) | ||
| if should_notify: | ||
| self._send_signal_to_kotlin(STATUS_AVAILABLE) | ||
|
|
||
| if self._kotlin_call_bridge is not None: | ||
| try: | ||
| self._kotlin_call_bridge.onCallEnded(identity) | ||
| except Exception as e: | ||
| RNS.log(f"Error notifying Kotlin of link close: {e}", RNS.LOG_ERROR) |
There was a problem hiding this comment.
Bug: The __link_closed() method does not clear the _tx_batch, causing stale audio frames from a previous call to be sent to the recipient of a subsequent call.
Severity: CRITICAL
Suggested Fix
In the __link_closed() method, reset the transmit batch state by clearing _tx_batch and resetting _tx_batch_start and _tx_send_time_sum, similar to the logic already present in the hangup() method.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: python/lxst_modules/call_manager.py#L559-L576
Potential issue: When a call ends because the remote party hangs up or due to a network
failure, the `__link_closed()` method is invoked. This method fails to clear the audio
transmit buffer (`_tx_batch`) and its associated timestamp (`_tx_batch_start`). If a new
call is initiated shortly after, the first audio packet of the new call will be appended
to the stale frames from the previous call. The old timestamp will likely cause an
immediate flush, sending audio from the previous call to the recipient of the new call,
resulting in a data leak between calls.
Did we get this right? 👍 / 👎 to inform future reviews.
Summary
verticalScrollto theNodeTypeFilterDialogcolumn so all 9 checkbox rows (3 node types + audio + 5 interface types) are accessible on shorter phone screensCloses #480
Test plan
🤖 Generated with Claude Code