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

SynthDesc and SynthDescLib should restore metadata to their SynthDefs too #5122

Merged
merged 7 commits into from
Aug 18, 2020

Conversation

jamshark70
Copy link
Contributor

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

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • This PR is ready for review

@jamshark70 jamshark70 added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library labels Aug 5, 2020
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.

thanks @jamshark70 !

just some comments on the tests

testsuite/classlibrary/TestSynthDesc.sc Outdated Show resolved Hide resolved
testsuite/classlibrary/TestSynthDesc.sc Outdated Show resolved Hide resolved
testsuite/classlibrary/TestSynthDesc.sc Outdated Show resolved Hide resolved
testsuite/classlibrary/TestSynthDesc.sc Outdated Show resolved Hide resolved
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.

looks mostly good, thanks!

@@ -0,0 +1,57 @@
TestSynthDescMetadata : UnitTest {
var dir, path, mdPath, metadata, original, oldMdPlugin;
var defname = \test_md;
Copy link
Contributor

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...

Copy link
Contributor Author

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];
Copy link
Contributor

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)

@jamshark70 jamshark70 force-pushed the topic/SynthDescMetadata branch from 125a5cf to 11d5681 Compare August 14, 2020 00:50
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.

thanks!

@mossheim mossheim merged commit dc839e1 into supercollider:develop Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants