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

class library: refactor functionality: connectToServerAddr #5569

Conversation

telephon
Copy link
Member

@telephon telephon commented Sep 9, 2021

Purpose and Motivation

While looking for a fix of #5568, a small refactoring was recommended.

Types of changes

  • Bug fix

To-do list

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

@dyfer dyfer added the comp: class library SC class library label Sep 9, 2021
@dyfer
Copy link
Member

dyfer commented Sep 9, 2021

Thanks so much!
(especially that the linked issue was introduced by my commit...)

  • Maybe it would be good to add the documentation for the new method in the help...?
  • Do you think this should be cherry-picked for 3.12 or is it fine to stay on develop?

@telephon
Copy link
Member Author

I'm still usure whether connectToServerAddr is the best name for the method – it seems misleading when taken out of context. Whe only connect if necessary (for tcp), otherwise we directly call the onComplete message.

In fact, I would have thought it good to pack the startAliveThread into it, but the method boot passes this only when the argument startAliveThread = true (sorry, this is all a bit complicated to explain).

@dyfer
Copy link
Member

dyfer commented Sep 11, 2021

In fact, I would have thought it good to pack the startAliveThread into it, but the method boot passes this only when the argument startAliveThread = true (sorry, this is all a bit complicated to explain).

Yeah... the whole Server class and boot behavior was mean to be rewritten, but it seems it never happened. Not sure how much we want to change before the (hopefully) rewrite later on, and how much we should resort to minimal patching.

@telephon
Copy link
Member Author

@adcxyz did a rather large rewrite long ago, but as far as I know, it somehow was never taken up and merged.

@adcxyz
Copy link
Contributor

adcxyz commented Sep 15, 2021

@adcxyz did a rather large rewrite long ago, but as far as I know, it somehow was never taken up and merged.

Yes, that was discussed here : #3310
and the related fork that sketched this out: https://github.com/adcxyz/supercollider/tree/server_ServerProcess

@telephon
Copy link
Member Author

telephon commented Sep 17, 2021

Ah good, thanks for pointing to this. As you wrote, the branches have diverged. What do you think: is the current PR doing harm (I am always ready to minimize impact at the cost of beauty :))

@telephon telephon requested a review from adcxyz January 8, 2022 14:34
@telephon
Copy link
Member Author

telephon commented Jan 8, 2022

what do you think?

Copy link
Contributor

@adcxyz adcxyz left a comment

Choose a reason for hiding this comment

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

looks good to me, clearer than before.

@adcxyz adcxyz merged commit c3301b8 into supercollider:develop Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants