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

class library: event - fix group in grain event #3483

Merged
merged 4 commits into from
Feb 3, 2018
Merged

class library: event - fix group in grain event #3483

merged 4 commits into from
Feb 3, 2018

Conversation

telephon
Copy link
Member

Fixes: #3481.

Quick test:

(type:\grain).play;

g = Group(s);

(type:\grain, group: g).play;

@telephon
class library: event - fix group in grain event
daff941 
We need to either call `asControlInput` or `value` to get the group,
because by default it is a function. `asControlInput` is more general,
it will also work for Group objects.
@mossheim
Copy link
Contributor

Thanks! Can you please add a unit test for this? (Not familiar with this code so apologies in advance if that's asking a lot)

@telephon
Copy link
Member Author

ok, I'll do that.

@nhthn nhthn added this to the 3.9.1 milestone Jan 29, 2018
@nhthn
Copy link
Contributor

nhthn commented Jan 29, 2018

this is ✔️ for 3.9.1

@nhthn nhthn added the comp: class library SC class library label Jan 29, 2018
@jamshark70
Copy link
Contributor

We should fix all affected event types in the main class library (from #3481: "This is a bug now in many of the event types").

We get bugs like this by making a global change and testing only one case. We keep bugs like this by fixing them on a case-by-case basis.

@jamshark70
Copy link
Contributor

Ok, I did a quick sweep of Event.sc and EventTypesWithCleanup.sc and it seems \grain was the only one missing the conversion. So I think this is good to go. Thanks, Julian!

@telephon
Copy link
Member Author

I'll write a test that covers a little more than this case.

@telephon
test: test head of event type OSC messages
b7e975e
@telephon
test: refactor event tests
468e396 
Event tests should point to the server’s defaults.
patrickdupuis
patrickdupuis approved these changes Feb 1, 2018
View reviewed changes
Copy link
Contributor

@patrickdupuis patrickdupuis left a comment

Choose a reason for hiding this comment

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

Builds on Linux. Test passed all runs.

nhthn
nhthn reviewed Feb 1, 2018
View reviewed changes
testsuite/classlibrary/TestEvent.sc Outdated
@@ -102,7 +102,98 @@ TestEvent : UnitTest {

}

test_server_messages {
test_server_message_head_type_grain {
this.assertEqualServerMessage(\grain, [ 9, \default, -1, 0, this.defaultGroupID])
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace: don't put spaces after [ or before ], use exactly one space after ,

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't put them there personally, but I can remove them ;)

nhthn
nhthn reviewed Feb 1, 2018
View reviewed changes
testsuite/classlibrary/TestEvent.sc Outdated




Copy link
Contributor

Choose a reason for hiding this comment

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

too many blank lines

@nhthn nhthn modified the milestones: 3.9.1, 3.9.2 Feb 3, 2018
@nhthn nhthn merged commit 2cee904 into supercollider:3.9 Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhthn nhthn nhthn approved these changes

@patrickdupuis patrickdupuis patrickdupuis approved these changes

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

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