-
Notifications
You must be signed in to change notification settings - Fork 45
Default to capturing mic + all system audio. #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e24ef27
Default to capturing mic + all system audio.
aaugustin fc84b7e
Honor an explicit mic = false when a mic device is configured
paberr 1c3825f
README: document mic capture on by default
paberr 188ed03
Point at --no-mic when no audio was captured
paberr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,13 +30,49 @@ def test_no_summarize_flag(self): | |
| config = mock_run.call_args[0][0] | ||
| assert config.summarization.enabled is False | ||
|
|
||
| def test_mic_flag(self): | ||
| def test_no_mic_flag(self): | ||
| runner = CliRunner() | ||
| with _mock_config(), mock.patch("ownscribe.pipeline.run_pipeline") as mock_run: | ||
| result = runner.invoke(cli, ["--no-mic"]) | ||
| assert result.exit_code == 0 | ||
| config = mock_run.call_args[0][0] | ||
| assert config.audio.mic is False | ||
|
Comment on lines
+33
to
+39
Comment on lines
+33
to
+39
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two small things on the tests here, non-blocking:
Once the |
||
|
|
||
| def test_mic_flag_overrides_config(self): | ||
| runner = CliRunner() | ||
| config = Config() | ||
| config.audio.mic = False | ||
| with _mock_config(config), mock.patch("ownscribe.pipeline.run_pipeline") as mock_run: | ||
| result = runner.invoke(cli, ["--mic"]) | ||
| assert result.exit_code == 0 | ||
| assert mock_run.call_args[0][0].audio.mic is True | ||
|
|
||
| def test_mic_device_implies_mic(self): | ||
| runner = CliRunner() | ||
| config = Config() | ||
| config.audio.mic = False | ||
| with _mock_config(config), mock.patch("ownscribe.pipeline.run_pipeline") as mock_run: | ||
| result = runner.invoke(cli, ["--mic-device", "USB Mic"]) | ||
| assert result.exit_code == 0 | ||
| config = mock_run.call_args[0][0] | ||
| assert config.audio.mic is True | ||
| assert config.audio.mic_device == "USB Mic" | ||
|
|
||
| def test_no_mic_wins_over_configured_mic_device(self): | ||
| runner = CliRunner() | ||
| config = Config() | ||
| config.audio.mic_device = "USB Mic" | ||
| with _mock_config(config), mock.patch("ownscribe.pipeline.run_pipeline") as mock_run: | ||
| result = runner.invoke(cli, ["--no-mic"]) | ||
| assert result.exit_code == 0 | ||
| assert mock_run.call_args[0][0].audio.mic is False | ||
|
|
||
| def test_no_mic_with_mic_device_errors(self): | ||
| runner = CliRunner() | ||
| with _mock_config(): | ||
| result = runner.invoke(cli, ["--no-mic", "--mic-device", "USB Mic"]) | ||
| assert result.exit_code != 0 | ||
| assert "--no-mic and --mic-device cannot be used together" in result.output | ||
|
|
||
| def test_device_flag(self): | ||
| runner = CliRunner() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| """Tests for the Core Audio helper command line.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
| from unittest import mock | ||
|
|
||
|
|
||
| def _make_recorder(**kwargs): | ||
| from ownscribe.audio.coreaudio import CoreAudioRecorder | ||
|
|
||
| binary = Path("/usr/local/bin/ownscribe-audio") | ||
| with mock.patch("ownscribe.audio.coreaudio._find_binary", return_value=binary): | ||
| return CoreAudioRecorder(**kwargs) | ||
|
|
||
|
|
||
| def _capture_cmd(recorder, tmp_path: Path) -> list[str]: | ||
| with mock.patch("ownscribe.audio.coreaudio.subprocess.Popen") as mock_popen: | ||
| recorder.start(tmp_path / "recording.wav") | ||
| return mock_popen.call_args[0][0] | ||
|
|
||
|
|
||
| class TestCoreAudioRecorderCommand: | ||
| def test_mic_and_device_passed_when_mic_enabled(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(mic=True, mic_device="USB Mic"), tmp_path) | ||
|
|
||
| assert "--mic" in cmd | ||
| assert cmd[cmd.index("--mic-device") + 1] == "USB Mic" | ||
|
|
||
| def test_mic_device_ignored_when_mic_disabled(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(mic=False, mic_device="USB Mic"), tmp_path) | ||
|
|
||
| assert "--mic" not in cmd | ||
| assert "--mic-device" not in cmd | ||
|
|
||
| def test_capture_mode_all(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(capture_mode="all"), tmp_path) | ||
|
|
||
| assert "--capture-mode-all" in cmd | ||
|
|
||
| def test_capture_mode_picker(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(capture_mode="picker"), tmp_path) | ||
|
|
||
| assert "--capture-mode-all" not in cmd | ||
|
|
||
| def test_silence_timeout_passed(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(silence_timeout=120), tmp_path) | ||
|
|
||
| assert cmd[cmd.index("--silence-timeout") + 1] == "120" | ||
|
|
||
| def test_silence_timeout_omitted_when_disabled(self, tmp_path): | ||
| cmd = _capture_cmd(_make_recorder(silence_timeout=0), tmp_path) | ||
|
|
||
| assert "--silence-timeout" not in cmd |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: I think
--no-micdoesn't actually disable the mic whenmic_deviceis set inconfig.toml:--no-micsetsconfig.audio.mic = False, butconfig.audio.mic_devicekeeps its value from the config file.CoreAudioRecorder.start()re-enables the mic from the device name alone (if self._mic or self._mic_device: cmd.append("--mic")).--mic --mic-device "USB Mic"and we record the mic anyway.Possible fix: clear
config.audio.mic_devicewhen--no-micis given, e.g.(same as CoPilot's comment below I just noticed)