PhilipsHue / PhilipsHueSDK-Java-MultiPlatform-Android

The Software Development Kit for Philips Hue Java Mulfi-Platform and Android (beta)
273 stars 214 forks source link

com.philips.lighting.model.PHBridge#removeSchedule(String, PHScheduleListener) is not working #5

Closed lorenzfischer closed 10 years ago

lorenzfischer commented 10 years ago

Hi

The removeSchedule() method provided on the PHBridge object seems to be broken. When I call it my schedules don't get deleted from the bridge and the listener is not called on any of its methods.

I decompiled the code from the SDK and it seems to me that the problem is that the PHLocalBridgeDelegator1_0#removeSchedule() method, which calls PHNotificationManagerImpl.getNotificationManager() instead of PHNotificationManagerImpl.getInstance(). The getNotificationManager() just returns the value currently set on PHNotificationManagerImpl#notificationManager instead of creating such an instance. As the return value of this call is NULL, the removeSchedule() method returns on line 1042 without having removed the schedules.

Can you have a look into this?

Best, Lorenz

p.s. Are there any plans on opensourcing the API? That way I could fix it myself and attach a pull request.

lorenzfischer commented 10 years ago

I investigated this a bit more and realized that this NotificationManager is created when a new SDK instance is instantiated and that calling PHHueSDK.destroySDK() will remove the reference to the manager. As all calls to PHHueSDK.create() and PHHueSDK.getInstance() return a reference to the same SDK object, calling destroySDK() on any of these references will remove the NotificationManager and the createSchedule and removeSchedule() methods will stop working.

Hence, my workaround for now is not calling PHHueSDK.destroySDK() in the onDestroy() methods of the activities of my app. I don't know what (longterm) implications this will have on my app, yet.

SteveyO commented 10 years ago

Apologies in the delay in responding. Yes, what you say is correct. The destroySDK should only be called when exiting the App. However, usually to make a bridge call you would do something like: PHHueSDK phHueSDK = PHHueSDK.getInstance(); PHBridge bridge = phHueSDK.getSelectedBridge(). bridge.removeSchedule(...

So even if you have destroyed the SDK in another activity, this will create a new instance (and a new Notification manager instance) so you should be fine in most situations.

With regards to opensourcing the API, we are examining a range of ways of improving our developer program and this is one idea under consideration. I don't have any indication of timings as of yet.

lorenzfischer commented 10 years ago

No problem - it's not a pressing issue ;-)

Regarding the PHHueSDK.getInstance() method: I currently don't have access to the decompiled code, but if I remember correctly, I think that method returns the singleton instance. If any piece of code previously called the destroy method, that returned SDK instance has a "destroyed" NotificationManager. Calling PHHueSDK.create() merely calls PHHueSDK.getInstance(), so the same problem exists there, as well.

As I understand the Android App lifecycle, there is no clear "exiting the App". There are just

lorenzfischer commented 10 years ago

No problem - it's not a pressing issue ;-)

Regarding the PHHueSDK.getInstance() method: I currently don't have access to the decompiled code, but if I remember correctly, I think that method returns the singleton instance. If any piece of code previously called the destroy method, that returned SDK instance has a "destroyed" NotificationManager. Calling PHHueSDK.create() merely calls PHHueSDK.getInstance(), so the same problem exists there, as well.

As I understand the Android App lifecycle, there is no clear "exiting the App". Where do you suggest I insert the destroy() method?

SteveyO commented 10 years ago

Hi again, PHHueSDK.create() does infact call the PHHueSDK.getInstance() as you say, however, calling getInstance() also creates the NotificationManager instance, so if it has been nulled (by calling destroy method) then it will be re-created and will carry on functioning normally. I just ran a quick test by connecting to a bridge, setting lights, destroying the sdk, getting the instance again and setting lights again and it worked as I had expected, so I can't see any issues here. If you do find any issues let me know of course.

Its been a while since I did any Android development but I would have thought that destroying the SDK inside the onDestroy method (of your main Activity) should suffice, or inside onPause method if you want notifications to stop if the App is brought to the background.

lorenzfischer commented 10 years ago

Ok, I'll try that. Thanks!