-
Notifications
You must be signed in to change notification settings - Fork 757
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
SynthDesc and SynthDescLib should restore metadata to their SynthDefs too #5122
SynthDesc and SynthDescLib should restore metadata to their SynthDefs too #5122
Conversation
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.
thanks @jamshark70 !
just some comments on the tests
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.
looks mostly good, thanks!
@@ -0,0 +1,57 @@ | |||
TestSynthDescMetadata : UnitTest { | |||
var dir, path, mdPath, metadata, original, oldMdPlugin; | |||
var defname = \test_md; |
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.
i think you can make this a const
? or are symbol literals not compatible with const...
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.
Both done.
test_SynthDescLib_read_restores_metadata_in_desc_and_def { | ||
var synthdesc; | ||
synthdesc = SynthDescLib.read(dir +/+ "*.scsyndef").global; | ||
synthdesc = synthdesc[\test_md]; |
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.
should be defname, same above (line 29)
125a5cf
to
11d5681
Compare
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.
thanks!
Purpose and Motivation
Fixes an issue noted on sc-users.
If a SynthDef has been written to disk with metadata, and later read from disk, in the current release you get:
SynthDescLib.global.at(defname).metadata
contains the metadata.SynthDescLib.global.at(defname).def.metadata
-- the reconstructed SynthDef -- does not contain the metadata.Unit testing was (as is often the case) trickier than expected to write -- but the process of writing it identified a second code path reading the metadata, so the fix is more thorough than was discussed on the mailing list.
Types of changes
To-do list