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

fix link doc to envir #3764

Merged
merged 7 commits into from Jun 12, 2018
Merged

Conversation

telephon
Copy link
Member

@telephon telephon commented Jun 8, 2018

This fixes #3717.

The basic assumption is that only one document can be in focus.

  1. when a document comes to focus that has an envir bound to it, it should make that envir the currentEnvironment. Whatever envir that was current at that time should be saved.
  2. when a document loses focus (either by closing or because one moves to another one), it should restore the envir that was currentEnvironment before.
  3. when we set the envir of a document that has no focus, it should not do anything but store the envir for the moment it comes to focus.
  4. when we set the envir of a document that has focus, the envir should by default become currentEnvironment, so that it is in a consistent state.

Instead of relying on user code, test if the document is in front, and
push the environment correctly if this is the case. Save the current
environment only if we are not already in a document that has a saved
environment.

We add a getter for `savedEnvir` for introspection.

#testing: Test all methods that rely on setting the environment of a
document.
This implicit setter broke `didBecomeKey`. It was probably a leftover
from past revisions.

This fixes supercollider#3760.

#testing: Test calls to Document.current
This argument (`pushNow`) can be a very confusing option when set to
false. It creates a document which will be linked properly to the
environment only after losing focus and then coming back. For documents
that are not in focus, on the other hand, it is much better not to
push. This avoids user errors.

This is an API change.

#testing: Test calls of linkDoc and unlinkDoc to subclasses of
EnvironmentRedirect, e.g. ProxySpace.linkDoc. As this is an API change,
it might change the behaviour of user code (though it now behaves as
probably used in most cases, and was broken before).
@telephon telephon requested a review from LFSaw June 8, 2018 16:06
@LFSaw
Copy link
Member

LFSaw commented Jun 8, 2018

can you provide a test case please? ;)

@LFSaw
Copy link
Member

LFSaw commented Jun 8, 2018

sorry, I was too fast... there's a test class and I'll have a look at how to deal with it (following advice in UnitTest documentation and brian's unit testing overview
https://gist.github.com/brianlheim/91222d487afa18582c287b0a722ae272 ).

@telephon
Copy link
Member Author

telephon commented Jun 8, 2018

yes, it doesn't adhere to the "only one assert per method" rule. The resulting code would be much harder to read, because one test relies on the state of the previous.

@telephon
Copy link
Member Author

telephon commented Jun 8, 2018

It would be good if you could test it also "by hand", maybe just with ProxySpace(s).linkDoc, and see if you are happy with the behavior.

@telephon
Copy link
Member Author

telephon commented Jun 9, 2018

@LFSaw I've improved the tests a bit.

@telephon telephon added this to the 3.10 milestone Jun 9, 2018
@LFSaw
Copy link
Member

LFSaw commented Jun 9, 2018 via email

@LFSaw
Copy link
Member

LFSaw commented Jun 12, 2018

(( side note if you name your PRs/branches topic/fix/linkDoc-03 instead of topic-fix-linkDoc-03, it is recognised by many (e.g. my preferred) git gui tool as folders in a tree overview. this makes searching for topics much easier for externals. ))

Copy link
Member

@LFSaw LFSaw left a comment

Choose a reason for hiding this comment

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

  • TestDocumentEnvir.runAll resulted in no errors.
  • TestEnvironmentDocument.runAll resulted in no errors.

testing "by hand"

  1. create four documents, evaluate their content.
  2. switching between documents results in correct environment being printed

// 1

// ProxySpace behaviour
ProxySpace(s).linkDoc;
Document.current.toFrontAction = {currentEnvironment.postln}

// 2
// explicitely link current Einvironment
currentEnvironment.linkDoc;
Document.current.toFrontAction = {currentEnvironment.postln}

// 3

// arbitrary Envir
(test: true).linkDoc;

Document.current.toFrontAction = {currentEnvironment.postln}

// 4
// default case
Document.current.toFrontAction = {currentEnvironment.postln}

}


TestEnvironmentDocument : UnitTest {
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct Naming scheme?

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 supposed that this is reasonable. So far, we haven't been very systematic about it.

@telephon
Copy link
Member Author

re side note: github desktop doesn't permit slashes for whatever reason

@telephon telephon merged commit 8c0a133 into supercollider:develop Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProxySpace.linkDoc is broken
2 participants