Videodock / cordova-plugin-chromecast

Control Chromecast from Cordova
Other
1 stars 2 forks source link

Remove sharedPreferences and NSUserDefaults for storing the app id #2

Closed ChristiaanScheermeijer closed 4 months ago

ChristiaanScheermeijer commented 1 year ago

This PR updates the app ID configuration because we found some issues due to using NSUserDefaults and SharedPreferences.

Tested on iOS and Android devices

ChristiaanScheermeijer commented 1 year ago

Yes indeed. Because of the usage of these storages, the receiver id is persisted once connected. This never seems to be overridden, and it creates an issue when the receiver ID is changed.

This mechanism is in place because the app needs the receiver ID on startup. But you shouldn't manually edit the iOS/Android projects in Cordova apps. For Capacitor apps, this isn't the case anymore. So now we can configure the receiver IDs in the native configuration instead of using the workaround previously implemented.

wouterbin commented 1 year ago

Hi,

We are using your fork of this plugin and implemented it recently. We had to replace https://github.com/ConnectSDK/Connect-SDK-Cordova-Plugin with a Capacitor compatible alternative. This plugin is, thanks!

The proposed changes seem counter intuitive to me. The plugin gives the possibility to start a session given a certain appId:

new chrome.cast.SessionRequest('ABCD1234')

With these changes the argument can be given but is silently ignored. I think the user should be warned that this is not the way to use this method or it should work as expected. As mentioned the plugin advocates that it implements the cast sender web SDK API. In my opion this breaks it ☺️ .

Question: What is the use case for changing the appId? Is this used in production apps?

ChristiaanScheermeijer commented 1 year ago

Hi @wouterbin,

Thank you very much for your feedback! I'm happily surprised that this fork is being used πŸ˜„

You're right. This change will make it impossible to change the appId from the web app. But there are a few reasons for this.

I will add a warning and update the readme.

wouterbin commented 1 year ago

Thanks for the changes βœ… !

I'm happily surprised that this fork is being used πŸ˜„

Yeah, it was at the time (probably still is) the most maintained fork. So it was an obvious choice πŸ˜‰. Keep up the good work πŸ‘ . Would you be interested if we stumble upon issues with the plugin (The issues on this repo are disabled now)?

ChristiaanScheermeijer commented 1 year ago

@wouterbin yes, sure! I've enabled issues. Please let us know if you have any issues.