diff --git a/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m b/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m index 03ac8fa33..2fd63ef13 100644 --- a/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m +++ b/Quicksilver/Code-QuickStepInterface/QSInterfaceController.m @@ -652,15 +652,22 @@ - (void)executeCommand:(id)sender cont:(BOOL)cont encapsulate:(BOOL)encapsulate NSInteger argumentCount = [action argumentCount]; if (argumentCount == 2) { - // for the case where we require an argument, update the indirects *then* perform the execute - [self updateIndirectObjectsWithCompletion:^{ - [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; - }]; - } else { - // otherwise, just perform the execute straight away - [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; + BOOL indirectIsRequired = ![action indirectOptional]; + BOOL indirectIsInvalid = ![iSelector objectValue]; + BOOL indirectIsTextProxy = [[[iSelector objectValue] primaryType] isEqual:QSTextProxyType]; + if (indirectIsRequired && (indirectIsInvalid || indirectIsTextProxy)) { + // Indirect is needed but not yet available — the async update from + // searchObjectChanged: may not have completed yet. Explicitly fetch + // indirects and execute once they're ready. + [self updateIndirectObjectsWithCompletion:^{ + [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; + }]; + return; + } } - return; + + // Indirect not needed, or user already provided one. Execute immediately. + [self performExecuteAction:action argumentCount:argumentCount cont:cont encapsulate:encapsulate]; } - (void)performExecuteAction:(QSAction *)action argumentCount:(NSInteger)argumentCount cont:(BOOL)cont encapsulate:(BOOL)encapsulate { diff --git a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m index 363d96d93..bf46ed2c6 100644 --- a/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m +++ b/Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m @@ -7,6 +7,7 @@ #import "QSInterfaceController.h" #import "QSController.h" #import "QSObject.h" +#import "QSObject_FileHandling.h" #import "QSSearchObjectView.h" @interface QSInterfaceControllerTests : XCTestCase @@ -14,6 +15,16 @@ @interface QSInterfaceControllerTests : XCTestCase @implementation QSInterfaceControllerTests +- (void)setUp { + [super setUp]; + // Reset action and indirect panes between tests to prevent pollution. + // Do NOT clear dSelector — that destroys its result/source arrays which + // are populated from the catalog and needed for search-based tests. + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + [i clearObjectView:[i aSelector]]; + [i clearObjectView:[i iSelector]]; +} + /// Wait for pending QSGCDAsync + QSGCDMainAsync operations to complete - (void)waitForAsyncUpdates { XCTestExpectation *exp = [self expectationWithDescription:@"async updates"]; @@ -224,19 +235,16 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { NSInteger originalArgCount = [action argumentCount]; id originalIndirectOptional = [[action actionDict] objectForKey:kActionIndirectOptional]; - // Force the action to appear as a 2-arg action with optional indirect. + // Force the action to appear as a 2-arg action with REQUIRED indirect. // argumentCount == 2 makes executeCommand: take the two-arg code path. - // indirectOptional ensures performExecuteAction: doesn't bail at the - // indirect validity check (since iSelector will be nil). + // Removing indirectOptional (defaults to NO) makes the indirect required, + // which triggers the deferred completion block path when iSelector is nil. [action setArgumentCount:2]; - [[action actionDict] setObject:@YES forKey:kActionIndirectOptional]; + [[action actionDict] removeObjectForKey:kActionIndirectOptional]; // Track whether the notification fires synchronously (before executeCommand: returns) // vs asynchronously (in the completion block, after executeCommand: returns). - // Using queue:nil so the observer fires inline on the posting thread — if the - // notification is posted during executeCommand:, the block runs before it returns. __block BOOL notificationReceived = NO; - XCTestExpectation *exp = [self expectationWithDescription:@"Command eventually executed"]; id observer = [[NSNotificationCenter defaultCenter] addObserverForName:QSCommandExecutedNotification @@ -244,20 +252,20 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { queue:nil usingBlock:^(NSNotification *notif) { notificationReceived = YES; - [exp fulfill]; }]; [i executeCommand:self]; // Check IMMEDIATELY after executeCommand: returns, before spinning the run loop. - // With the fix: execution is deferred (async via completion block) → NO notification yet. - // Without the fix: execution ran synchronously inside executeCommand: → notification already fired. + // With the fix: indirect is required but missing (iSelector is nil), so execution + // is deferred to the updateIndirectObjectsWithCompletion: callback → no notification yet. + // Without the fix: execution ran synchronously inside executeCommand:. XCTAssertFalse(notificationReceived, - @"For 2-arg actions, execution must be deferred to the completion block. " + @"For 2-arg actions with required indirect, execution must be deferred to the completion block. " @"Synchronous execution indicates the race condition fix is missing."); - // Now wait for the async completion to verify the action DOES eventually fire. - [self waitForExpectationsWithTimeout:5.0 handler:nil]; + // Allow the async completion to drain so it doesn't interfere with other tests + [self waitForAsyncUpdates]; [[NSNotificationCenter defaultCenter] removeObserver:observer]; @@ -270,4 +278,110 @@ - (void)testTwoArgExecutionDeferredToCompletionBlock { } } +/** + * Regression test for the race condition introduced by making updateIndirectObjects + * fully async (commit e10140de). When the user presses Return and the action requires + * an indirect (argumentCount == 2), the async indirect update may not have completed. + * executeCommand: should fetch indirects via completion block rather than beeping. + * + * Uses setObjectValue: directly (not keyDown:) to avoid catalog search dependencies. + */ +- (void)testExecuteCommandWaitsForIndirectsWhenMissing { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Set a file object directly in dSelector + QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; + [[i dSelector] setObjectValue:fileObj]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Populate actions directly (don't rely on timer — it may have been + // invalidated by a previous test calling executeCommand:) + [i updateActionsNow]; + XCTAssertNotNil([[i aSelector] objectValue]); + + // Set a 2-arg action (Open With). This triggers searchObjectChanged: which + // calls updateIndirectObjects asynchronously (fire-and-forget). + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // At this point, iSelector is likely empty because the async indirect update + // hasn't completed yet. This is the exact race condition we're testing. + + // Listen for QSCommandExecutedNotification — posted after the action runs. + // With the fix: executeCommand: detects the missing indirect, defers to the + // completion block of updateIndirectObjectsWithCompletion:, which fetches + // indirects on a background thread then executes on main. The notification + // fires once execution completes, giving us a deterministic wait point. + // Without the fix: executeCommand: would synchronously beep and focus iSelector. + XCTestExpectation *commandExecuted = [self expectationWithDescription:@"command executed via deferred completion"]; + id observer = [[NSNotificationCenter defaultCenter] + addObserverForName:QSCommandExecutedNotification + object:nil + queue:nil + usingBlock:^(NSNotification *notif) { + [commandExecuted fulfill]; + }]; + + [i executeCommand:self]; + + // Wait for the command to actually execute (deterministic — no GCD timing hacks) + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + [[NSNotificationCenter defaultCenter] removeObserver:observer]; + + // The first responder should NOT be iSelector — that would mean the bug occurred + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertNotEqual(firstResponder, [i iSelector], + @"Focus should not have moved to iSelector — the indirect objects race condition was not handled"); +} + +/** + * Regression test: executeCommand: must NOT unconditionally re-fetch indirect objects, + * as that would overwrite user-entered content in the 3rd pane. + * + * Simulates: select file → choose "Open With" → select an app in 3rd pane → Return + * The user's selection in the 3rd pane must be preserved, not overwritten. + */ +- (void)testExecuteCommandPreservesUserSelectedIndirect { + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; + XCTAssertNotNil(i); + + // Set a file object in dSelector + QSObject *fileObj = [QSObject fileObjectWithPath:@"/System"]; + [[i dSelector] setObjectValue:fileObj]; + XCTAssertNotNil([[i dSelector] objectValue]); + + // Populate actions directly (don't rely on timer state) + [i updateActionsNow]; + + // Set a 2-arg action + QSAction *openWithAction = [QSAction actionWithIdentifier:@"FileOpenWithAction"]; + XCTAssertNotNil(openWithAction, @"FileOpenWithAction must exist for this test"); + [[i aSelector] setObjectValue:openWithAction]; + + // Wait for the indirect objects to be fully populated + XCTestExpectation *indirectsReady = [self expectationWithDescription:@"indirects populated"]; + [i updateIndirectObjectsWithCompletion:^{ + [indirectsReady fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 handler:nil]; + + // Now simulate the user selecting a specific app in the 3rd pane + QSObject *userSelectedApp = [QSObject fileObjectWithPath:@"/Applications/Safari.app"]; + [[i iSelector] setObjectValue:userSelectedApp]; + XCTAssertNotNil([[i iSelector] objectValue], @"iSelector should have the user's selection"); + + // Call executeCommand: — this should use the user's selection, NOT re-fetch + [i executeCommand:self]; + + // Wait for any async operations to settle + [self waitForAsyncUpdates]; + + // executeCommand: should NOT have moved focus to iSelector + NSResponder *firstResponder = [[i window] firstResponder]; + XCTAssertNotEqual(firstResponder, [i iSelector], + @"executeCommand: should have used the user's existing indirect selection, not re-fetched"); +} + @end diff --git a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m index b1c929ca3..12d06127f 100644 --- a/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m +++ b/Quicksilver/Quicksilver Tests/Quicksilver_Tests.m @@ -39,7 +39,7 @@ - (void)testRightArrowIntoSynonym { [proxy setIdentifier:[NSString stringWithFormat:@"QSUserDefinedProxy:%@", name]]; [proxy setName:name]; [proxy setObject:targetID forMeta:@"target"]; - + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; [[i dSelector] selectObjectValue:proxy]; [[i dSelector] moveRight:self]; @@ -49,23 +49,23 @@ - (void)testRightArrowIntoSynonym { - (void)testCreateObjectFromRTFClipboard { // create an rtf string in the clipboard NSPasteboard *pboard = [NSPasteboard generalPasteboard]; - + NSAttributedString *rtfString = [[NSAttributedString alloc] initWithRTF:[@"{\\rtf1\\ansi\\ansicpg1252\\cocoartf1187\\cocoasubrtf340\n{\\fonttbl\\f0\\fnil\\fcharset0 Monaco;}\n{\\colortbl;\\red255\\green0\\blue0;}\n\\margl1440\\margr1440\\vieww10800\\viewh8400\\viewkind0\n\\pard\\tx720\\tx1440\\tx2160\\tx2880\\tx3600\\tx4320\\tx5040\\tx5760\\tx6480\\tx7200\\tx7920\\tx8640\\ql\\qnatural\\pardirnatural\n\n\\f0\\fs24 \\cf1 hello}" dataUsingEncoding:NSUTF8StringEncoding] documentAttributes:nil]; [pboard clearContents]; [pboard writeObjects:@[rtfString]]; - + // create a QSObject from the clipboard QSObject *obj = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([obj containsType:NSPasteboardTypeRTF] && [[obj primaryType] isEqualToString:NSPasteboardTypeRTF]); - + // now write this object to the pasteboard [pboard clearContents]; XCTAssertTrue([obj putOnPasteboard:pboard]); - + // read the object back from the pasteboard QSObject *obj2 = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([obj2 containsType:NSPasteboardTypeRTF] && [[obj2 primaryType] isEqualToString:NSPasteboardTypeRTF]); - + } // test to make sure when file objects are added to the clipboard, a string of their path is also copied @@ -76,12 +76,12 @@ - (void)testAddingFileObjectToPasteboard { XCTAssertEqualObjects([obj singleFilePath], path); XCTAssertTrue([obj putOnPasteboard:pboard] == YES); XCTAssertTrue([[pboard types] containsObject:NSPasteboardTypeFileURL]); - + // for file objects, we don't write the QSTextType to the pasteboard - that messes with drag/drop in some applications XCTAssertFalse([[pboard types] containsObject:QSTextType]); NSArray *a = [pboard propertyListForType:NSPasteboardTypeFileURL]; XCTAssertEqualObjects(a, @"file:///usr/bin/cd"); - + // try this for an imagined type that already has a string type set: obj = [QSObject fileObjectWithPath:path]; NSString *textString = @"My Important String"; @@ -109,13 +109,13 @@ - (void)testAddingFileObjectToPasteboardWithMultiplePaths { return [pbitem propertyListForType:NSPasteboardTypeFileURL]; }]; XCTAssertEqualObjects(a, fileArray); - + // now try re-creating the object from the pasteboard QSObject *newObj = [QSObject objectWithPasteboard:pboard]; XCTAssertTrue([newObj containsType:QSFilePathType]); XCTAssertEqualObjects([newObj arrayForType:QSFilePathType], filePathArray); } - + /** * In order to run these tests, you must select 'Quicksilver' as the scheme, as opposed to 'Quicksilver Distribution' @@ -126,7 +126,7 @@ - (void)testAddingFileObjectToPasteboardWithMultiplePaths { */ - (void)testClearingFirstPane { - + QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController]; // Test is sure to fail if we can't get the interface controller @@ -134,7 +134,7 @@ - (void)testClearingFirstPane // Assumes the current interface can collect QSCollectingSearchObjectView *dSelector = (QSCollectingSearchObjectView *)[i dSelector]; - + // Make a couple of file objects and add them to the dSelector NSArray *paths = @[@"/System", @"/Users"]; NSArray *objs = @[[QSObject fileObjectWithPath:paths[0]], [QSObject fileObjectWithPath:paths[1]]]; @@ -142,15 +142,15 @@ - (void)testClearingFirstPane [dSelector setObjectValue:o]; [dSelector collect:o]; } - + // Ensure the collected objects are the same as those added XCTAssertEqualObjects([[dSelector objectValue] splitObjects], objs, @"Collected objects aren't correct"); - + // Call some action on the objects (Get File URL known to be problematic QSAction *getFileURLAction = [QSAction actionWithIdentifier:@"FileGetURLAction"]; [[i aSelector] setObjectValue:getFileURLAction]; [i executeCommandThreaded]; - + // Ensure the returned files are what we expected (for this action) NSString *fileURLs = [[NSURL performSelector:@selector(fileURLWithPath:) onObjectsInArray:paths returnValues:YES] componentsJoinedByString:@"\n"]; QSObject *fileURLStringObject = [QSObject objectWithType:QSTextType value:fileURLs name:fileURLs]; @@ -166,16 +166,16 @@ - (void)testClearingSearchStringOnTrigger { NSEvent *typeAEvent = [NSEvent keyEventWithType:NSEventTypeKeyDown location:NSMakePoint(0, 0) modifierFlags:256 timestamp:15127.081604936 windowNumber:[[i window] windowNumber] context:nil characters:@"a" charactersIgnoringModifiers:@"a" isARepeat:NO keyCode:0]; // Simulate typing 'a' into the dSelector [[i dSelector] keyDown:typeAEvent]; - + XCTAssertEqualObjects(@"a", [[i dSelector] searchString], @"The search string typed into the dSelector is incorrect"); - + // Simulate opening a trigger like iTunes' 'Search Artists' (a.k.a. a search children action) QSAction *searchChildrenAction = [QSAction actionWithIdentifier:@"QSObjectSearchChildrenAction"]; [[i aSelector] setObjectValue:searchChildrenAction]; [i executeCommandThreaded]; - + [[i dSelector] keyDown:typeAEvent]; - + XCTAssertEqualObjects(@"a", [[i dSelector] searchString], @"The previous search string was not cleared when invoking a 'search children' command"); }