Skip to content

Add 'record with playback' to settings menu#625

Merged
killergerbah merged 7 commits intokillergerbah:mainfrom
NirDafnai:feature/configurable-audio-recording-playback
Jan 28, 2025
Merged

Add 'record with playback' to settings menu#625
killergerbah merged 7 commits intokillergerbah:mainfrom
NirDafnai:feature/configurable-audio-recording-playback

Conversation

@NirDafnai
Copy link
Copy Markdown
Contributor

Fixes issue #620

It's now configurable in settings menu:
image

Copy link
Copy Markdown
Owner

@killergerbah killergerbah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the AudioClip.fromCard call in handleCopy in App.tsx as well. That will ensure that this setting behaves consistently in all cases.

"preCacheSubtitleDom": "Pre-cache Subtitle DOM",
"preCacheSubtitleDomHelperText": "If enabled, asbplayer will pre-render subtitle text elements in an offscreen element, and re-use those elements for subtitle display. This allows external code to persistently modify subtitle text before it is displayed.",
"recordingBind": "Recording",
"recordWithAudioPlayback": "Record with audio playback",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This copy should specify that it only applies to local video.
Something like: "Play audio while recording audio from local video"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@NirDafnai
Copy link
Copy Markdown
Contributor Author

NirDafnai commented Jan 24, 2025

Please update the AudioClip.fromCard call in handleCopy in App.tsx as well. That will ensure that this setting behaves consistently in all cases.

Fixed


const initialAudioClip = useMemo(
() => AudioClip.fromCard(card, settings.audioPaddingStart, settings.audioPaddingEnd, false),
() => AudioClip.fromCard(card, settings.audioPaddingStart, settings.audioPaddingEnd, settings.recordWithAudioPlayback),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this on the first pass. You need to update the dependency array being passed to useMemo to ensure the audio clip changes whenever the setting changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, I added now the setting to the array below this line if that's what you meant. Correct me if i'm wrong

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change is the right one. If you're interested in learning why you can look up react hooks and dependency arrays.

}, []);

}, []);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra space will fail the Prettier formatting check on this repo. Easiest way to fix this is run Prettier on the file whenever you make a change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, fixed!

settingsRef.current.audioPaddingStart,
settingsRef.current.audioPaddingEnd,
true
settings.recordWithAudioPlayback
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, use settingsRef to access the setting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@NirDafnai
Copy link
Copy Markdown
Contributor Author

Forgot to add de localization, try running workflow now

@NirDafnai
Copy link
Copy Markdown
Contributor Author

Okay fixed formatting, now it should work.
sorry for the back and forth, this is my first open-source contribution :O

@killergerbah
Copy link
Copy Markdown
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants