Skip to content

Aligning with UTransport::register_listener() and UTransport::unregister_listener() with UListener#67

Closed
PLeVasseur wants to merge 4 commits intoeclipse-uprotocol:mainfrom
PLeVasseur:feature/ulistener
Closed

Aligning with UTransport::register_listener() and UTransport::unregister_listener() with UListener#67
PLeVasseur wants to merge 4 commits intoeclipse-uprotocol:mainfrom
PLeVasseur:feature/ulistener

Conversation

@PLeVasseur
Copy link
Contributor

@PLeVasseur PLeVasseur commented Mar 15, 2024

Hey there 👋

Based on #65, I took my shot at aligning our UTransport trait closer to the spec.

In particular, I have removed the requirement for the implementation to explicitly manage listeners using String identifiers, instead leaning on concrete implementations of the UListener trait.

This change is to align with registerListener() and unregisterListener() by bringing in a UListener trait.

As you read the code, you can definitely see there were compromises that had to be made to accomplish this.

Let me list the ways:

  • UListener must have another function as_any() which must be implemented by the concrete impls of it to always return self
    • IMHO not a huge deal, but we could go a step farther by writing a proc macro that people would call instead which could ensure this is always performed
    • I'm also open to any suggestions on how UListener could be tweaked to remove this requirement!
  • If we wish to unregister_listener(), we must instantiate another Box<dyn UListener> from the concrete type to do so
    • Kinda annoying, but not a deal-breaker IMHO

In addition, it does make it quite a bit more annoying because instead of being able to pass in any boxed function anywhere that meets the signature, you must now first implement UListener for each and every different function that you wish to be able to register with UTransport. This is not a small change and pretty annoying in my opinion.

I'm looking to see how this would impact current and future implementations of UTransport for UPClients. Feedback would be great from @AnotherDaniel, @sophokles73, @evshary in particular 🙂

@AnotherDaniel
Copy link
Contributor

Hm - we explicitly removed the UListener trait approach in favour of a simple listener ID, back in the good old days (round of reviews for the initial PR).
To be honest, every time I'm beginning to play around with code implementing one of these interfaces, something more fundamental comes along (like up-rust improvements... many to go), and I get detracted again - so myself I've still not really done code that actually uses these interfaces :-(

So on this question, I'd look at @sophokles73 for informed opinions, for now.

@PLeVasseur
Copy link
Contributor Author

Closing in favor of #68 which resolves all of the issues I had with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants