Skip to content
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

Make recorded file URLs public in NodeRecorder.swift #2883

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

NickCulbertson
Copy link
Member

This allows us to access the recorded files from our main class.

@misteu
Copy link
Contributor

misteu commented Feb 23, 2024

TL;DR

Consider making it private(set) static var recordedFiles = [URL]() instead.

Or would you need a public setter as well?

I think it general it makes sense to make it accessible, in the current state I am also not sure for what the recordedFiles are used for anyway. The newly recorded URLs are appended but nothing is done with them (apart from removing them via that cleanup method)

I'm actually only using record() which creates an internalAudioFile if needed. And access it's URL via audioFile and I never bothered about this array but it still might make sense to have it accessible.

Copy link
Member

@Matt54 Matt54 left a comment

Choose a reason for hiding this comment

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

The only reason I can think of to keep it private is to protect the url that's currently being recorded to since we append that url from the moment we create a new file in createAudioFile

I could see private(set) being more appropriate, but perhaps that is opinionated. I could see a situation where someone would want to remove a specific recording (some type of cancel operation).

I'd personally be fine with us just making it public and letting developers handle it how they would like.

@NickCulbertson I'm currently bookkeeping these files by grabbing a reference to nodeRecorder.audioFile?.url right before calling .stop() and .createNewFile(). It's a little clunky, but it works for me.

@Matt54
Copy link
Member

Matt54 commented Feb 23, 2024

Just seeing this PR from December, sorry it slipped through the cracks @NickCulbertson! Got a notification for it this morning thanks to @misteu's comment.

@aure aure merged commit e58f4aa into AudioKit:main Feb 23, 2024
2 checks passed
blezin2205 pushed a commit to Volodymyr-13/AudioKit that referenced this pull request Apr 1, 2024
* main:
  Fix unit tests to run correctly on Apple Silicon (AudioKit#2886)
  MIDIPacket Pre-allocate Bytes (AudioKit#2891)
  Fix crash after reading a MIDICustomMetaEvent with payload size > 127 bytes (AudioKit#2892)
  Make recorded file URLs public in NodeRecorder.swift (AudioKit#2883)
mat2uken pushed a commit to mat2uken/AudioKit that referenced this pull request Apr 22, 2024
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.

4 participants