search

otalk / TLKSimpleWebRTC

MIT License
28 stars 21 forks source link

TLKSimpleWebRTC improvements #3

Closed piemonte closed 9 years ago

piemonte commented 9 years ago

hey there,

my friends and i decided to use your stack for a little fun project. along the way, i figured i could start contributing some changes.

this pull request has a few minor changes but i'll soon be making more major changes, so figured i'd submit this before the heavy pass. see also:

https://github.com/otalk/TLKWebRTC/pull/3

thanks for the nice libraries,

patrick

piemonte commented 9 years ago

btw, i noticed the interface uses blocks, delegation, and KVO. is there a preference to one method?

i'd like to remove the KVO requirement.

hjon commented 9 years ago

Thanks for the proposed changes! I'd love to see what you put together, if the project is public.

(I'm using this reply as part of https://github.com/otalk/TLKWebRTC/pull/3 for convenience and completeness.)

Would you be able to split up the changes into smaller pull requests? That's our preferred process and makes it a bit easier to go through than cherry-picking, as well as allowing comments/changes in a more fine-grained manner. If not, I still intend to go through them within a few weeks; it'll just take a bit longer as my attention is elsewhere at the moment.

hjon commented 9 years ago

Maybe I'm forgetting something, but I didn't think we had a KVO requirement. Could you point that out?

My current tendency is as follows: If the block of code will be called multiple times, use delegation. If the block of code is only called once per unique "operation" (more in the style of completion handler), use blocks. KVO is an added bonus, but not a requirement. That's not always an option and doesn't fit every use case (AZSocketIO, current WebRTC API, enumerateObjectsUsingBlock:, etc.), but that's my starting point and general preference.

I'm open for discussion on that, which is part of the reason smaller pull requests are nice. You're definitely right that it would be good to be more consistent with that.

piemonte commented 9 years ago

hey @hjon. sorry for the delay in getting back to you. unfortunately, i don't have time to split these up, sorry if it'll take longer.

on a lighter note is that most of the changes don't effect the overall logic but mostly objc convention type-stuff. over the coming weeks i may have more logic specific changes to share.

there was some exposed properties of the lower level code that use KVO:

https://github.com/otalk/TLKSimpleWebRTC/blob/master/TLKSocketIOSignaling.h#L31-L32

that's all i was curious about, thanks for the guidance. hopefully i'll have some more good stuff to share in the coming weeks.

hjon commented 9 years ago

Thanks for the patch! Sorry it took so long to get to. Overall it looks good to me, so I'm merging it in. In the interest of "least surprise", I want to mention that the style may change in the future. Some things differ from my usual practice, but since I don't feel too strongly about them, I'd rather accept a consistent approach (at least until I have stronger opinions and document them). :smiley:

piemonte commented 9 years ago

:+1: