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

Proxy mixer edit gui should use proxyspace #4339

Conversation

adcxyz
Copy link
Contributor

@adcxyz adcxyz commented Feb 28, 2019

Purpose and Motivation

Bug fix, Fixes #4335.

A ProxyMixer has a proxyspace that it shows, so all named it displays are within that proxyspace.
The editGui in the mixer did not use the proxyspace, but relied on either using Ndefs, or the
mixer's proxyspace being the currentEnvironment.
Thus, when the mixer's proxyspace is not current, proxy names would be displayed as anonymous.

This PR ensures that the editGui shows all names as found within the mixer's proxyspace.

  • Code is tested (see below)
  • This PR is ready for discussion

Tests for correct functioning:

p = ProxySpace.new.push(s.boot);
// make some proxies
~out = { \in.ar(0!2) };
~osc = { SinOsc.ar(440).dup };
// map a proxy to an ar input
~osc <>> ~out;

// make p != currentEnvironment
p.pop;

// make a mixer: should show names of proxies within p
g = p.gui;

// send ~out to editGui: editGui name should show "out", 
// and editGui's first paramView should show "~osc" as mapped input
g.arGuis[1].edBut.valueAction_(1);

// add an ar proxy: name should be shown correctly in arGuis on the left
p[\test].ar; 

// add a control proxy: show show name in krGuis in the middle
p[\lfo] = { SinOsc.kr(3) }

// send p[\test] to editGui: name "test" should get shown correctly in editGui on the right
g.arGuis[2].edBut.valueAction_(1);

// p[\out] to editGui: paramGui should show param "in" as mapped "~osc"
g.arGuis[1].edBut.valueAction_(1);

// add second source osc2: name "osc2" should get shown in arGuis
p[\osc2] = { Pulse.ar(440).dup };

// map ~out param in to osc2: name "osc2" should get shown in editGui.paramViews
p[\osc2] <>> p[\out];

Copy link
Contributor

@jamshark70 jamshark70 left a comment

Choose a reason for hiding this comment

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

Just a couple of places to simplify -- otherwise, if it's basically an indentation change to wrap inside the use function, then it's fine 👍


.action_({ arg btn, mod;
this.proxyspace.use {
protect {
Copy link
Contributor

Choose a reason for hiding this comment

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

protect inside use is not needed, because use already does protect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thank, did not look up the implementation of .use

this.name_(newState[\name])
};
this.proxyspace.use {
protect {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same (don't need protect here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, both done!

@jamshark70
Copy link
Contributor

LGTM now.

use has to include protect -- if an error occurs while you're using a different environment, you get all kinds of confusion if it doesn't restore the old environment.

Also, for future reference, protect { ... func ... } by itself doesn't make any sense. It needs to be protect { ... func ... } { ... unwind ... } where unwind restores an earlier state (such as currentEnvironment -- closing a file is another common case).

Approving.

@telephon
Copy link
Member

telephon commented Mar 2, 2019

retriggering AppVeyor build

@telephon telephon closed this Mar 2, 2019
@telephon telephon reopened this Mar 2, 2019
@telephon telephon merged commit bd73bc8 into supercollider:develop Mar 2, 2019
@nhthn nhthn added this to the 3.10.3 milestone Jun 2, 2019
nhthn pushed a commit that referenced this pull request Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jitlib] ProxyMixer widget actions are not bound to the environment
4 participants