Skip to content

Commit 4ccf293

Browse files
bilal-alsharifibilal-alsharifi
authored
Merge pull request #1277 from smartdevicelink/feature/issue_1275
Update SCM to notify listeners if fresh copy of capability is received
2 parents d2d34d7 + b1edc68 commit 4ccf293

2 files changed

Lines changed: 159 additions & 17 deletions

File tree

‎android/sdl_android/src/androidTest/java/com/smartdevicelink/test/proxy/SystemCapabilityManagerTests.java‎

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,7 @@ public void testAddOnSystemCapabilityListenerWithSubscriptionsNotSupportedAndCap
457457
// Add listener1
458458
// When the first listener is added, GetSystemCapability request should out because because capability is not cached
459459
OnSystemCapabilityListener onSystemCapabilityListener1 = mock(OnSystemCapabilityListener.class);
460-
doAnswer(createOnSendGetSystemCapabilityAnswer(true, true)).when(internalInterface).sendRPC(any(GetSystemCapability.class));
460+
doAnswer(createOnSendGetSystemCapabilityAnswer(true, false)).when(internalInterface).sendRPC(any(GetSystemCapability.class));
461461
scm.addOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener1);
462462
verify(internalInterface, times(1)).sendRPC(any(GetSystemCapability.class));
463463
verify(onSystemCapabilityListener1, times(1)).onCapabilityRetrieved(any(Object.class));
@@ -490,6 +490,129 @@ public void testAddOnSystemCapabilityListenerWithSubscriptionsNotSupportedAndCap
490490
verify(internalInterface, times(1)).sendRPC(any(GetSystemCapability.class));
491491
}
492492

493+
public void testAddOnSystemCapabilityListenerThenGetCapabilityWhenSubscriptionsAreNotSupported() {
494+
SdlMsgVersion sdlMsgVersion = new SdlMsgVersion(5, 0); // This version doesn't support capability subscriptions
495+
sdlMsgVersion.setPatchVersion(0);
496+
ISdl internalInterface = mock(ISdl.class);
497+
when(internalInterface.getSdlMsgVersion()).thenReturn(sdlMsgVersion);
498+
SystemCapabilityManager scm = new SystemCapabilityManager(internalInterface);
499+
scm.setCapability(SystemCapabilityType.VIDEO_STREAMING, videoStreamingCapability);
500+
501+
502+
// Add listener1
503+
// When the first listener is added, GetSystemCapability request should go out with subscribe=false
504+
OnSystemCapabilityListener onSystemCapabilityListener1 = mock(OnSystemCapabilityListener.class);
505+
doAnswer(createOnSendGetSystemCapabilityAnswer(true, false)).when(internalInterface).sendRPC(any(GetSystemCapability.class));
506+
scm.addOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener1);
507+
verify(internalInterface, times(0)).sendRPC(any(GetSystemCapability.class));
508+
verify(onSystemCapabilityListener1, times(1)).onCapabilityRetrieved(any(Object.class));
509+
510+
511+
// Get Capability (should notify listener1 again)
512+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
513+
verify(internalInterface, times(1)).sendRPC(any(GetSystemCapability.class));
514+
verify(onSystemCapabilityListener1, times(2)).onCapabilityRetrieved(any(Object.class));
515+
516+
517+
// Add listener2
518+
OnSystemCapabilityListener onSystemCapabilityListener2 = mock(OnSystemCapabilityListener.class);
519+
scm.addOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener2);
520+
verify(onSystemCapabilityListener2, times(1)).onCapabilityRetrieved(any(Object.class));
521+
522+
523+
// Get Capability (should notify listener1 & listener2 again)
524+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
525+
verify(internalInterface, times(2)).sendRPC(any(GetSystemCapability.class));
526+
verify(onSystemCapabilityListener1, times(3)).onCapabilityRetrieved(any(Object.class));
527+
verify(onSystemCapabilityListener2, times(2)).onCapabilityRetrieved(any(Object.class));
528+
529+
530+
// Add listener3
531+
OnSystemCapabilityListener onSystemCapabilityListener3 = mock(OnSystemCapabilityListener.class);
532+
scm.addOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener3);
533+
verify(onSystemCapabilityListener3, times(1)).onCapabilityRetrieved(any(Object.class));
534+
535+
536+
// Get Capability (should notify listener1 & listener2 & listener3 again)
537+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
538+
verify(internalInterface, times(3)).sendRPC(any(GetSystemCapability.class));
539+
verify(onSystemCapabilityListener1, times(4)).onCapabilityRetrieved(any(Object.class));
540+
verify(onSystemCapabilityListener2, times(3)).onCapabilityRetrieved(any(Object.class));
541+
verify(onSystemCapabilityListener3, times(2)).onCapabilityRetrieved(any(Object.class));
542+
543+
544+
// Remove listener1
545+
scm.removeOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener1);
546+
547+
548+
// Get Capability (should notify listener2 & listener3 again)
549+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
550+
verify(internalInterface, times(4)).sendRPC(any(GetSystemCapability.class));
551+
verify(onSystemCapabilityListener1, times(4)).onCapabilityRetrieved(any(Object.class));
552+
verify(onSystemCapabilityListener2, times(4)).onCapabilityRetrieved(any(Object.class));
553+
verify(onSystemCapabilityListener3, times(3)).onCapabilityRetrieved(any(Object.class));
554+
555+
556+
// Remove listener2
557+
scm.removeOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener2);
558+
559+
560+
// Get Capability (should notify listener3 again)
561+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
562+
verify(internalInterface, times(5)).sendRPC(any(GetSystemCapability.class));
563+
verify(onSystemCapabilityListener1, times(4)).onCapabilityRetrieved(any(Object.class));
564+
verify(onSystemCapabilityListener2, times(4)).onCapabilityRetrieved(any(Object.class));
565+
verify(onSystemCapabilityListener3, times(4)).onCapabilityRetrieved(any(Object.class));
566+
567+
568+
// Remove listener3
569+
scm.removeOnSystemCapabilityListener(SystemCapabilityType.VIDEO_STREAMING, onSystemCapabilityListener3);
570+
verify(internalInterface, times(5)).sendRPC(any(GetSystemCapability.class));
571+
572+
573+
// Get Capability (should not notify any listener again because they are all removed)
574+
scm.getCapability(SystemCapabilityType.VIDEO_STREAMING, null, true);
575+
verify(internalInterface, times(6)).sendRPC(any(GetSystemCapability.class));
576+
verify(onSystemCapabilityListener1, times(4)).onCapabilityRetrieved(any(Object.class));
577+
verify(onSystemCapabilityListener2, times(4)).onCapabilityRetrieved(any(Object.class));
578+
verify(onSystemCapabilityListener3, times(4)).onCapabilityRetrieved(any(Object.class));
579+
}
580+
581+
public void testGetAndAddListenerForDisplaysCapability() {
582+
ISdl internalInterface;
583+
SystemCapabilityManager scm;
584+
OnSystemCapabilityListener onSystemCapabilityListener;
585+
DisplayCapabilities retrievedCapability;
586+
587+
588+
// Test case 1 (capability cached, listener not null, forceUpdate true)
589+
// Force updating DISPLAYS capability should call onError()
590+
internalInterface = mock(ISdl.class);
591+
scm = new SystemCapabilityManager(internalInterface);
592+
onSystemCapabilityListener = mock(OnSystemCapabilityListener.class);
593+
doAnswer(createOnSendGetSystemCapabilityAnswer(true, null)).when(internalInterface).sendRPC(any(GetSystemCapability.class));
594+
scm.setCapability(SystemCapabilityType.DISPLAYS, new DisplayCapabilities());
595+
retrievedCapability = (DisplayCapabilities) scm.getCapability(SystemCapabilityType.DISPLAYS, onSystemCapabilityListener, true);
596+
assertNotNull(retrievedCapability);
597+
verify(internalInterface, times(0)).sendRPC(any(GetSystemCapability.class));
598+
verify(onSystemCapabilityListener, times(0)).onCapabilityRetrieved(any(Object.class));
599+
verify(onSystemCapabilityListener, times(1)).onError(any(String.class));
600+
601+
602+
// Test case 2 (Add listener)
603+
// When the first DISPLAYS listener is added, GetSystemCapability request should not go out
604+
OnSystemCapabilityListener onSystemCapabilityListener1 = mock(OnSystemCapabilityListener.class);
605+
scm.addOnSystemCapabilityListener(SystemCapabilityType.DISPLAYS, onSystemCapabilityListener1);
606+
verify(internalInterface, times(0)).sendRPC(any(GetSystemCapability.class));
607+
verify(onSystemCapabilityListener1, times(1)).onCapabilityRetrieved(any(Object.class));
608+
609+
610+
// Test case 3 (Remove listener)
611+
// When the last DISPLAYS listener is removed, GetSystemCapability request should not go out
612+
scm.removeOnSystemCapabilityListener(SystemCapabilityType.DISPLAYS, onSystemCapabilityListener1);
613+
verify(internalInterface, times(0)).sendRPC(any(GetSystemCapability.class));
614+
}
615+
493616
public void testListConversion(){
494617
SystemCapabilityManager systemCapabilityManager = createSampleManager();
495618
Object capability = systemCapabilityManager.getCapability(SystemCapabilityType.SOFTBUTTON);

‎base/src/main/java/com/smartdevicelink/proxy/SystemCapabilityManager.java‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ public void onReceived(RPCMessage message) {
312312
// this notification can return only affected windows (hence not all windows)
313313
List<DisplayCapability> newCapabilities = (List<DisplayCapability>) capability;
314314
updateCachedDisplayCapabilityList(newCapabilities);
315+
systemCapabilitiesSubscriptionStatus.put(SystemCapabilityType.DISPLAYS, true);
315316
}
316317
}
317318
if (capability != null) {
@@ -338,8 +339,8 @@ public void onReceived(RPCMessage message) {
338339
* @param capability the value of the capability that will be set
339340
*/
340341
public synchronized void setCapability(SystemCapabilityType systemCapabilityType, Object capability) {
341-
cachedSystemCapabilities.put(systemCapabilityType, capability);
342-
notifyListeners(systemCapabilityType, capability);
342+
cachedSystemCapabilities.put(systemCapabilityType, capability);
343+
notifyListeners(systemCapabilityType, capability);
343344
}
344345

345346
/**
@@ -450,16 +451,17 @@ private boolean isSubscribedToSystemCapability(SystemCapabilityType systemCapabi
450451
*/
451452
private Object getCapabilityPrivate(final SystemCapabilityType systemCapabilityType, final OnSystemCapabilityListener scListener, final Boolean subscribe, final boolean forceUpdate) {
452453
Object cachedCapability = cachedSystemCapabilities.get(systemCapabilityType);
453-
OnSystemCapabilityListener listener = scListener;
454454

455-
if (!forceUpdate && cachedCapability != null && listener != null) {
456-
listener.onCapabilityRetrieved(cachedCapability);
457-
listener = null;
455+
boolean shouldUpdateSystemCapabilitySubscription = (subscribe != null) && (subscribe != isSubscribedToSystemCapability(systemCapabilityType)) && supportsSubscriptions();
456+
boolean shouldSendGetCapabilityRequest = forceUpdate || (cachedCapability == null) || shouldUpdateSystemCapabilitySubscription;
457+
boolean shouldCallListenerWithCachedValue = (cachedCapability != null) && (scListener != null) && !shouldSendGetCapabilityRequest;
458+
459+
if (shouldCallListenerWithCachedValue) {
460+
scListener.onCapabilityRetrieved(cachedCapability);
458461
}
459462

460-
boolean shouldUpdateSystemCapabilitySubscription = (subscribe != null) && (subscribe != isSubscribedToSystemCapability(systemCapabilityType)) && supportsSubscriptions();
461-
if (forceUpdate || (cachedCapability == null) || shouldUpdateSystemCapabilitySubscription) {
462-
retrieveCapability(systemCapabilityType, listener, subscribe);
463+
if (shouldSendGetCapabilityRequest) {
464+
retrieveCapability(systemCapabilityType, scListener, subscribe);
463465
}
464466

465467
return cachedCapability;
@@ -500,7 +502,6 @@ public Object getCapability(final SystemCapabilityType systemCapabilityType) {
500502
* @param listener callback to execute upon retrieving capability
501503
*/
502504
public void addOnSystemCapabilityListener(final SystemCapabilityType systemCapabilityType, final OnSystemCapabilityListener listener) {
503-
getCapabilityPrivate(systemCapabilityType, listener, true, false);
504505
synchronized(LISTENER_LOCK) {
505506
if (systemCapabilityType != null && listener != null) {
506507
if (onSystemCapabilityListeners.get(systemCapabilityType) == null) {
@@ -509,6 +510,7 @@ public void addOnSystemCapabilityListener(final SystemCapabilityType systemCapab
509510
onSystemCapabilityListeners.get(systemCapabilityType).add(listener);
510511
}
511512
}
513+
getCapabilityPrivate(systemCapabilityType, listener, true, false);
512514
}
513515

514516
/**
@@ -526,7 +528,7 @@ public boolean removeOnSystemCapabilityListener(final SystemCapabilityType syste
526528
&& onSystemCapabilityListeners.get(systemCapabilityType) != null) {
527529
success = onSystemCapabilityListeners.get(systemCapabilityType).remove(listener);
528530
// If the last listener for the supplied capability type is removed, unsubscribe from the capability type
529-
if (success && onSystemCapabilityListeners.get(systemCapabilityType).isEmpty() && isSubscribedToSystemCapability(systemCapabilityType)) {
531+
if (success && onSystemCapabilityListeners.get(systemCapabilityType).isEmpty() && isSubscribedToSystemCapability(systemCapabilityType) && systemCapabilityType != SystemCapabilityType.DISPLAYS) {
530532
retrieveCapability(systemCapabilityType, null, false);
531533
}
532534
}
@@ -539,7 +541,7 @@ public boolean removeOnSystemCapabilityListener(final SystemCapabilityType syste
539541
* @param subscribe flag to subscribe to updates of the supplied capability type. True means subscribe; false means cancel subscription; null means don't change current subscription status.
540542
*/
541543
private void retrieveCapability(final SystemCapabilityType systemCapabilityType, final OnSystemCapabilityListener scListener, final Boolean subscribe) {
542-
if (!systemCapabilityType.isQueryable()) {
544+
if (!systemCapabilityType.isQueryable() || systemCapabilityType == SystemCapabilityType.DISPLAYS) {
543545
String message = "This systemCapabilityType cannot be queried for";
544546
DebugTool.logError(message);
545547
if (scListener != null) {
@@ -549,18 +551,35 @@ private void retrieveCapability(final SystemCapabilityType systemCapabilityType,
549551
}
550552
final GetSystemCapability request = new GetSystemCapability();
551553
request.setSystemCapabilityType(systemCapabilityType);
552-
request.setSubscribe((subscribe != null) ? subscribe : isSubscribedToSystemCapability(systemCapabilityType)); // If subscribe is null, then don't change the current subscription status
554+
555+
/*
556+
The subscription flag in the request should be set based on multiple variables:
557+
- if subscribe is null (no change), shouldSubscribe = current subscription status, or false if the HU does not support subscriptions
558+
- if subscribe is false, then shouldSubscribe = false
559+
- if subscribe is true and the HU supports subscriptions, then shouldSubscribe = true
560+
*/
561+
boolean shouldSubscribe = (subscribe != null) ? subscribe : isSubscribedToSystemCapability(systemCapabilityType);
562+
final boolean willSubscribe = shouldSubscribe && supportsSubscriptions();
563+
request.setSubscribe(willSubscribe);
553564
request.setOnRPCResponseListener(new OnRPCResponseListener() {
554565
@Override
555566
public void onResponse(int correlationId, RPCResponse response) {
556567
if (response.getSuccess()) {
557568
Object retrievedCapability = ((GetSystemCapabilityResponse) response).getSystemCapability().getCapabilityForType(systemCapabilityType);
558-
cachedSystemCapabilities.put(systemCapabilityType, retrievedCapability);
569+
setCapability(systemCapabilityType, retrievedCapability);
570+
// If the listener is not included in the onSystemCapabilityListeners map, then notify it
571+
// This will be triggered if we are just getting capability without adding a listener to the map
559572
if (scListener != null) {
560-
scListener.onCapabilityRetrieved(retrievedCapability);
573+
synchronized (LISTENER_LOCK) {
574+
CopyOnWriteArrayList<OnSystemCapabilityListener> notifiedListeners = onSystemCapabilityListeners.get(systemCapabilityType);
575+
boolean listenerAlreadyNotified = (notifiedListeners != null) && notifiedListeners.contains(scListener);
576+
if (!listenerAlreadyNotified) {
577+
scListener.onCapabilityRetrieved(retrievedCapability);
578+
}
579+
}
561580
}
562581
if (supportsSubscriptions()) {
563-
systemCapabilitiesSubscriptionStatus.put(systemCapabilityType, subscribe);
582+
systemCapabilitiesSubscriptionStatus.put(systemCapabilityType, willSubscribe);
564583
}
565584
} else {
566585
if (scListener != null) {

0 commit comments

Comments
 (0)