Skip to content

Fix up #3103 #3105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pjrobertson merged 3 commits into from
Mar 24, 2026
Merged

Fix up #3103 #3105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
600d19e
Fix up #3103
pjrobertson Mar 23, 2026
0fce836
Fix up tests
pjrobertson Mar 23, 2026
c2de0f1
Finally (hopefully) fix flaky tests
pjrobertson Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions Quicksilver/Code-QuickStepInterface/QSInterfaceController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
140 changes: 127 additions & 13 deletions Quicksilver/Quicksilver Tests/QSInterfaceControllerTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,24 @@
#import "QSInterfaceController.h"
#import "QSController.h"
#import "QSObject.h"
#import "QSObject_FileHandling.h"
#import "QSSearchObjectView.h"

@interface QSInterfaceControllerTests : XCTestCase
@end

@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"];
Expand Down Expand Up @@ -224,40 +235,37 @@ - (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
object:nil
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];

Expand All @@ -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
38 changes: 19 additions & 19 deletions Quicksilver/Quicksilver Tests/Quicksilver_Tests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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
Expand All @@ -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";
Expand Down Expand Up @@ -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'
Expand All @@ -126,31 +126,31 @@ - (void)testAddingFileObjectToPasteboardWithMultiplePaths {
*/
- (void)testClearingFirstPane
{

QSInterfaceController *i = [(QSController *)[NSApp delegate] interfaceController];

// Test is sure to fail if we can't get the interface controller
XCTAssertNotNil(i);

// 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]]];
for (QSObject *o in objs) {
[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];
Expand All @@ -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");
}

Expand Down
Loading