Fix data loss: save-and-delete in the favorites store deleted from the main list - #12
Open
MiMoHo wants to merge 1 commit into
Open
Fix data loss: save-and-delete in the favorites store deleted from the main list#12MiMoHo wants to merge 1 commit into
MiMoHo wants to merge 1 commit into
Conversation
Shift+S in the bezel saves the selected clipping to a file and then removes it.
With the favorites store active ('f' in the bezel) it removed the wrong
clipping, from the wrong list:
bool success = [flycutOperator saveFromStack]; // reads the favorites store
[self performSelector:@selector(hideApp) ...];
[self restoreStashedStoreAndUpdate]; // swaps the store AND the position
if ( success && shift ) [flycutOperator clearItemAtStackPosition];
-restoreStashedStore swaps clippingStore back to the main store and swaps
stackPosition with stashedStackPosition, so the delete that follows hits the
main list at whatever position the user had been at there before pressing 'f'.
The favorite was saved and kept, and an unrelated clipping - one the user was
never shown - was destroyed instead.
Deleting before the restore fixes it: while the store that was saved from is
still the current one, clearItemAtStackPosition removes exactly the clipping
that was saved. Without an active stash -restoreStashedStore returns NO and
the path is unchanged, so plain Shift+S in the main list behaves exactly as
before.
Verified with a test program driving the real FlycutOperator, modelling both
orders (main list MAIN0..MAIN5 with the cursor at position 3, then 'f',
favorites FAV0..FAV2 with the cursor at position 1, then Shift+S):
before: main 6 -> 5, "MAIN2" gone, favorites unchanged (data loss)
after: main unchanged at 6, favorites 3 -> 2, FAV1 gone (correct)
Builds with Xcode 26.5, 76 warnings, identical to master.
Co-Authored-By: Claude <noreply@anthropic.com>
Author
|
Follow-up: #13 makes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Data loss
Shift+Sin the bezel saves the selected clipping to a file and then removes it. With the favorites store active (fin the bezel) it removed the wrong clipping, from the wrong list:restoreStashedStoreswapsclippingStoreback to the main store and swapsstackPositionwithstashedStackPosition. The delete that follows therefore hits the main list, at whatever position the user happened to be at there before pressingf.So the favorite was saved and kept, and an unrelated clipping — one the user was never shown and never selected — was destroyed instead. Silently.
Fix
Delete while the store that was saved from is still the current one.
clearItemAtStackPositionthen removes exactly the clipping that was saved, and thehideAppdelay and the restore follow unchanged.Without an active stash
restoreStashedStorereturnsNO, so plainShift+Sin the main list takes the same path as before — the behaviour change is confined to the favorites store.Verification
A test program drives the real
FlycutOperatorand models both orders. Main listMAIN0..MAIN5with the cursor at position 3, thenf, favoritesFAV0..FAV2with the cursor at position 1, thenShift+S:The test deliberately does not call
saveFromStack— its role in the defect is only to return success, and it writes a file to the Desktop — and it forcessaveForgottenClippings,saveForgottenFavoritesandsavePreferenceoff so nothing touches the disk.Builds with Xcode 26.5 (
BUILD SUCCEEDED), 76 warnings, identical tomasterbuilt the same way.Scope, and what is deliberately left alone
This changes only the order of the delete and the store restore in the bezel's
s/Shandler. Auditing the area turned up several other things around the favorites stash. They are pre-existing, independent of this change, and left for separate PRs so this one stays reviewable:Esc,hideApp,windowDidResignKey:andapplicationWillResignActive:never callrestoreStashedStoreAndUpdate, so afterf+Escthe favorites store stays active and later copies land there.saveEnginewrites the currentclippingStoreunder thejcListkey without knowing aboutstashedStore, so while swapped it persists the favorites list as the main history. This is the root cause of the point above.checkRememberNumPref:forPrimaryStore:passesYESunconditionally at one call site, so cancelling the resize warning for the favorites store writes its value underrememberNum.saveFromStore:atIndex:withPrefix:ignores theBOOLfromwriteToFile:and always returnsYES, soShift+Sdeletes even when the file was not written.One honest note about this change: with
savePreference >= 2("after each change"), the now-earlierclearItemAtStackPositiontriggersactionAfterListModificationwhile the store is still swapped, so onesaveEngineruns against the favorites store. That is the pre-existing aliasing above rather than something this patch introduces — onmasterthe same write happened, and it persisted the wrong deletion. It corrects itself on the next list change or on quit. Fixing it properly means makingsaveEnginestash-aware, which belongs in its own change.Not verified here
The bezel's own appearance during the ~0.2 s before it hides — one
updateBezelon the favorites store, then the restore'supdateBezelon the main store — was assessed from the code only. Keyboard synthesis is blocked by TCC from a terminal context, so the bezel cannot be driven programmatically. If you would like that frame checked on screen before merging, say so.Authored and reviewed with Claude Code using Claude Opus 5 at
xhighreasoning effort. Cross-checked by a multi-agent adversarial review (independent Opus 5 agents on correctness, side effects, same-class defects elsewhere, and upstream fit; every finding then put to three skeptics tasked with refuting it — 20 raised, 6 survived, 14 refuted, and the surviving ones are the scope notes above). The patch applies cleanly tomasterand to all nine other open branches from this fork.🤖 Generated with Claude Code