Conversation
|
this prevents the crash from #475, which is good. however it introduces silent failures -- if there's no mic, the app won't crash but also won't tell the user anything. they'll press push-to-talk and nothing happens additionally, if the worker exits early during init, calling i think we should validate the audio config in |
| Ok(c) => c, | ||
| Err(e) => { | ||
| log::error!("Failed to get audio config: {}", e); | ||
| return; |
There was a problem hiding this comment.
this prevents the crash but now open() returns Ok(()) even when the worker immediately exits. the caller (audio manager) sets is_open = true and thinks everything is fine
maybe we do config validation in open() before spawning the worker, or use a oneshot channel to communicate init success / failure back
| } | ||
| }; | ||
|
|
||
| if let Err(e) = stream.play() { |
There was a problem hiding this comment.
if the worker exits here ^, stop() will deadlock, and it sends Cmd::Stop(resp_tx) to a dead worker and resp_rx.recv() blocks forever waiting for a response that never comes
There was a problem hiding this comment.
think we should not let open() succeed if init fails
|
Thanks for the review @VirenMohindra, I definitely want to consider a toasting ui of some kind for error handling generally. I think probably having some unified way to throw up to the UI would be helpful for big errors Tbh I just let Claude write whatever and hadn't even reviewed this, if you would like to submit changes and improvements I would be overjoyed!! I already have a huge backlog and would love to focus on keyboard input stuff instead |
happy to! def want to clear out my in flight PRs before i jump on this one though, before they get too stale 😅 |
|
@VirenMohindra i will try and get them in soon! I'm not planning to pull much else in for a week or two. I'm trying to wrap up a rewrite of the keyboard input library generally |
2 participants
title