zbkoong / android-rcs-ims-stack

Automatically exported from code.google.com/p/android-rcs-ims-stack
0 stars 0 forks source link

API inconsistency - ImsApi does not follow the same usage pattern as other *Api classes #62

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I was trying to build a generic API wrapper that connects various APIs offered 
and handled disconnects. The goal was to make sure we are always using the 
"connected" API instance and if not - attempt to connect and fail if there is 
no  handleApiConnected() call within certain short period.

It seems that this pattern does not work for ImsApi. I see that all these *Api 
classes are subclasses of ClientApi abstract class. However, unlike all other 
implementations, ImsApi does not have its own connectApi() method 
implementation and ClientApi.connectApi() gets effectively called when calling 
ImsApi.connectApi(). 

ImsApi.connectApi() uses ClientApi.imsApiConnection to monitor the service 
connection. Upon receiving the call to its onServiceConnected() method it 
dispatches the call to ClientApi.notifyEventImsConnected() which, in its turn, 
delivers the notification to all ImsEventListener instances registered.

This makes ImsApi different from other implementations. All other 
implementations have their own apiConnection fields and they all call 
ClientApi.notifyEventApiConnected() which informs all ClientApiListener 
instances. Moreover, I see that ImsApi class also has its apiConnection field 
which is not used anywhere. 

It appears to me that there is certain overlap between the base class ClientApi 
and ImsApi subclass (and IImsApi interface). ClientApi has method 
isImsConnected() which seems to be more appropriate for ImsApi.

Original issue reported on code.google.com by ngrigor...@gmail.com on 4 Apr 2012 at 7:49

GoogleCodeExporter commented 9 years ago
In fact we want to have an IMS API (methods/events) common to the other API 
(messaging, richcall, etc). So today there is no connectApi method in the IMS 
API because this is used via the other AP.
Then I think is no very clean as you said.
The solution may be to well separate the IMS API from the other API.

Original comment by jmauffret@gmail.com on 5 Apr 2012 at 6:12

GoogleCodeExporter commented 9 years ago
Yes, I understood it. And I would agree that separating IMS API would be a 
possible solution. Sorry if I did not make myself clear. What I was trying to 
say is that ClientApi base class defines certain contract (connectApi() + 
listeners) for managing the API instance lifecycle. ImsApi, while being a 
subclass of ClientApi, does not follow this contract. All other subclasses of 
ClientApi follow it.

Original comment by ngrigor...@gmail.com on 5 Apr 2012 at 12:58

GoogleCodeExporter commented 9 years ago
To add to it, after carefully looking at ImsApi code it becomes clear that its 
isImsConnected() method always returns null. Its coreApi property is never set 
to anything - because its apiConnection property is never used. 

Original comment by ngrigor...@gmail.com on 5 Apr 2012 at 1:31

GoogleCodeExporter commented 9 years ago
I have internalize the isConnected method and rmove the public IMS API. This 
will be available in the next release.

Original comment by jmauffret@gmail.com on 12 Apr 2012 at 4:15