Skip to content
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

Proposed simplification of CoT event dispatching #41

Closed
a-f-G-U-C opened this issue Oct 10, 2020 · 2 comments
Closed

Proposed simplification of CoT event dispatching #41

a-f-G-U-C opened this issue Oct 10, 2020 · 2 comments

Comments

@a-f-G-U-C
Copy link
Contributor

The change from PR #39 works as intended and fixes issue #35 (but not #34 since it only dispatches internally). However, since API methods are now used that provide appropriate abstraction, the additional layer of abstraction via mCotDispatcher seems to me somewhat redundant.

I suggest that rewriting the function retransmitCotToLocalhost() to invoke the dispatcher methods directly could reduce some of the complexity without impacting the code structure - and fix issue #34 in one fell swoop.

Here's what worked well for me (in InboundMessageHandler.java ):

import com.atakmap.android.cot.CotMapComponent;
[...]

    public void retransmitCotToLocalhost(CotEvent cotEvent) {
        CotMapComponent.getInternalDispatcher().dispatch(cotEvent);
        CotMapComponent.getExternalDispatcher().dispatch(cotEvent);
    }

(the second line dispatches the CoT event externally, fixing issue #34)

I'm reluctant to submit a PR around this change since it should also include a cleanup of the InboundMessageHandler class - a task requiring a level of familiarity with your code that I don't have yet. :) But I hope it helps nonetheless.

@paulmandal
Copy link
Owner

Thanks for letting me know that this solution wasn't complete!

#43 fixes this by using both the internal and external CotDispatchers in InboundMessageHandler.retransmitCotToLocalhost(CotEvent)

I left the mInternalCotDispatcher and mExternalCotDispatcher abstraction in InboundMessageHandler because it allows for the injection of mocks through the constructor for those dependencies if I ever get around to writing testing code for this codebase.

@a-f-G-U-C
Copy link
Contributor Author

I had a feeling that I was missing something important! :)
Tested and works as intended, so I'll close this issue now.

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

No branches or pull requests

2 participants