From f007af506fa820df0589c234ee094fb4709e7c13 Mon Sep 17 00:00:00 2001 From: Milan Hoppe Date: Sun, 5 Jul 2026 02:29:51 +0200 Subject: [PATCH 1/7] Fix menu click sometimes pasting the newest clipping instead of the clicked one The pollPB: timer runs in NSRunLoopCommonModes, so a clipboard change can be noticed while the status menu is open (deliberate, for Universal Clipboard). That triggers updateMenu, which removes every clipping item and inserts new NSMenuItem objects. If the rebuild lands between the user's click and the action dispatch, the clicked item is no longer in the menu, [sender menu] is nil, and [[sender menu] indexOfItem:sender] messages nil and yields 0 -- pasting stack position 0, the most recently copied clipping, instead of the entry the user clicked. With the 1-second poll interval this commonly happens when the user copies something and opens the menu right away. - processMenuClippingSelection: bail out when the sender is orphaned or its index can't be resolved, instead of pasting the wrong clipping. - pasteIndexAndUpdate: now returns whether a clipping was placed on the pasteboard, so no Cmd-V is faked when nothing was pasted (previously that re-pasted whatever was already on the pasteboard). Also bounds-check the search mapping, which could throw NSRangeException. - searchWindowItemSelected: use clickedRow for double-clicks so a selection change between click and action can't redirect the paste to row 0, and bounds-check the search mapping. Co-Authored-By: Claude Fable 5 --- AppController.m | 60 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/AppController.m b/AppController.m index 3898ab0..cd98ccd 100755 --- a/AppController.m +++ b/AppController.m @@ -906,21 +906,25 @@ - (void)moveItemAtStackPositionToTopOfStack } } -- (void)pasteIndexAndUpdate:(int) position { +// Returns true if a clipping was found and placed on the pasteboard. +- (bool)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]; + if ( position < 0 || position >= (int)[mapping count] ) + return false; // The list changed since the menu was built. position = [mapping[position] intValue]; } 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 @@ -1536,8 +1540,22 @@ - (void)updateMenuContaining:(NSString*)search { -(IBAction)processMenuClippingSelection:(id)sender { - int index=[[sender menu] indexOfItem:sender]; - [self pasteIndexAndUpdate:index]; + // pollPB: runs in NSRunLoopCommonModes, so a clipboard change noticed while + // the menu is open triggers updateMenu, which replaces every clipping item. + // If that rebuild lands between the click and this action, the clicked item + // is no longer in the menu and [sender menu] is nil, so the previous + // [[sender menu] indexOfItem:sender] messaged nil and yielded index 0 -- + // pasting the most recent clipping instead of the one the user clicked. + // Bail out instead of pasting the wrong clipping. + NSMenu *senderMenu = [sender menu]; + if ( nil == senderMenu ) + return; + NSInteger index = [senderMenu indexOfItem:sender]; + if ( index < 0 ) + return; + + if ( ! [self pasteIndexAndUpdate:(int)index] ) + return; // Nothing was placed on the pasteboard, so don't fake a Cmd-V. if ( [[NSUserDefaults standardUserDefaults] boolForKey:@"menuSelectionPastes"] ) { [self performSelector:@selector(hideApp) withObject:nil]; @@ -1894,19 +1912,35 @@ - (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]; } From e90f7b00ec15bc9930034adc1b94dc5d73ccc279 Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Sun, 19 Jul 2026 14:45:03 +0200 Subject: [PATCH 2/7] Freeze clipping capture while the menu or search window is open pollPB: runs in NSRunLoopCommonModes so it keeps firing during menu tracking. When it notices a clipboard change mid-selection it inserts a new clipping at store index 0, shifting every clipping's index by one. The pending selection is then resolved against the shifted store, so the user pastes the wrong entry - typically the freshly-arrived newest one - instead of the one they clicked. The same shift breaks the search window, whose result list is a snapshot while getPasteFromIndex: uses live indices. Guard pollPB: to skip capture while isMenuOpen or isSearchWindowDisplayed, without touching pbCount so the change is still detected and captured the moment the surface closes (menuDidClose: fires a catch-up poll). Track menu open state via menuWillOpen:/menuDidClose:. Co-Authored-By: Claude Opus 4.8 --- AppController.h | 1 + AppController.m | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) 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 cd98ccd..4a40708 100755 --- a/AppController.m +++ b/AppController.m @@ -337,7 +337,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,6 +369,10 @@ -(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]; @@ -391,7 +395,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 @@ -1070,6 +1077,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 shift every + // clipping's index by one. The pending selection is then resolved against the shifted + // store (see -pasteIndexAndUpdate: / -searchWindowItemSelected:), so the user pastes the + // wrong entry - typically the freshly-arrived one - instead of the one they clicked. + // 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 From c92a5646095160784eb26f821110ac8e488c6595 Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Sat, 25 Jul 2026 22:37:54 +0200 Subject: [PATCH 3/7] Resolve menu selections by clipping identity instead of menu position Follow-up to the first commit, which stopped the wrong paste but could only bail out: when the rebuild had already detached the clicked item there was nothing left to resolve, so the click did nothing. Each clipping row now carries what it stands for instead of relying on where it sits: -updateMenuContaining: attaches [store position, display string] as the item's representedObject, and -storePositionForMenuItem: resolves the click from that, using the position only as a hint that is re-validated against the current store. -[FlycutClipping displayString] returns the same string object on every call and -previousDisplayStrings: passes those objects through unchanged, so pointer equality identifies exactly one clipping; a content comparison is only used as a fallback for a store reloaded from disk. A menu rebuild between the click and the delivery of the action is therefore harmless rather than merely detected. While the store position is known at build time, the search mapping is resolved there as well, so -pasteStorePositionAndUpdate: (renamed from -pasteIndexAndUpdate:) no longer reads the menu's search box at paste time. That box is cleared asynchronously by -updateMenu, so a click arriving after the clearing used to resolve a filtered row number against the unfiltered store - another way to paste the newest clipping. The bezel path benefits too: -moveItemAtStackPositionToTopOfStack passes a store position, which the old method would have run through the menu's search mapping. Menu items removed by a rebuild are held for one generation. jcMenu is their only owner, so a sender AppKit delivers after the rebuild would otherwise be a freed object - which the first commit's nil check would already have had to touch. When a click still cannot be resolved, nothing is pasted, the failure is logged without any clipping contents and NSBeep gives feedback, so "nothing happened" can be told apart from a wrong paste in a bug report. Co-Authored-By: Claude --- AppController.m | 170 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 128 insertions(+), 42 deletions(-) diff --git a/AppController.m b/AppController.m index 4a40708..23114dc 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 @@ -906,24 +912,23 @@ - (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]; } } -// Returns true if a clipping was found and placed on the pasteboard. -- (bool)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]; - if ( position < 0 || position >= (int)[mapping count] ) - return false; // The list changed since the menu was built. - 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 ) @@ -1079,10 +1084,10 @@ -(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 shift every - // clipping's index by one. The pending selection is then resolved against the shifted - // store (see -pasteIndexAndUpdate: / -searchWindowItemSelected:), so the user pastes the - // wrong entry - typically the freshly-arrived one - instead of the one they clicked. + // 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 ) @@ -1526,7 +1531,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]; @@ -1537,18 +1551,47 @@ - (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]; + } + NSMenuItem *item; item = [[NSMenuItem alloc] initWithTitle:[clipStrings objectAtIndex:i] action:@selector(processMenuClippingSelection:) keyEquivalent:@""]; [item setTarget:self]; [item setEnabled:YES]; + // Remember what the row stands for rather than where it sits: the display string + // is the clipping's own (pointer-stable) string and therefore an identity, the + // position is only a hint that -storePositionForMenuItem: re-validates. + // representedObject is a strong property, so the item keeps the string alive even + // if the store drops the clipping in the meantime. + [item setRepresentedObject:[NSArray arrayWithObjects: + [NSNumber numberWithInt:storePosition], + [clipStrings objectAtIndex:i], + 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]; @@ -1556,28 +1599,72 @@ - (void)updateMenuContaining:(NSString*)search { }); } --(IBAction)processMenuClippingSelection:(id)sender +// Works out which clipping a menu item stands for. The item carries its own identity (the +// clipping's display string) 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 { - // pollPB: runs in NSRunLoopCommonModes, so a clipboard change noticed while - // the menu is open triggers updateMenu, which replaces every clipping item. - // If that rebuild lands between the click and this action, the clicked item - // is no longer in the menu and [sender menu] is nil, so the previous - // [[sender menu] indexOfItem:sender] messaged nil and yielded index 0 -- - // pasting the most recent clipping instead of the one the user clicked. - // Bail out instead of pasting the wrong clipping. - NSMenu *senderMenu = [sender menu]; - if ( nil == senderMenu ) - return; - NSInteger index = [senderMenu indexOfItem:sender]; - if ( index < 0 ) - return; + id represented = [item representedObject]; + if ( ! [represented isKindOfClass:[NSArray class]] || 2 != [(NSArray *)represented count] ) + return -1; + + id hintNumber = [(NSArray *)represented objectAtIndex:0]; + id displayString = [(NSArray *)represented objectAtIndex:1]; + if ( ! [hintNumber isKindOfClass:[NSNumber class]] || ! [displayString isKindOfClass:[NSString class]] ) + return -1; + + int hint = [hintNumber intValue]; + NSArray *current = [flycutOperator previousDisplayStrings:[flycutOperator jcListCount] containing:nil]; + int count = (int)[current count]; - if ( ! [self pasteIndexAndUpdate:(int)index] ) - return; // Nothing was placed on the pasteboard, so don't fake a Cmd-V. + // -[FlycutClipping displayString] hands out the very same string object every time, so + // pointer equality identifies exactly one clipping even when several share the same + // truncated text. The hint makes the common case O(1). + if ( hint >= 0 && hint < count && [current objectAtIndex:hint] == displayString ) + return hint; + + for ( int i = 0; i < count; i++ ) + if ( [current objectAtIndex:i] == displayString ) + return i; + + // The store may have been rebuilt from disk, which produces new string objects. + for ( int i = 0; i < count; i++ ) + if ( [[current objectAtIndex:i] isEqualToString:(NSString *)displayString] ) + return i; + + return -1; +} + +-(IBAction)processMenuClippingSelection:(id)sender +{ + // 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]; } } @@ -1961,13 +2048,10 @@ - (IBAction)searchWindowItemSelected:(id)sender } 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]; } @@ -2054,6 +2138,8 @@ - (void) dealloc { [searchRecorder release]; [searchWindow release]; // This will release its subviews automatically [searchResults release]; + [sRetiredMenuClippingItems release]; + sRetiredMenuClippingItems = nil; [super dealloc]; } From bc2c64e9088ea6e15a9bcb7f14e9d10f0e1de7e7 Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Wed, 29 Jul 2026 16:48:26 +0200 Subject: [PATCH 4/7] Use the clipping itself as the menu row's identity, not its display string The previous commit claimed that -[FlycutClipping displayString] hands out a pointer-unique string per clipping. That is wrong for short strings: a display string 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. A test program built against FlycutClipping shows "test\nZeile A" and "test\nZeile B" both yielding 0x802e93b4acce3b26. The consequence was the very bug this PR is about, only rarer: with the hint position invalidated by a store change, resolution could land on the wrong twin, and the hint check itself could accept a position that a same-looking clipping had moved into. So carry the FlycutClipping object instead. Object pointers are exact, the comparison is cheaper than a string compare, and a contents comparison remains as a fallback for a store reloaded from disk, where the objects are new but two clippings with equal contents are interchangeable for pasting anyway. This needs one read-only accessor on FlycutOperator, -clippingAtPosition:, forwarding to the store; the clipping is fetched before the NSMenuItem is allocated so a store change mid-build cannot leak an item. Verified with a test program covering: unchanged store, shifted store, a same-looking twin sitting at the hint position, a store reloaded from disk, and a deleted clipping (which must resolve to -1 and paste nothing). All six resolve to the clipping the user clicked. Co-Authored-By: Claude --- AppController.m | 55 ++++++++++++++++++++++++++++-------------------- FlycutOperator.h | 4 ++++ FlycutOperator.m | 8 +++++++ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/AppController.m b/AppController.m index 23114dc..8a48965 100755 --- a/AppController.m +++ b/AppController.m @@ -1577,20 +1577,28 @@ - (void)updateMenuContaining:(NSString*)search { 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]; - // Remember what the row stands for rather than where it sits: the display string - // is the clipping's own (pointer-stable) string and therefore an identity, the - // position is only a hint that -storePositionForMenuItem: re-validates. - // representedObject is a strong property, so the item keeps the string alive even - // if the store drops the clipping in the meantime. + // 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], - [clipStrings objectAtIndex:i], + 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. @@ -1599,10 +1607,10 @@ - (void)updateMenuContaining:(NSString*)search { }); } -// Works out which clipping a menu item stands for. The item carries its own identity (the -// clipping's display string) 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. +// 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]; @@ -1610,28 +1618,29 @@ - (int)storePositionForMenuItem:(NSMenuItem *)item return -1; id hintNumber = [(NSArray *)represented objectAtIndex:0]; - id displayString = [(NSArray *)represented objectAtIndex:1]; - if ( ! [hintNumber isKindOfClass:[NSNumber class]] || ! [displayString isKindOfClass:[NSString class]] ) + id clipping = [(NSArray *)represented objectAtIndex:1]; + if ( ! [hintNumber isKindOfClass:[NSNumber class]] || ! [clipping isKindOfClass:[FlycutClipping class]] ) return -1; int hint = [hintNumber intValue]; - NSArray *current = [flycutOperator previousDisplayStrings:[flycutOperator jcListCount] containing:nil]; - int count = (int)[current count]; + int count = [flycutOperator jcListCount]; - // -[FlycutClipping displayString] hands out the very same string object every time, so - // pointer equality identifies exactly one clipping even when several share the same - // truncated text. The hint makes the common case O(1). - if ( hint >= 0 && hint < count && [current objectAtIndex:hint] == displayString ) + // 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 ( [current objectAtIndex:i] == displayString ) + if ( [flycutOperator clippingAtPosition:i] == clipping ) return i; - // The store may have been rebuilt from disk, which produces new string objects. - for ( int i = 0; i < count; i++ ) - if ( [[current objectAtIndex:i] isEqualToString:(NSString *)displayString] ) - 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; } 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..468b7c6 100644 --- a/FlycutOperator.m +++ b/FlycutOperator.m @@ -503,6 +503,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]; From c77ce768a795c7c808e879c4df25017259779783 Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Wed, 29 Jul 2026 19:44:42 +0200 Subject: [PATCH 5/7] Never let the capture freeze outlive the menu or the search window The freeze added in the second commit stops -pollPB: while isMenuOpen or isSearchWindowDisplayed is set. Both flags can be left standing, and because capture then never resumes, Flycut silently stops recording clippings with nothing in the UI or the log to say why. Option-click on the status icon is the documented way to pause capture while copying a password (help.md, readme.md). It cancels the menu from inside -menuWillOpen:, and AppKit does not send -menuDidClose: for a menu cancelled there - verified with a standalone AppKit program: menuWillOpen fires, menuDidClose never does. So isMenuOpen stayed set forever. The first option-click was harmless because -pollPB: also requires the store to be enabled, but the second one re-enabled the store while capture stayed frozen. Cleared in that branch, where the menu is not opening anyway. -windowDidResignKey: routed every window to -hideApp, which does not close the search window properly, so isSearchWindowDisplayed survived clicking away from it. It now closes the search window through -hideSearchWindow. Before the freeze this only leaked a bit of state; with it, capture died. Both flags are additionally cleared in -hideApp as a backstop: hiding the app tears down the menu and the search window anyway, and neither flag is meant to outlive its surface. Co-Authored-By: Claude --- AppController.m | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/AppController.m b/AppController.m index 8a48965..cad85f4 100755 --- a/AppController.m +++ b/AppController.m @@ -382,6 +382,12 @@ -(void)menuWillOpen:(NSMenu *)menu 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) { @@ -948,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]; } @@ -1347,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]; } From bcee0b7400743410e68277e04db170c981f54c17 Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Thu, 30 Jul 2026 00:41:57 +0200 Subject: [PATCH 6/7] Let the bezel selection follow its clipping instead of a position While the bezel is open, -pollPB: keeps capturing, and -addClipping: reset the stack position to 0 on every insert. A clipping arriving mid-selection therefore moved the selection onto it, and the user pasted the newest entry instead of the one they had picked - the same defect this PR fixes for the status menu, on the third selection surface. The position now follows the clipping it points at: -addClipping: anchors on that clipping before changing the store and looks up where it ended up afterwards. -indexOfClipping: matches on contents, which is what is wanted here - two clippings with equal contents are interchangeable for pasting, and it also covers removeDuplicates, where the existing copy is moved to the top rather than a new one being inserted. The anchor is retained because the store can drop clippings while inserting. Position 0 is deliberately not anchored. It is where every selection starts, so it expresses no choice, and the bezel always shows whatever sits at the top; following the old top clipping from there would move the selection away from what the user is looking at. Staying put keeps display and paste in agreement. Because the bezel opening at the newest clipping used to be a side effect of that reset, -hitMainHotKey: now says so explicitly. Not in -showBezel:, which the favourites toggle also calls after -toggleToFromFavoritesStore has swapped in that store's own remembered position. Finally, the store redraws the bezel from inside its own insert, by way of -endUpdates, before the position has settled - so the bezel would show the neighbour of the clipping a paste would deliver. The clipping-added callback now redraws once the position is settled. No new state is introduced, so there is no new way for a flag to be left standing and stop capture, which is what the previous commit had to repair twice. Verified with a test program against FlycutStore/FlycutClipping: a clipping arriving mid-selection, two in a row, an untouched bezel at position 0 (also with a single stored clipping, and after navigating back up to 0), the anchor pushed out by the rememberNum limit, and removeDuplicates both with a duplicate above the selection and of the selection itself. Co-Authored-By: Claude --- AppController.m | 22 +++++++++++++++++++++- FlycutOperator.m | 26 +++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/AppController.m b/AppController.m index cad85f4..5217c5e 100755 --- a/AppController.m +++ b/AppController.m @@ -1143,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)]; } }); } @@ -1385,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]; @@ -1811,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; diff --git a/FlycutOperator.m b/FlycutOperator.m index 468b7c6..3f0b47c 100644 --- a/FlycutOperator.m +++ b/FlycutOperator.m @@ -462,15 +462,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]; From a4aafc638af435315714073f1d4ef7f93218659c Mon Sep 17 00:00:00 2001 From: MiMoHo Date: Thu, 30 Jul 2026 02:11:45 +0200 Subject: [PATCH 7/7] Settle the stack position before moving a pasted clipping to the top With pasteMovesToTop enabled, -getPasteFromIndex: moved the clipping and only then set the stack position to 0. The store redraws the bezel from inside that move (-delegateEndUpdates -> -[AppController endUpdates], which always has needBezelUpdate set via -noteChangeAtIndex:), so the redraw ran with the pre-move position - which after the move points at the neighbour of the clipping being pasted. Reported from a visual test: pressing Return on the third entry flashed the fourth one for an instant. The paste itself was correct. Assigning the position first fixes it, because once the clipping has been moved to the top, position 0 is that clipping. Reproduced and verified with a test program that stands in for the store delegate and records what the bezel would draw at -endUpdates: before, the redraw shows the neighbour; after, it shows the clipping that is pasted. Co-Authored-By: Claude --- FlycutOperator.m | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/FlycutOperator.m b/FlycutOperator.m index 3f0b47c..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]; }