search

spatialaudio / jackclient-python

🂻 JACK Audio Connection Kit (JACK) Client for Python :snake:
https://jackclient-python.readthedocs.io/
MIT License
132 stars 26 forks source link

DOCS: add warnings about interacting with jack daemon from callbacks #31

Closed mk-fg closed 8 years ago

mk-fg commented 8 years ago

As mentioned in #30 and on the jack-devel mailing list:

You cannot make any calls that talk to the server from within a callback from the server.

This PR adds a "Warning:" to every non-RT (where such calls seem to be expected) callback function description and same info to the README section that mentions callbacks.

mk-fg commented 8 years ago

I did test same (only slightly modified) example as posted on the ML with client_registration_callback and port_connect_callback btw, all seem to time-out in exactly same fashion.

mgeier commented 8 years ago

Thanks for the PR and thanks for testing this, that's really good to know.

Did you also try it with the blocksize callback? This seems to be a somewhat special case ...

I think it's not good to add this stuff to the README, it only complicates matters. In fact, I just removed some details from the README: 092623ac9f6e38f6382b4a8f6b503bb3956c0e53.

I find a "warning" for this a bit heavy, a "note" would surely be enough. Probably it would be even enough to write it as a separate paragraph without any special decoration.

Actually, I'm thinking about removing the "note" markup from this sentence from the set_*_callback() methods: "This function cannot be called while the client is activated (after activate() has been called)." That many "note" blocks may become a bit annoying, what do you think?

mk-fg commented 8 years ago

I think it's not good to add this stuff to the README, it only complicates matters.

Yeah, I agree, this probably belongs to callback docs.

I find a "warning" for this a bit heavy, a "note" would surely be enough.

Don't really have opinion either way - I think it can be a warning, note, or part of a spec/description, depending on how you phrase it.

That many "note" blocks may become a bit annoying, what do you think?

I think separate "note" blocks make it way easier to process these tips, i.e. you don't glaze over them as one text chunk, but rather take mental notes of each one individually.

Plus, it's a good markup for when you only want to know what the method is about (i.e. description) and whether you want it (or e.g. some other method), and don't care about caveats (unless it's the one you want), so I think it's actually better to move stuff like All “notification events” are received ... into such notes, and keep generic descriptions in the same way you'd have first line of a git commit.

Note that this might not be well-informed or even thought-out opinion.

iteract -> interact (or communicate)?

Yeah, definitely a typo, copy-pasted all over the place ;(

Noted other comments as well.

Given that you want to shuffle stuff around anyway, maybe it'd be easier for you to just put these wherever you like (as I suspect reviewing this PR already took more of your time than just copy-pasting that line properly yourself)?

If not, can definitely do it, would you like it to be a warning, note or part of a description though?

mgeier commented 8 years ago

Thanks for your comments!

reviewing this PR already took more of your time than just copy-pasting that line properly yourself

It's not about me telling you what's proper (because I don't know that yet), I'm more interested in some discussion and your statements already helped me a lot.

It sounds reasonable to keep the notes which are already there, and I now agree with you that the new repeated "warnings" should also get their own boxes.

But I still think actual "warnings" are too strong, I think I would prefer "notes".

There may be cases where two notes come next to each other, but I guess it's no problem to have multiple notes in a row, right?

If you agree, could you please rebase and amend your PR accordingly?

mk-fg commented 8 years ago

There may be cases where two notes come next to each other, but I guess it's no problem to have multiple notes in a row, right?

I think it's better than two paragraphs, as it's easier to skim through these and there's no ambiguity about their relation to each other.

If you agree, could you please rebase and amend your PR accordingly?

Yeah, sure, will do.

mk-fg commented 8 years ago

Rebased, changed the message as suggested.

Did you also try it with the blocksize callback? This seems to be a somewhat special case ...

Just tried with mod to the same script - https://gist.github.com/mk-fg/392ad642a33956950ff6 - disconnects in similar fashion on the first call into that callback, apparently.

mgeier commented 8 years ago

Cool, thanks for the changes and for testing!

Merging ...

If you have further suggestions for improvements, don't hesitate to speak up!

mk-fg commented 8 years ago

Thanks! Will do, if I'll ever get to use JACK in a python project again.

mgeier commented 8 years ago

Great!

If you need a more cross-platform audio solution for Python (using PortAudio), you should check out https://github.com/spatialaudio/python-sounddevice!

mk-fg commented 8 years ago

Don't think I ever worked on anything audio-related outside of this one-off project (and some minor hacks for controlling pulse on my workstation), so not much of a target audience for such stuff, I'm afraid, but will keep in mind if I'll need to do something like that.