diff --git a/AppController.h b/AppController.h index 669bed8..a088cec 100755 --- a/AppController.h +++ b/AppController.h @@ -35,6 +35,7 @@ SRKeyCodeTransformer *srTransformer; BOOL isBezelDisplayed; BOOL isBezelPinned; + BOOL isMenuOpen; NSString *currentKeycodeCharacter; NSDateFormatter* dateFormat; diff --git a/AppController.m b/AppController.m index 3898ab0..5217c5e 100755 --- a/AppController.m +++ b/AppController.m @@ -53,6 +53,12 @@ - (BOOL)performKeyEquivalent:(NSEvent *)theEvent { @end +// Clipping items that -updateMenuContaining: has removed from the status menu. jcMenu is +// their only owner (see the -release right after -insertItem: below), so they would be +// deallocated at once - while AppKit may still be about to deliver a click for one of them. +// Keeping the previous generation alive makes such a sender safe to inspect. +static NSArray *sRetiredMenuClippingItems = nil; + @implementation AppController @@ -337,7 +343,7 @@ - (void)awakeFromNib selector:@selector(pollPB:) userInfo:nil repeats:YES]; - // Assign it to NSRunLoopCommonModes so that it will still poll while the menu is open. Using a simple NSTimer scheduledTimerWithTimeInterval: would result in polling that stops while the menu is active. In the past this was okay but with Universal Clipboard a new clipping an arrive while the user has the menu open. + // Assign it to NSRunLoopCommonModes so the timer keeps ticking while the menu is open. Using a simple NSTimer scheduledTimerWithTimeInterval: would suspend polling during menu tracking. Note that -pollPB: intentionally skips capture while the menu or search window is open (a new clipping inserted mid-selection would shift the indices and cause the wrong entry to be pasted); the tick still runs so capture resumes promptly, and menuDidClose: performs a catch-up poll. [[NSRunLoop currentRunLoop] addTimer:pollPBTimer forMode:NSRunLoopCommonModes]; // Finish up @@ -369,9 +375,19 @@ -(void)savePreferencesOnDict:(NSMutableDictionary *)saveDict -(void)menuWillOpen:(NSMenu *)menu { + // Freeze clipping capture while the menu is open so the item indices the user sees stay + // stable until they make a selection (see -pollPB:). + isMenuOpen = YES; + NSEvent *event = [NSApp currentEvent]; if([event modifierFlags] & NSEventModifierFlagOption) { [menu cancelTracking]; + // A menu cancelled from inside -menuWillOpen: never gets a -menuDidClose:, verified + // with a standalone AppKit program. Leaving the flag set would freeze -pollPB: + // for good: the option-click is the documented way to pause capture while copying a + // password, and the second option-click would re-enable the store but never resume + // capturing. The menu is not opening, so clear it here. + isMenuOpen = NO; bool disableStore = [self toggleMenuIconDisabled]; if (!disableStore) { @@ -391,7 +407,10 @@ -(void)menuWillOpen:(NSMenu *)menu -(void)menuDidClose:(NSMenu *)menu { - // Menu closed - no special handling needed now that we removed search box activation + // Resume clipping capture and immediately pick up anything that landed on the pasteboard + // while the menu was open (a selection's own paste is ignored via pbBlockCount in -pollPB:). + isMenuOpen = NO; + [self pollPB:nil]; } -(bool)toggleMenuIconDisabled @@ -899,28 +918,31 @@ - (void)pasteFromStack - (void)moveItemAtStackPositionToTopOfStack { if ( [flycutOperator stackPositionIsInBounds] ) { - [self pasteIndexAndUpdate: [flycutOperator stackPosition]]; + // stackPosition already IS a store position. It must never be run through the menu's + // search-box mapping, which is what the old -pasteIndexAndUpdate: did. + [self pasteStorePositionAndUpdate: [flycutOperator stackPosition]]; [self performSelector:@selector(hideApp) withObject:nil afterDelay:0.2]; } else { [self performSelector:@selector(hideApp) withObject:nil afterDelay:0.2]; } } -- (void)pasteIndexAndUpdate:(int) position { - // If there is an active search, we need to map the menu index to the stack position. - NSString* search = [searchBox stringValue]; - if ( nil != search && 0 != search.length ) - { - NSArray *mapping = [flycutOperator previousIndexes:[[NSUserDefaults standardUserDefaults] integerForKey:@"displayNum"] containing:search]; - position = [mapping[position] intValue]; - } +// Pastes the clipping at the given STORE position. Unlike the old -pasteIndexAndUpdate: +// this never reads the menu's search box: every caller resolves its own mapping while it +// still knows which list the position came from. Returns true when something actually +// reached the pasteboard, so the caller can skip faking cmd-V - otherwise the previous +// pasteboard contents (typically the last thing pasted) would be inserted again. +- (bool)pasteStorePositionAndUpdate:(int) position { + if ( position < 0 || position >= [flycutOperator jcListCount] ) + return false; NSString *content = [flycutOperator getPasteFromIndex: position]; - if ( nil != content ) - { - [self addClipToPasteboard:content]; - [self updateMenu]; - } + if ( nil == content ) + return false; + + [self addClipToPasteboard:content]; + [self updateMenu]; + return true; } - (void)metaKeysReleased @@ -932,6 +954,14 @@ - (void)metaKeysReleased } - (void)windowDidResignKey:(NSNotification *)notification { + if ( [notification object] == searchWindow ) { + // -hideApp does not close the search window properly, so isSearchWindowDisplayed + // would stay set - and -pollPB: reads that as "a selection is in progress" and + // stops capturing altogether, with nothing to show the user why. + [self hideSearchWindow]; + return; + } + [self hideApp]; } @@ -1066,6 +1096,17 @@ -(BOOL)control:(NSControl *)control textView:(NSTextView *)fieldEditor doCommand -(void)pollPB:(NSTimer *)timer { + // Do not capture new clippings while the user is picking one from the menu or the + // search window. Because this timer runs in NSRunLoopCommonModes it fires during menu + // tracking, and inserting a new clipping at the top of the store would make every row + // jump down by one while the user is aiming at it. Correctness no longer depends on + // this freeze - a selection is resolved by clipping identity, see + // -storePositionForMenuItem: - but the list should not move under the pointer. + // pbCount is left untouched so the change is still detected and captured once the menu + // or search window closes (menuDidClose: fires a catch-up poll). + if ( isMenuOpen || isSearchWindowDisplayed ) + return; + NSString *type = [jcPasteboard availableTypeFromArray:[NSArray arrayWithObject:NSPasteboardTypeString]]; if ( [pbCount intValue] != [jcPasteboard changeCount] && ![flycutOperator storeDisabled] ) { // Reload pbCount with the current changeCount @@ -1102,7 +1143,7 @@ -(void)pollPB:(NSTimer *)timer // background queue and the main thread (e.g. showing the bezel) causes crashes. dispatch_async(dispatch_get_main_queue(), ^{ if ( ! [pbCount isEqualTo:pbBlockCount] ) { - [flycutOperator addClipping:contents ofType:type fromApp:[currRunningApp localizedName] withAppBundleURL:currRunningApp.bundleURL.path target:self clippingAddedSelector:@selector(updateMenu)]; + [flycutOperator addClipping:contents ofType:type fromApp:[currRunningApp localizedName] withAppBundleURL:currRunningApp.bundleURL.path target:self clippingAddedSelector:@selector(clippingWasAdded)]; } }); } @@ -1320,6 +1361,11 @@ - (void) hideBezel -(void)hideApp { isBezelPinned = NO; + // Hiding the app tears down any open menu and the search window anyway, so clear both + // freeze flags here as a backstop. Neither is meant to outlive its surface, and a + // stale one silently stops -pollPB: from capturing anything at all. + isMenuOpen = NO; + isSearchWindowDisplayed = NO; [self hideBezel]; [NSApp hide:self]; } @@ -1339,6 +1385,13 @@ - (void)hitMainHotKey:(SGHotKey *)hotKey if ( [[NSUserDefaults standardUserDefaults] boolForKey:@"stickyBezel"] ) { isBezelPinned = YES; } + // The stack position is bezel state, so a new selection starts at the newest clipping. + // This used to happen as a side effect of -addClipping: resetting it, which meant a + // clipping arriving mid-selection moved the selection instead of the list moving under + // a selection that stays put. Note this is deliberately not in -showBezel:, which the + // favourites toggle also calls after -toggleToFromFavoritesStore has swapped in that + // store's own remembered position. + [flycutOperator setStackPositionToFirstItem]; [self showBezel]; } else { [self stackDown]; @@ -1504,7 +1557,16 @@ - (void)updateMenuContaining:(NSString*)search { dispatch_async(dispatch_get_main_queue(), ^{ [jcMenu setMenuChangedMessagesEnabled:NO]; - NSArray *returnedDisplayStrings = [flycutOperator previousDisplayStrings:[[NSUserDefaults standardUserDefaults] integerForKey:@"displayNum"] containing:search]; + int menuDisplayNum = (int)[[NSUserDefaults standardUserDefaults] integerForKey:@"displayNum"]; + BOOL filtering = ( nil != search && 0 != [search length] ); + + NSArray *returnedDisplayStrings = [flycutOperator previousDisplayStrings:menuDisplayNum containing:search]; + + // previousDisplayStrings: and previousIndexes: are both newest-first and element-wise + // aligned, so the store position of every row is known right here. Recording it now is + // what allows the selection to be resolved later without asking the menu where the + // clicked item sits and without re-reading the search box. + NSArray *returnedIndexes = filtering ? [flycutOperator previousIndexes:menuDisplayNum containing:search] : nil; NSArray *menuItems = [[[jcMenu itemArray] reverseObjectEnumerator] allObjects]; @@ -1515,18 +1577,55 @@ - (void)updateMenuContaining:(NSString*)search { int oldItems = [menuItems count]-jcMenuBaseItemsCount; int newItems = [clipStrings count]; DLog(@"list=%@, oldItems=%d, newItems=%d", returnedDisplayStrings, oldItems, newItems); - + + // Hold on to the outgoing items for one generation instead of letting jcMenu drop + // their last reference, so that a click AppKit delivers after this rebuild still + // has a valid sender. + NSMutableArray *retiringItems = [NSMutableArray array]; for ( int i = 0; i < oldItems; i++ ) + { + [retiringItems addObject:[jcMenu itemAtIndex:0]]; [jcMenu removeItemAtIndex:0]; - + } + [sRetiredMenuClippingItems release]; + sRetiredMenuClippingItems = [[NSArray alloc] initWithArray:retiringItems]; + for ( int i = 0; i < newItems; i++ ) { + // clipStrings is oldest-first (it was reversed above), everything coming from + // the store is newest-first. + int newestFirst = newItems - 1 - i; + int storePosition = newestFirst; + if ( filtering ) + { + if ( newestFirst >= (int)[returnedIndexes count] ) + continue; // The store changed while the menu was being built. + storePosition = [[returnedIndexes objectAtIndex:newestFirst] intValue]; + } + + // Remember what the row stands for rather than where it sits: the clipping object + // itself is the identity, the position is only a hint that + // -storePositionForMenuItem: re-validates. + // + // The display string deliberately is NOT the identity: it is the first line + // truncated to displayLen, and short strings are tagged pointers, so two + // different clippings whose first lines happen to match compare pointer-equal. + FlycutClipping *clipping = [flycutOperator clippingAtPosition:storePosition]; + if ( nil == clipping ) + continue; // The store changed while the menu was being built. + NSMenuItem *item; item = [[NSMenuItem alloc] initWithTitle:[clipStrings objectAtIndex:i] action:@selector(processMenuClippingSelection:) keyEquivalent:@""]; [item setTarget:self]; [item setEnabled:YES]; + // representedObject is a strong property, so the item keeps the clipping alive + // even if the store drops it in the meantime. + [item setRepresentedObject:[NSArray arrayWithObjects: + [NSNumber numberWithInt:storePosition], + clipping, + nil]]; [jcMenu insertItem:item atIndex:0]; // Way back in 0.2, failure to release the new item here was causing a quite atrocious memory leak. [item release]; @@ -1534,14 +1633,73 @@ - (void)updateMenuContaining:(NSString*)search { }); } +// Works out which clipping a menu item stands for. The item carries the clipping itself +// plus the store position it had when the menu was built; that position is only trusted +// while the store still agrees with it. Returns -1 when the clipping can no longer be +// found, in which case the caller must not paste anything. +- (int)storePositionForMenuItem:(NSMenuItem *)item +{ + id represented = [item representedObject]; + if ( ! [represented isKindOfClass:[NSArray class]] || 2 != [(NSArray *)represented count] ) + return -1; + + id hintNumber = [(NSArray *)represented objectAtIndex:0]; + id clipping = [(NSArray *)represented objectAtIndex:1]; + if ( ! [hintNumber isKindOfClass:[NSNumber class]] || ! [clipping isKindOfClass:[FlycutClipping class]] ) + return -1; + + int hint = [hintNumber intValue]; + int count = [flycutOperator jcListCount]; + + // The clipping object itself is the identity, so the position it happened to have when + // the menu was built is only a hint - checking it first keeps the common case O(1). + if ( hint >= 0 && hint < count && [flycutOperator clippingAtPosition:hint] == clipping ) + return hint; + + for ( int i = 0; i < count; i++ ) + if ( [flycutOperator clippingAtPosition:i] == clipping ) + return i; + + // Reloading the store from disk builds new clipping objects, so fall back to comparing + // contents. Two clippings with identical contents are interchangeable for pasting. + NSString *contents = [(FlycutClipping *)clipping contents]; + if ( nil != contents ) + for ( int i = 0; i < count; i++ ) + if ( [contents isEqualToString:[[flycutOperator clippingAtPosition:i] contents]] ) + return i; + + return -1; +} + -(IBAction)processMenuClippingSelection:(id)sender { - int index=[[sender menu] indexOfItem:sender]; - [self pasteIndexAndUpdate:index]; + // Never derive the selection from the item's position in the menu. -updateMenuContaining: + // runs on the main queue via dispatch_async and replaces every clipping item, which can + // happen between the click and the delivery of this action. The previous code did + // int index = [[sender menu] indexOfItem:sender]; + // and an item that had just been removed from the menu answers nil to -menu, so the + // message to nil returned 0 - silently pasting the newest clipping instead of the one the + // user clicked. Resolving by identity makes a rebuild in that window harmless. + int position = -1; + if ( [sender isKindOfClass:[NSMenuItem class]] ) + position = [self storePositionForMenuItem:(NSMenuItem *)sender]; + + bool pasted = ( position >= 0 ) ? [self pasteStorePositionAndUpdate:position] : false; + + if ( ! pasted ) { + // Log without any clipping contents, so a "nothing happened" report can be told + // apart from a wrong paste later on. + NSLog(@"processMenuClippingSelection: unresolved menu item (position=%d, item=%@) - nothing pasted", + position, + ( [sender isKindOfClass:[NSMenuItem class]] && nil != [(NSMenuItem *)sender menu] ) ? @"still in menu" : @"detached"); + NSBeep(); // Pasting some other clipping would be worse than pasting nothing at all. + } if ( [[NSUserDefaults standardUserDefaults] boolForKey:@"menuSelectionPastes"] ) { + // Hide either way, so focus returns to the application the user was working in. [self performSelector:@selector(hideApp) withObject:nil]; - [self performSelector:@selector(fakeCommandV) withObject:nil afterDelay:0.3]; + if ( pasted ) + [self performSelector:@selector(fakeCommandV) withObject:nil afterDelay:0.3]; } } @@ -1660,6 +1818,19 @@ - (NSString*)alertWithMessageText:(NSString*)message informationText:(NSString*) return buttons[result - NSAlertFirstButtonReturn]; } +// Called by -[FlycutOperator addClipping:...] once the store has been changed AND the stack +// position has been moved to follow the user's selection across that change. +- (void)clippingWasAdded +{ + [self updateMenu]; + + // The store redraws the bezel from inside its own insert, by way of -endUpdates, which is + // before the stack position has been settled. Left at that, the bezel would show the + // neighbour of the clipping a paste would actually deliver. Redraw now that it is settled. + if ( isBezelDisplayed ) + [self updateBezel]; +} + - (void)beginUpdates { needBezelUpdate = NO; needMenuUpdate = NO; @@ -1894,28 +2065,41 @@ - (void)updateSearchResults - (IBAction)searchWindowItemSelected:(id)sender { - NSInteger selectedRow = [searchWindowTableView selectedRow]; - if (selectedRow < 0) { - selectedRow = 0; // Default to first item if none selected + NSInteger selectedRow; + if (sender == searchWindowTableView) { + // Invoked by double-click: use the row that was actually clicked. The + // selection can change between the click and this action (for example + // updateSearchResults re-selects row 0 when the search field action + // fires), which pasted the newest entry instead of the clicked one. + selectedRow = [searchWindowTableView clickedRow]; + if (selectedRow < 0) { + return; // Double-click below the last row. + } + } else { + // Invoked by Enter in the search field. + selectedRow = [searchWindowTableView selectedRow]; + if (selectedRow < 0) { + selectedRow = 0; // Default to first item if none selected + } } - + if (selectedRow < [searchResults count]) { // Get the content and paste it like bezel does NSString* searchText = [searchWindowSearchField stringValue]; NSArray *mapping = nil; int position = (int)selectedRow; - + if (searchText && [searchText length] > 0) { mapping = [flycutOperator previousIndexes:[[NSUserDefaults standardUserDefaults] integerForKey:@"displayNum"] containing:searchText]; + if (selectedRow >= (NSInteger)[mapping count]) { + return; // The store changed since the results were built. + } position = [mapping[selectedRow] intValue]; } - - NSString *content = [flycutOperator getPasteFromIndex:position]; - if (content) { - [self addClipToPasteboard:content]; - [self updateMenu]; // Update menu like bezel does + + if ( [self pasteStorePositionAndUpdate:position] ) { [self hideSearchWindow]; - + // Always paste immediately (like bezel behavior), ignore menuSelectionPastes preference [self performSelector:@selector(fakeCommandV) withObject:nil afterDelay:0.3]; } @@ -2002,6 +2186,8 @@ - (void) dealloc { [searchRecorder release]; [searchWindow release]; // This will release its subviews automatically [searchResults release]; + [sRetiredMenuClippingItems release]; + sRetiredMenuClippingItems = nil; [super dealloc]; } diff --git a/FlycutOperator.h b/FlycutOperator.h index 362db60..5da909a 100644 --- a/FlycutOperator.h +++ b/FlycutOperator.h @@ -73,6 +73,10 @@ -(BOOL) isValidClippingNumber:(NSNumber *)number; -(NSString *) clippingStringWithCount:(int)count; +// Read-only access to a stored clipping, for callers that need to hold on to a +// specific clipping across store changes. Returns nil for an out-of-range position. +-(FlycutClipping *) clippingAtPosition:(int)position; + // Save and load -(void) saveEngine; -(bool) loadEngineFromPList; diff --git a/FlycutOperator.m b/FlycutOperator.m index ca5c581..3bb54c0 100644 --- a/FlycutOperator.m +++ b/FlycutOperator.m @@ -339,8 +339,13 @@ - (NSString*)getPasteFromIndex:(int) position { NSString *clipping = [self getClipFromCount:position]; if ( [[NSUserDefaults standardUserDefaults] boolForKey:@"pasteMovesToTop"] ) { - [clippingStore clippingMoveToTop:position]; + // Settle the stack position BEFORE moving, because the store redraws the bezel from + // inside the move (delegateEndUpdates -> -[AppController endUpdates]). Assigning + // afterwards meant that redraw used the pre-move position, which then pointed at the + // neighbour of the clipping being pasted - a visible flash of the wrong entry. + // Once the clipping has been moved to the top, position 0 is that clipping. stackPosition = 0; + [clippingStore clippingMoveToTop:position]; [self actionAfterListModification]; } @@ -462,15 +467,35 @@ -(int)indexOfClipping:(NSString*)contents ofType:(NSString*)type fromApp:(NSStri -(bool)addClipping:(NSString*)contents ofType:(NSString*)type fromApp:(NSString *)appName withAppBundleURL:(NSString *)bundleURL target:(id)selectorTarget clippingAddedSelector:(SEL)clippingAddedSelector { if ( [clippingStore jcListCount] == 0 || ! [contents isEqualToString:[clippingStore clippingContentsAtPosition:0]]) { + // Remember which clipping the stack position points at, so the selection can follow it + // across the insert. Retained because the store may drop clippings while inserting. + // + // Position 0 is excluded on purpose. It is where every selection starts, so it does + // not express a choice, and the bezel shows whatever sits at the top - following the + // old top clipping there would move the selection away from what the user is looking + // at. Staying at 0 keeps the display and the paste in agreement. + FlycutClipping *anchor = nil; + if ( stackPosition > 0 && stackPosition < [clippingStore jcListCount] ) + anchor = [[clippingStore clippingAtPosition:stackPosition] retain]; + bool success = [clippingStore addClipping:contents ofType:type fromAppLocalizedName:appName fromAppBundleURL:bundleURL atTimestamp:[[NSDate date] timeIntervalSince1970]]; -// The below tracks our position down down down... Maybe as an option? -// if ( [clippingStore jcListCount] > 1 ) stackPosition++; - stackPosition = 0; + // Follow the clipping rather than the position. This used to be stackPosition = 0, + // which moved a selection the user was making in the bezel onto the clipping that had + // just arrived - so they pasted the newest entry instead of the one they had picked. + // -indexOfClipping: matches on contents, which is what is wanted here: two clippings + // with equal contents are interchangeable, and it also covers removeDuplicates, where + // the existing copy is moved to the top instead of a new one being inserted. + // A new selection starts at the newest clipping because opening the bezel says so, + // not because an unrelated store change reset the position. + int followed = ( nil != anchor ) ? [clippingStore indexOfClipping:anchor] : -1; + stackPosition = ( followed >= 0 ) ? followed : 0; + [anchor release]; + [selectorTarget performSelector:clippingAddedSelector]; [self actionAfterListModification]; @@ -503,6 +528,14 @@ -(int)jcListCount return [clippingStore jcListCount]; } +-(FlycutClipping *) clippingAtPosition:(int)position +{ + if ( position < 0 || position >= [clippingStore jcListCount] ) + return nil; + + return [clippingStore clippingAtPosition:position]; +} + -(int)rememberNum { return [clippingStore rememberNum];