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

Add bounds arg to {}.scope & s.plotTree #6282

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

prko
Copy link
Contributor

@prko prko commented May 1, 2024

Purpose and Motivation

This PR is a part of #6230

Types of changes

  • Documentation
  • New feature

To-do list

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

prko added 24 commits March 6, 2024 13:51
documentation update for the method .scope to define the location of scope window
documentation update for the method .scope to define the scope window location
update .scope method to locate scope window position
added variables to have a singleton plotTreeWindow and a singleton scopeWindowDefined
changed position argument in scope method to bounds
The position arguments for the server scope method and the function scope method are changed to bounds. And the server scope is changed so that it displays only one scope and works fine when users use manually assigned bounds and the default bound value of nil in succession.
has been modified to show only one boundary and to work fine when users are using manually assigned bounds and the default boundary value of nil, one after the other. Also, pressing m will change its size based on user-defined bounds, if any.
position argument for scope method is changed to bounds, so the documentation is updated
added bounds argument to the method `.plotTree`
documentation update for the added argument `bounds` for the method `.plotTree`
added point to define the server meter position.
documentation update for the added position argument for .meter method.
spaces within curly brackets are added
The behaviour of resizing has been corrected. It now retains the position as before if the new width and height do not exceed the screen boundary. If the new height exceeds the screen boundary, the vertical point is modified so that it does not exceed the screen boundary. If the new width exceeds the screen boundary, then the horizontal point is modified so that it does not exceed the screen boundary.
added the behaviour of change size if the new size exceeds the screen boundary.
The implementation of .scope has been simplified.
The implementation of .scope has been simplified.
added bounds for scope argument for Bus
bounds = nil in arg 
-> 
bounds
position = nil in arg
-> 
position
@prko
Copy link
Contributor Author

prko commented May 1, 2024

@mtmccrea

Hi, I think I split #6230 as you advised.
However, {}.scope(bounds: ... ) in this PR does not work because of the following line:

server.scope(numChannels, outbus, bufsize, zoom, outUGen.rate, bounds);

This line should be changed as follows:

server.scope(numChannels, outbus, bufsize, zoom, outUGen.rate). bounds_(bounds); 

However, #6281 should be approved and merged first.

Um... I have two questions:

  • Should I wait for add .bounds_ method to Stethoscope #6281 to be merged, or should I change it now?
  • Should I split this PR again as follows?
    • Add bounds arg to {}.scope
    • Add bounds arg to s.plotTree;
    • Make s.plotTree a singleton window

@mtmccrea
Copy link
Member

mtmccrea commented May 2, 2024

However, #6181 should be approved and merged first.
... should I change it now?

That's fine, you can change it now and just make a note of that in the PR description above, and we can put the has merge note tag on this.

Please add a description to this PR. The reason for breaking up the previous PR was to simplify it by having smaller standalone PRs, so this will need a standalone description of the changes. It's of course OK to reference it for context, but shouldn't rely on it.

Should I split this PR again as follows?

It's ultimately up to you to decide which changes are similar enough to "belong" together. Just know that someone may object to one but not all changes, and that objection will hold up the review of all of them. For example, is Make s.plotTree a singleton window related to or required for setting its bounds? (I'm not sure because there is no description).

@mtmccrea mtmccrea added the has merge note The Conversation has a note concerning how/when this PR is merged label May 2, 2024
@prko
Copy link
Contributor Author

prko commented May 3, 2024

Thanks for adding has merge note.

is Make s.plotTree a singleton window related to or required for setting its bounds?

Making s.plotTree a singleton window is a separate issue from adding the bounds argument.

Currently SuperCollider makes s.plotTree every time it evaluates s.plotTree. I think this behaviour should be changed, and I changed it when I added the bounds argument. It is technically in a different category, so it should be written with a separate PR. However, I thought it was better to make one complex PR rather than many PRs with a tiny change.

If you like it, I will split it.

After splitting it again, the original PR is split into 5 PRs.

(
Technically, this PR should be split into three:

  1. add bounds arg to {}.scope;
  2. add bounds arg to s.plotTree;
  3. make s.plotTree a singleton window...

Oh... the thing I thought was one is actually 6 things. Sorry again, I misunderstood submitting PR when I did it
)

@mtmccrea
Copy link
Member

mtmccrea commented May 4, 2024

Making s.plotTree a singleton window is a separate issue from adding the bounds argument.

Then it should probably be a different PR

BTW some git concepts like rebasing, branching, squashing that may help you with this kinds of project splits and updates. There is info on our wiki, and I recently make a tutorial on using GitHub Desktop for this kind of thing.

@prko
Copy link
Contributor Author

prko commented May 4, 2024

Thanks! I will do it!

@prko
Copy link
Contributor Author

prko commented May 5, 2024

@mtmccrea

Then it should probably be a different PR

I spilt only making .plotTree singleton Window from this PR.
It has now one more Merge Note:
When this PR is approved or merged, the following PR should be checked to see if each of them has already been approved or merged.

@mtmccrea mtmccrea changed the title Add bounds arg to {}.scope & s.plotTree; make s.plotTree a singleton window Add bounds arg to {}.scope & s.plotTree May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has merge note The Conversation has a note concerning how/when this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants