kirillzyusko / react-native-wifi-p2p

Library that provide access for working with wi-fi direct (p2p) module in android.
164 stars 32 forks source link

sendMessage and receiveMessage refactoring? #66

Closed viniciuscb closed 1 year ago

viniciuscb commented 1 year ago

I am having another problem now, regarding the sendMessage method. Sometimes the connection is closed and I could not have this information before sending another message. This kills the app. Here is the problem, because groupOwnerAddress is null and I cannot call getHostAddress() on null.

Besides that, I think there should be some way to stop receiving messages. If I call receiveMessage() and (unexpectedly) don't receive any message, a port will stay open waiting for messages. This is a real issue when I call it in a screen, go out and enter this screen again.

Maybe adding a stopReceivingMessage() method or replacing for two methods, startReceivingMessages() and stopReceivingMessages()

kirillzyusko commented 1 year ago

Hi @viniciuscb

I think it make sense to add stopReceivingMessage and stopReceivingFile 🙂 Would you mind to contribute and add these methods? 👀

Sometimes the connection is closed

Would be good to understand the reason behind it, i. e. why the connection was closed. Is it a bug in a library or it depends on external factors, such "user closed app"/"distance to receiver side became too big and because of that the signal was lost"? Also maybe would be good to send an event "CONNECTION_CLOSED" or something like that, if we actuall can derive this event.

viniciuscb commented 1 year ago

Hi Kirill. The problem with sendMessage is happening due to external factors. For instance, when the app crashes/is closed in one device, and I didn't receive yet (in the other device) the information that the connection changed in react native level (through listeners/subscribe methods).

kirillzyusko commented 1 year ago

Got it @viniciuscb 👍 Then I think we'll need to have stopReceivingMessage/stopReceivingFile methods and it would be very helpful to know, when "CONNECTION_CLOSED" event occured (to actually call stopReceivingMessage/stopReceivingFile methods). Agree?

P. S. I'm going to create 3.4.0 release, but I'll be glad to release 3.5.0 with new API methods if you submit PR 😊

viniciuscb commented 1 year ago

I see now that the AsyncTask class became deprecated in API 30. So the receiveMessage will need to have a refactoring anyway.

From: https://developer.android.com/reference/android/os/AsyncTask

AsyncTask was intended to enable proper and easy use of the UI thread. However, the most common use case was for integrating into UI, and that would cause Context leaks, missed callbacks, or crashes on configuration changes. It also has inconsistent behavior on different versions of the platform, swallows exceptions from doInBackground, and does not provide much utility over using Executors directly.

Besides that, @kirillzyusko and @seba9999 , maybe we can pass an optional parameter to receiveMessage to receive also the metadata (currently only the origin IP address). This way we can maintain backward compatibility.

kirillzyusko commented 1 year ago

@viniciuscb I think if you rewrite and migrate away from AsynTask, then you may not worry about backward compatibility :)

In my opinion it's useful to know sender address - of course it will be breaking change, but it will be easy to migrate.

Moreover I already have some deprecated methods in API and I would like to remove them and publish new 4.0.0 version :)

viniciuscb commented 1 year ago

I've proposed a PR ( #80 ) that does not break backward compatibility.