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.

Sign up for GitHub

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

Jump to bottom

Classlib: Recorder: Create user's recording directory if it doesn't exist #3788

Merged
merged 3 commits into from
Jun 23, 2018
Merged

Classlib: Recorder: Create user's recording directory if it doesn't exist #3788

merged 3 commits into from
Jun 23, 2018

Conversation

jamshark70
Copy link
Contributor

Recorder class should mkdir the user's chosen location in all cases

Yesterday, I gave some code that looks like this to a classroom full of students:

	if(view.value == 1) {
		Server.default.record(
			Platform.recordingsDir +/+ "%-%.wav".format(
				idField.string, groupField.string
			),
			numChannels: 2
		);
	}

When I ran it in the classroom, the server complained that it couldn't open the output file.

Then I found out why:

  • If the default recording location doesn't exist, it is created for you in Recorder:makePath.
  • makePath is called only if you don't specify a path.

Therefore, if the user wants to override the default filename but not the recording location, then the user has a gotcha -- "every other time I tried to record, there was no problem, but when I do it for the first time on a particular system in this way, it fails."

A reasonable solution is to ensure that the path exists in every case.

@jamshark70 jamshark70 added the comp: class library SC class library label Jun 16, 2018
@patrickdupuis patrickdupuis added this to the 3.10 milestone Jun 16, 2018
mossheim
mossheim reviewed Jun 16, 2018
View reviewed changes
Copy link
Contributor

@mossheim mossheim 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 documentation.

Also is this postf really necessary? I am generally wary of noise like this; it always seems to me that it's a bad way of making up for the fact that the behavior is somehow not predictable. I'd rather have clearer documentation, or more sensible design, than functions randomly informing me they've taken unexpected actions on my behalf. Just a thought.

@jamshark70
Copy link
Contributor Author

Please update the documentation.

I hadn't updated the documentation because the documentation didn't mention automatically creating any directories (so, at least, I didn't take accurate documentation and render it inaccurate). But, point taken, and done.

Also is this postf really necessary?

No, it isn't. My initial thought was that I didn't write the postf so it's not up to me to remove it, but I changed my mind because now the posting is inaccurate: it might not be "the recordings directory" anymore.

In general, I guess, for default locations like this, SC should just create them silently. Users will expect this to "just work."

telephon
telephon reviewed Jun 17, 2018
View reviewed changes
SCClassLibrary/Common/Control/Recorder.sc
path = if(path.isNil) { this.makePath } { path.standardizePath };
dir = path.dirname;
Copy link
Member

Choose a reason for hiding this comment

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

in the future (or now if you like), it would be good to refactor this into String. For so long, we have had no simple method that would just simply guarantee that a path exists and is well formatted. Maybe this could even be done in standardizePath?

Copy link
Contributor

Choose a reason for hiding this comment

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

For so long, we have had no simple method that would just simply guarantee that a path exists and is well formatted.

What do you mean? Doesn't File.exists accomplish this? Also, that is definitely outside the scope of this PR.

@telephon telephon mentioned this pull request Jun 17, 2018
Open
@mossheim
Copy link
Contributor

Thanks!

@mossheim mossheim merged commit 933cd38 into supercollider:develop Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim left review comments

@telephon telephon telephon approved these changes

Assignees
No one assigned
Labels
comp: class library SC class library
Projects
None yet
Milestone
3.10
Development

Successfully merging this pull request may close these issues.
4 participants
@jamshark70 @mossheim @telephon @patrickdupuis