-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
additional, custom, local info in Peer #23
Comments
Follow up: experimenting with the ...unless switching to |
I'm not sure I understand why this would have to be a part of the library 🤔 For example, I use MultipeerKit in AirBuddy, and I do keep track of "reachability" for each peer, without the need for MultipeerKit to have anything built-in for that purpose. The goal of MultipeerKit is to remove the boilerplate code required to get a MultipeerConnectivity session up and running and add a nice Swift layer on top of message exchanges, based on Keeping track of state for the purposes of the specific type of communication that's being performed by the client application is outside the scope of this library. Something like a "heartbeat" is an implementation detail of whatever communication protocol you're building on top of MultipeerKit, so it seems weird that the library would need to provide an implementation for that. Could you elaborate on why it's not possible for your app to manage this state without extending the library? |
P.S.: I would be open to including some way of associating an arbitrary piece of state with a given |
I agree adding a specific detail is beyond the scope of this library and should be kept outside (which is why I was thinking out loud about how to add additional data in a transparent and light way). From an app project perspective (e.g. my rewritten Exhibition app), communicating to other apps will end up with 99% being covered by MultipeerKit and a small amount of state (e.g. heartbeat) for each peer would then need extra glue code to keep the additional info in sync with the "owner of the peer" - which is the tranceiver. Let me try this out in another PR to see the effect, maybe I was too reluctant to try it in the first place... ...and thanks for the feedback. |
A draft PR how a new
So I think it's doable, still the new type and the internal mapping from |
I think #25 shows a way for users of the library to solve this use case in their code. I do believe I have a different approach that I'd prefer to use in the sample code provided with MultipeerKit, so I'll try to get a PR with that up as soon as I can for you to have a look. |
Thank you for the feedback and for this library, I have successfully used it in my upcoming project and it works like a charm. I will use it in another project soon, which needs the additional local infos we discussed here. Maybe within this context I can then test another approach and see whether it will work (the idea is to move to a generic for ps: for housekeeping, I'm fine with rejecting #24 and #25 - same goes with closing this issue, if you want... Again thanks for the feedback so far (and have a nice wwdc week ahead) |
That's awesome! I'll close the PRs, but leave this issue opened to remind me to update one of the sample projects with a suggested approach. Enjoy WWDC as well |
Hi,
although I try to minimize states, in an upcoming project I could use additional, local state information inside a
Peer
, e.g.isConnected
Normally I would subclass
Peer
but MultiPeerKit would not use it obviously. In rare cases I would use objc dynamic runtime and add an "object association" butPeer
is a struct.Currently, a "wrapper" would be needed to map an additional local struct to a peer. This creates update problems, keeping the local struct and the peer list in sync - an ugly and weak approach.
I thought about the common
userInfo
dict approach eachPeer
could offer for custom key-values. Sadly this is not very Swift-friendly although a local extension with computed properties could hide the dict.This
userInfo
dict is probably the least invasive approach for the current code, or do you have another idea in mind that is better suited?The text was updated successfully, but these errors were encountered: