Refactor one client patching

[sandreae]

No description provided.

[sandreae]

Still need a couple more tests...

[sandreae]

Need to test this once more now that I moved the disconnect and reconnect methods, the server's in use today though, so will do it tomorrow. Once we've merged this we can rewrite the pypatcher tests to be more thorough +1.

[sandreae]

I've tested this branch out more, up to 5 clients and booting from the automated script, all seems to be fine. Would be good to merge this if you're happy with it @madwort .

[madwort]

Sorry this is still hanging around - will try to make time to review it soon!

[madwort]

I think it would be worth splitting the lounge_music connections into a separate function to the one client connections here - with an eye to the feature requests list, it would help us if we want to send the lounge_music to darkice while we're soundchecking a session.

[madwort]

Lots of good stuff in the code here, a couple of minor things I think it would be good to tidy before merging - comment quality & where we patch the lounge_music. What you've done here is good for the way we've been using it, but maybe we're going to use it slightly differently soon...

[sandreae]

Hey, thanks for the review, I started fixing the comments then wondered if the naming was a bit off actually. I feel something like this is a bit clearer and concise. What do you think?

These are the main publicly called module functions:

disconnect_all(...)
connect_ports(...)
set_darkice_connections(...)
set_lounge_music_connections(...) <-- new
set_client_connections(...) set_all_connections(...)
make_connections(...) make_all_connections(...)

These are private functions (in which case they could be prefixed with _, right?):

connect_darkice_one_client(...) set_darkice_connections_one_client(...)
connect_one_client(...) set_connections_1_clients(...)
connect_two_clients(...) set_connections_2_clients(...)
connect_three_clients(...) set_connections_3_clients(...)

[madwort]

Hey, thanks for the review, I started fixing the comments then wondered if the naming was a bit off actually. I feel something like this is a bit clearer and concise. What do you think?

yeah, good thoughts!

These are the main publicly called module functions:

disconnect_all(...)
connect_ports(...)
set_darkice_connections(...)
set_lounge_music_connections(...) <-- new
set_client_connections(...) set_all_connections(...)
make_connections(...) make_all_connections(...)

yeah, this looks good, maybe we could go further & find a better word for "set" too, just thinking, with the current 2-phase thing ("set" them, then "make" them) maybe it should be "generate" (i.e. "generate" them, then "make" them), not sure, there's probably a better word that I'm missing...

These are private functions (in which case they could be prefixed with _, right?):

Ah, probably, yes, I'm not the most python-ish person, I'm used to either having explicit private methods - sounds good to differentiate them though.

connect_darkice_one_client(...) set_darkice_connections_one_client(...)
connect_one_client(...) set_connections_1_clients(...)
connect_two_clients(...) set_connections_2_clients(...)
connect_three_clients(...) set_connections_3_clients(...)

I'm not sure - if we're sticking with the "set" verb above, and these are sub-functions of that, they should probably stay as "set", although e.g. set_client_connections_one_client might be better?

[sandreae]

yeah, this looks good, maybe we could go further & find a better word for "set" too, just thinking, with the current 2-phase thing ("set" them, then "make" them) maybe it should be "generate" (i.e. "generate" them, then "make" them), not sure, there's probably a better word that I'm missing...

Yeh, I don't like set in this context either, generate could be good.

I'm not sure - if we're sticking with the "set" verb above, and these are sub-functions of that, they should probably stay as "set", although e.g. set_client_connections_one_client might be better?

Yeh, I actually thought they should be differentiated a bit more because of the parent / child nature of their relationship. If we use a _ though with the private methods I think this is enough.

I'll make these changes and see how it looks.

[sandreae]

I kept with set in the end just cos generate is too long...

lounge_music is refactored, which makes for some nicely readable code here:

jacktrip_pypatcher/jacktrip_pypatcher.py

Lines 108 to 114 in 66da550

	jcp.set_lounge_music_connections(
	jacktrip_clients, darkice_port, lounge_music.port
	)
	jcp.set_client_connections(jacktrip_clients, ladspa_ports)
	jcp.set_darkice_connections(
	darkice_ladspa_ports, darkice_port, jacktrip_clients
	)

It is a fairly major change though so I do wanna spin up a server to test all this is working as expected.

[sandreae]

What do you think about the code in all of these connection methods?:

jacktrip_pypatcher/jack_client_patching.py

Lines 63 to 68 in 66da550

	def _set_darkice_connections_one_client(self, jacktrip_ports, darkice):
	jacktrip_receive = self._jacktrip_receive(jacktrip_ports[0])
	darkice_send = self._darkice_send(darkice)
	self.connections.append((jacktrip_receive, darkice_send))

Should they be re-written like so (maybe with abbreviated naming?):

def _set_darkice_connections_one_client(self, jacktrip_ports, darkice):  
     self.connections.append((self._jacktrip_receive(jacktrip_ports[0]), self._darkice_send(darkice))) 

I think I prefer the former for readability, but the latter is many fewer lines of code, and with abbreviated names could be fairly compact....

[sandreae]

I mean, if we do that it's hardly worth having the helper method, as it's only used once here....

jacktrip_pypatcher/jack_client_patching.py

Lines 172 to 181 in 66da550

	def set_darkice_connections(self, ladspa_ports, darkice, jacktrip_ports=[]):
	if len(jacktrip_ports) == 1:
	self._set_darkice_connections_one_client(jacktrip_ports, darkice)
	return
	for ladspa in ladspa_ports:
	self.connections.append(
	(self._ladspa_receive(ladspa), self._darkice_send(darkice))
	)