insidegui / MultipeerKit

MultipeerConnectivity + Codable = ❤️
BSD 2-Clause "Simplified" License
1.1k stars 71 forks source link

additional, custom, local info in Peer #23

Open omichde opened 2 years ago

omichde commented 2 years ago

Hi,

although I try to minimize states, in an upcoming project I could use additional, local state information inside a Peer, e.g.

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" but Peer 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 each Peer 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?

omichde commented 2 years ago

Follow up: experimenting with the userInfo approach, this does not work as Peer is currently a value type and (privately) owned by the Tranceiver and hence not easily mutable...

...unless switching to Peer as a class - see https://github.com/insidegui/MultipeerKit/pull/24 (just to easily see diffs) ...if already on this dirty road, subclassing from NSObject then set_objc_assoc would be possible >;)

insidegui commented 2 years ago

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 Codable.

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?

insidegui commented 2 years ago

P.S.: I would be open to including some way of associating an arbitrary piece of state with a given Peer, without modifying the semantics of the type like you've done in #24

omichde commented 2 years ago

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.

omichde commented 2 years ago

A draft PR how a new ChatPeer type could look like: #25

So I think it's doable, still the new type and the internal mapping from Peer to ChatPeer feels like an inelegant effort for "only" having a history property alongside each Peer - but that's probably the price to be paid for a generic library ;)

insidegui commented 2 years ago

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.

omichde commented 2 years ago

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 Peer and let MultipeerKit use even the extended GamePeer I've used in #25 - sadly I'm not that familiar with generics and will have to try it out...).

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)

insidegui commented 2 years ago

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