Merge pull request #2107 from smartdevicelink/bugfix/issue-1781-good-text-failing

Bugfix/issue 1781 good text failing

5 files changed, 192 insertions, 21 deletions

diff --git a/SmartDeviceLink/private/SDLTextAndGraphicManager.m b/SmartDeviceLink/private/SDLTextAndGraphicManager.m
index 34c4dcf54..23a7a3df9 100644
--- a/SmartDeviceLink/private/SDLTextAndGraphicManager.m
+++ b/SmartDeviceLink/private/SDLTextAndGraphicManager.m
@@ -157,7 +157,7 @@ NS_ASSUME_NONNULL_BEGIN
         }
     } else if (self.isDirty) {
         self.isDirty = NO;
-        [self sdl_updateAndCancelPreviousOperations:YES completionHandler:handler];
+        [self sdl_createOperationWithCompletionHandler:handler];
     } else {
         if (handler != nil) {
             handler([NSError sdl_textAndGraphicManager_nothingToUpdate]);
@@ -165,12 +165,8 @@ NS_ASSUME_NONNULL_BEGIN
     }
 }
 
-- (void)sdl_updateAndCancelPreviousOperations:(BOOL)supersedePreviousOperations completionHandler:(nullable SDLTextAndGraphicUpdateCompletionHandler)handler {
+- (void)sdl_createOperationWithCompletionHandler:(nullable SDLTextAndGraphicUpdateCompletionHandler)handler {
     SDLLogD(@"Updating text and graphics");
-    if (self.transactionQueue.operationCount > 0 && supersedePreviousOperations) {
-        SDLLogV(@"Transactions already exist, cancelling them");
-        [self.transactionQueue cancelAllOperations];
-    }
 
     __weak typeof(self) weakSelf = self;
     SDLTextAndGraphicUpdateOperation *updateOperation = [[SDLTextAndGraphicUpdateOperation alloc] initWithConnectionManager:self.connectionManager fileManager:self.fileManager currentCapabilities:self.windowCapability currentScreenData:self.currentScreenData newState:[self currentState] currentScreenDataUpdatedHandler:^(SDLTextAndGraphicState *_Nullable newScreenData, NSError *_Nullable error) {
@@ -182,7 +178,10 @@ NS_ASSUME_NONNULL_BEGIN
         } else if (error != nil) {
             // Invalidate data that's different from our current screen data if a Show or SetDisplayLayout fails. This will prevent subsequent `Show`s from failing if the request failed due to the developer setting invalid data or subsequent `SetDisplayLayout`s from failing if the template is not supported on the module. 
             [strongSelf sdl_resetFieldsToCurrentScreenData];
-            [strongSelf sdl_updatePendingOperationsWithNewScreenData:strongSelf.currentScreenData];
+            SDLTextAndGraphicState *errorState = error.userInfo[SDLTextAndGraphicFailedScreenStateErrorKey];
+            if (errorState != nil) {
+                [strongSelf sdl_updatePendingOperationsWithFailedScreenState:errorState];
+            }
         }
     } updateCompletionHandler:handler];
 
@@ -204,6 +203,15 @@ NS_ASSUME_NONNULL_BEGIN
     }
 }
 
+- (void)sdl_updatePendingOperationsWithFailedScreenState:(SDLTextAndGraphicState *)errorState {
+    for (NSOperation *operation in self.transactionQueue.operations) {
+        if (operation.isExecuting) { continue; }
+        SDLTextAndGraphicUpdateOperation *updateOp = (SDLTextAndGraphicUpdateOperation *)operation;
+
+        [updateOp updateTargetStateWithErrorState:errorState];
+    }
+}
+
 - (void)sdl_resetFieldsToCurrentScreenData {
     _textField1 = _currentScreenData.textField1;
     _textField2 = _currentScreenData.textField2;
@@ -394,7 +402,7 @@ NS_ASSUME_NONNULL_BEGIN
     // Auto-send an updated show
     if ([self sdl_hasData]) {
         // TODO: HAX: Capability updates cannot supersede earlier updates because of the case where a developer batched a `changeLayout` call w/ T&G changes on <6.0 systems could cause this to come in before the operation completes. That would cause the operation to report a "failure" (because it was superseded by this call) when in fact the operation didn't fail at all and is just being adjusted. Even though iOS is able to tell the developer that it was superseded, Java Suite cannot, and therefore we are matching functionality with their library.
-        [self sdl_updateAndCancelPreviousOperations:NO completionHandler:nil];
+        [self sdl_createOperationWithCompletionHandler:nil];
     }
 }
 
diff --git a/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.h b/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.h
index d3f88b667..e06306937 100644
--- a/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.h
+++ b/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.h
@@ -21,6 +21,8 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+extern NSString *const SDLTextAndGraphicFailedScreenStateErrorKey;
+
 typedef void(^SDLTextAndGraphicUpdateCompletionHandler)(NSError *__nullable error);
 typedef void(^CurrentDataUpdatedHandler)(SDLTextAndGraphicState *__nullable newScreenData, NSError *__nullable error);
 
@@ -38,6 +40,10 @@ typedef void(^CurrentDataUpdatedHandler)(SDLTextAndGraphicState *__nullable newS
 /// @param updateCompletionHandler The handler potentially passed by the developer to be called when the update finishes
 - (instancetype)initWithConnectionManager:(id<SDLConnectionManagerType>)connectionManager fileManager:(SDLFileManager *)fileManager currentCapabilities:(SDLWindowCapability *)currentCapabilities currentScreenData:(SDLTextAndGraphicState *)currentData newState:(SDLTextAndGraphicState *)newState currentScreenDataUpdatedHandler:(CurrentDataUpdatedHandler)currentDataUpdatedHandler updateCompletionHandler:(nullable SDLTextAndGraphicUpdateCompletionHandler)updateCompletionHandler;
 
+/// Changes updated state to remove failed updates from previous update operations. This will find and revert those failed updates back to current screen data so that we don't duplicate the failure.
+/// @param errorState A updated state that failed in a previous operation that will be used to filter out erroneous data
+- (void)updateTargetStateWithErrorState:(SDLTextAndGraphicState *)errorState;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.m b/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.m
index cd47531e2..246e1a014 100644
--- a/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.m
+++ b/SmartDeviceLink/private/SDLTextAndGraphicUpdateOperation.m
@@ -26,6 +26,8 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+NSString *const SDLTextAndGraphicFailedScreenStateErrorKey = @"failedScreenState";
+
 @interface SDLTextAndGraphicUpdateOperation ()
 
 @property (weak, nonatomic) id<SDLConnectionManagerType> connectionManager;
@@ -97,6 +99,23 @@ NS_ASSUME_NONNULL_BEGIN
     }
 }
 
+- (void)updateTargetStateWithErrorState:(SDLTextAndGraphicState *)errorState {
+    self.updatedState.textField1 = [errorState.textField1 isEqualToString:self.updatedState.textField1] ? self.currentScreenData.textField1 : self.updatedState.textField1;
+    self.updatedState.textField2 = [errorState.textField2 isEqualToString:self.updatedState.textField2] ? self.currentScreenData.textField2 : self.updatedState.textField2;
+    self.updatedState.textField3 = [errorState.textField3 isEqualToString:self.updatedState.textField3] ? self.currentScreenData.textField3 : self.updatedState.textField3;
+    self.updatedState.textField4 = [errorState.textField4 isEqualToString:self.updatedState.textField4] ? self.currentScreenData.textField4 : self.updatedState.textField4;
+    self.updatedState.mediaTrackTextField = [errorState.mediaTrackTextField isEqualToString:self.updatedState.mediaTrackTextField] ? self.currentScreenData.mediaTrackTextField : self.updatedState.mediaTrackTextField;
+    self.updatedState.title = [errorState.title isEqualToString:self.updatedState.title] ? self.currentScreenData.title : self.updatedState.title;
+    self.updatedState.primaryGraphic = [errorState.primaryGraphic isEqual:self.updatedState.primaryGraphic] ? self.currentScreenData.primaryGraphic : self.updatedState.primaryGraphic;
+    self.updatedState.secondaryGraphic = [errorState.secondaryGraphic isEqual:self.updatedState.secondaryGraphic] ? self.currentScreenData.secondaryGraphic : self.updatedState.secondaryGraphic;
+    self.updatedState.alignment = [errorState.alignment isEqualToEnum:self.updatedState.alignment] ? self.currentScreenData.alignment : self.updatedState.alignment;
+    self.updatedState.textField1Type = [errorState.textField1Type isEqualToEnum:self.updatedState.textField1Type] ? self.currentScreenData.textField1Type : self.updatedState.textField1Type;
+    self.updatedState.textField2Type = [errorState.textField2Type isEqualToEnum:self.updatedState.textField2Type] ? self.currentScreenData.textField2Type : self.updatedState.textField2Type;
+    self.updatedState.textField3Type = [errorState.textField3Type isEqualToEnum:self.updatedState.textField3Type] ? self.currentScreenData.textField3Type : self.updatedState.textField3Type;
+    self.updatedState.textField4Type = [errorState.textField4Type isEqualToEnum:self.updatedState.textField4Type] ? self.currentScreenData.textField4Type : self.updatedState.textField4Type;
+    self.updatedState.templateConfig = [errorState.templateConfig isEqual:self.updatedState.templateConfig] ? self.currentScreenData.templateConfig : self.updatedState.templateConfig;
+}
+
 #pragma mark - Send Show / Set Display Layout
 
 - (void)sdl_updateGraphicsAndShow:(SDLShow *)show {
@@ -158,8 +177,12 @@ NS_ASSUME_NONNULL_BEGIN
             SDLLogD(@"Text and Graphic Show completed successfully");
             [strongSelf sdl_updateCurrentScreenDataFromShow:request];
         } else {
-            SDLLogD(@"Text and Graphic Show failed");
-            self.currentDataUpdatedHandler(nil, error);
+            SDLLogE(@"Text and Graphic Show failed: %@", error);
+            NSError *updateError = [NSError errorWithDomain:error.domain code:error.code userInfo:@{
+                NSUnderlyingErrorKey: error.userInfo,
+                SDLTextAndGraphicFailedScreenStateErrorKey: self.updatedState
+            }];
+            self.currentDataUpdatedHandler(nil, updateError);
         }
 
         handler(error);
diff --git a/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicManagerSpec.m b/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicManagerSpec.m
index ea230248c..29e9a775f 100644
--- a/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicManagerSpec.m
+++ b/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicManagerSpec.m
@@ -140,19 +140,23 @@ describe(@"text and graphic manager", ^{
             });
         });
 
-        // when previous updates have bene cancelled
-        context(@"when previous updates have bene cancelled", ^{
+        // without batching
+        context(@"without batching", ^{
             beforeEach(^{
-                testManager.textField1 = @"Hello";
-
-                // This should cancel the first operation
-                testManager.textField2 = @"Goodbye";
+                testManager.batchUpdates = NO;
+                testManager.textField1 = @"test1";
+                testManager.textField2 = @"test2";
+                testManager.textField3 = @"test3";
+                testManager.textField4 = @"test4";
             });
 
-            it(@"should properly queue the new update", ^{
+            it(@"should create individual operations and not be cancelled", ^{
                 expect(testManager.transactionQueue.isSuspended).to(beTrue());
-                expect(testManager.transactionQueue.operationCount).to(equal(2));
-                expect(testManager.transactionQueue.operations[0].cancelled).to(beTrue());
+                expect(testManager.transactionQueue.operationCount).to(equal(4));
+                expect(testManager.transactionQueue.operations[0].cancelled).to(beFalse());
+                expect(testManager.transactionQueue.operations[1].cancelled).to(beFalse());
+                expect(testManager.transactionQueue.operations[2].cancelled).to(beFalse());
+                expect(testManager.transactionQueue.operations[3].cancelled).to(beFalse());
             });
         });
 
@@ -450,6 +454,7 @@ describe(@"text and graphic manager", ^{
     describe(@"when the operation updates the current screen data", ^{
         __block SDLTextAndGraphicUpdateOperation *testOperation = nil;
         __block SDLTextAndGraphicUpdateOperation *testOperation2 = nil;
+        __block SDLTextAndGraphicUpdateOperation *testOperation3 = nil;
 
         beforeEach(^{
             testManager.textField1 = @"test";
@@ -475,11 +480,26 @@ describe(@"text and graphic manager", ^{
             beforeEach(^{
                 testManager.currentScreenData = [[SDLTextAndGraphicState alloc] init];
                 testManager.currentScreenData.textField1 = @"Test1";
-                testOperation.currentDataUpdatedHandler(nil, [NSError errorWithDomain:@"any" code:1 userInfo:nil]);
+
+                // Create a "bad data" text field 1, then set it in the manager, which should create an operation (op 2)
+                SDLTextAndGraphicState *errorState = [[SDLTextAndGraphicState alloc] init];
+                errorState.textField1 = @"Bad Data";
+                testManager.textField1 = errorState.textField1;
+                
+                // Create a "good data text field 4, which should create a second operation (op 3)
+                testManager.textField4 = @"Good Data";
+                testOperation3 = testManager.transactionQueue.operations[3];
+
+                // Simulate a failure of the first operation
+                NSDictionary *userInfo = @{
+                    SDLTextAndGraphicFailedScreenStateErrorKey: errorState
+                };
+                testOperation.currentDataUpdatedHandler(nil, [NSError errorWithDomain:@"any" code:1 userInfo:userInfo]);
             });
 
-            it(@"should reset the manager's data", ^{
+            it(@"should reset the manager's data and update other operations updated state", ^{
                 expect(testManager.textField1).to(equal(testManager.currentScreenData.textField1));
+                expect(testOperation3.updatedState.textField1).to(equal(testManager.currentScreenData.textField1));
             });
         });
     });
diff --git a/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicUpdateOperationSpec.m b/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicUpdateOperationSpec.m
index b03252c25..a3f780ab0 100644
--- a/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicUpdateOperationSpec.m
+++ b/SmartDeviceLinkTests/DevAPISpecs/SDLTextAndGraphicUpdateOperationSpec.m
@@ -797,6 +797,116 @@ describe(@"the text and graphic operation", ^{
         });
     });
 
+    // updating with error state
+    describe(@"updating with error state", ^{
+        beforeEach(^{
+            updatedState = [[SDLTextAndGraphicState alloc] init];
+            updatedState.textField1 = field1String;
+            updatedState.textField2 = field2String;
+            updatedState.textField3 = field3String;
+            updatedState.textField4 = field4String;
+            updatedState.mediaTrackTextField = mediaTrackString;
+            updatedState.title = titleString;
+            updatedState.primaryGraphic = testArtwork;
+            updatedState.secondaryGraphic = testArtwork2;
+            updatedState.alignment = SDLTextAlignmentLeft;
+            updatedState.textField1Type = SDLMetadataTypeMediaTitle;
+            updatedState.textField2Type = SDLMetadataTypeMediaArtist;
+            updatedState.textField3Type = SDLMetadataTypeMediaAlbum;
+            updatedState.textField4Type = SDLMetadataTypeMediaYear;
+
+            emptyCurrentData = [[SDLTextAndGraphicState alloc] init];
+
+            testOp = [[SDLTextAndGraphicUpdateOperation alloc] initWithConnectionManager:testConnectionManager fileManager:mockFileManager currentCapabilities:windowCapability currentScreenData:emptyCurrentData newState:updatedState currentScreenDataUpdatedHandler:^(SDLTextAndGraphicState * _Nullable newScreenData, NSError * _Nullable error) {} updateCompletionHandler:nil];
+            [testOp start];
+        });
+
+        it(@"should reset to current screen data for equivalent properties in updated state and error state", ^{
+            // Create an error state that matches the updated state, which should reset the updated state
+            SDLTextAndGraphicState *errorState = [updatedState copy];
+            errorState.primaryGraphic = testArtwork;
+            errorState.secondaryGraphic = testArtwork2;
+
+            [testOp updateTargetStateWithErrorState:errorState];
+
+            expect(updatedState.textField1).to(beNil());
+            expect(updatedState.textField2).to(beNil());
+            expect(updatedState.textField3).to(beNil());
+            expect(updatedState.textField4).to(beNil());
+            expect(updatedState.mediaTrackTextField).to(beNil());
+            expect(updatedState.title).to(beNil());
+            expect(updatedState.primaryGraphic).to(beNil());
+            expect(updatedState.secondaryGraphic).to(beNil());
+            expect(updatedState.textField1Type).to(beNil());
+            expect(updatedState.textField2Type).to(beNil());
+            expect(updatedState.textField3Type).to(beNil());
+            expect(updatedState.textField4Type).to(beNil());
+        });
+
+        it(@"should not reset to current screen data for non equivalent properties in updated state and error state", ^{
+            // Save an original of the updatedState for confirming no changes later
+            SDLTextAndGraphicState *originalState = [updatedState copy];
+            originalState.primaryGraphic = testArtwork;
+            originalState.secondaryGraphic = testArtwork2;
+
+            // Create an error state that does not match the updated state, which should not reset the updated state
+            SDLTextAndGraphicState *errorState = [[SDLTextAndGraphicState alloc] init];
+            errorState.textField1 = @"Error Text";
+            errorState.textField2 = @"Error Text";
+            errorState.textField3 = @"Error Text";
+            errorState.textField4 = @"Error Text";
+            errorState.mediaTrackTextField = @"Error Text";
+            errorState.title = @"Error Text";
+            errorState.primaryGraphic = testArtwork2;
+            errorState.secondaryGraphic = testArtwork;
+            errorState.alignment = SDLTextAlignmentRight;
+            errorState.textField1Type = SDLMetadataTypeMediaYear;
+            errorState.textField2Type = SDLMetadataTypeMediaAlbum;
+            errorState.textField3Type = SDLMetadataTypeMediaArtist;
+            errorState.textField4Type = SDLMetadataTypeMediaTitle;
+
+            [testOp updateTargetStateWithErrorState:errorState];
+
+            expect(updatedState.textField1).to(equal(originalState.textField1));
+            expect(updatedState.textField2).to(equal(originalState.textField2));
+            expect(updatedState.textField3).to(equal(originalState.textField3));
+            expect(updatedState.textField4).to(equal(originalState.textField4));
+            expect(updatedState.mediaTrackTextField).to(equal(originalState.mediaTrackTextField));
+            expect(updatedState.title).to(equal(originalState.title));
+            expect(updatedState.primaryGraphic).to(equal(originalState.primaryGraphic));
+            expect(updatedState.secondaryGraphic).to(equal(originalState.secondaryGraphic));
+            expect(updatedState.textField1Type).to(equal(originalState.textField1Type));
+            expect(updatedState.textField2Type).to(equal(originalState.textField2Type));
+            expect(updatedState.textField3Type).to(equal(originalState.textField3Type));
+            expect(updatedState.textField4Type).to(equal(originalState.textField4Type));
+        });
+
+        it(@"should not reset to current screen data for nil error state", ^{
+            // Save an original of the updatedState for confirming no changes later
+            SDLTextAndGraphicState *originalState = [updatedState copy];
+            originalState.primaryGraphic = testArtwork;
+            originalState.secondaryGraphic = testArtwork2;
+
+            // Create an empty error state
+            SDLTextAndGraphicState *errorState = [[SDLTextAndGraphicState alloc] init];
+
+            [testOp updateTargetStateWithErrorState:errorState];
+
+            expect(updatedState.textField1).to(equal(originalState.textField1));
+            expect(updatedState.textField2).to(equal(originalState.textField2));
+            expect(updatedState.textField3).to(equal(originalState.textField3));
+            expect(updatedState.textField4).to(equal(originalState.textField4));
+            expect(updatedState.mediaTrackTextField).to(equal(originalState.mediaTrackTextField));
+            expect(updatedState.title).to(equal(originalState.title));
+            expect(updatedState.primaryGraphic).to(equal(originalState.primaryGraphic));
+            expect(updatedState.secondaryGraphic).to(equal(originalState.secondaryGraphic));
+            expect(updatedState.textField1Type).to(equal(originalState.textField1Type));
+            expect(updatedState.textField2Type).to(equal(originalState.textField2Type));
+            expect(updatedState.textField3Type).to(equal(originalState.textField3Type));
+            expect(updatedState.textField4Type).to(equal(originalState.textField4Type));
+        });
+    });
+
     // updating image fields
     describe(@"updating image fields", ^{
         beforeEach(^{
@@ -933,6 +1043,8 @@ describe(@"the text and graphic operation", ^{
                         // Then it should return a failure and finish
                         expect(receivedState).to(beNil());
                         expect(receivedError).toNot(beNil());
+                        expect(receivedError.userInfo[NSUnderlyingErrorKey]).toNot(beNil());
+                        expect(receivedError.userInfo[SDLTextAndGraphicFailedScreenStateErrorKey]).to(equal(updatedState));
                         expect(completionError).toNot(beNil());
 
                         expect(testOp.isFinished).to(beTrue());
@@ -1295,6 +1407,8 @@ describe(@"the text and graphic operation", ^{
                         [testConnectionManager respondToLastRequestWithResponse:failShowResponse];
                         expect(receivedState).to(beNil());
                         expect(receivedError).toNot(beNil());
+                        expect(receivedError.userInfo[NSUnderlyingErrorKey]).toNot(beNil());
+                        expect(receivedError.userInfo[SDLTextAndGraphicFailedScreenStateErrorKey]).to(equal(updatedState));
 
                         expect(testOp.isFinished).to(beTrue());
                     });