Skip to content

Fix false "unsaved changes" prompt when a document is reverted to its original state - #9

Open
beefllama wants to merge 4 commits into
omacom-io:masterfrom
beefllama:fix-4
Open

Fix false "unsaved changes" prompt when a document is reverted to its original state#9
beefllama wants to merge 4 commits into
omacom-io:masterfrom
beefllama:fix-4

Conversation

@beefllama

Copy link
Copy Markdown

Summary

Fix the false "unsaved changes" prompt when a document is reverted to its original state.

  • Fixes Don't offer to save empty document #4 + the same unsaved changes behavior on existing files.
  • Added two regression tests in tests/tst_omawrite.cpp:
    • revertsEmptyDocumentToCleanState() — typing then deleting all text on a new document.
    • revertsSavedDocumentToCleanState() — editing an opened file then reverting to its saved contents.

Details

Backend::editorTextChanged() set the modified flag whenever the editor text differed from the last known text, but never cleared it when the text returned to its baseline. Typing text and then deleting it back to the original content left modified = true, so closing the window prompted to save changes that didn't exist.

Fix

editorTextChanged() now compares the current text against a baseline (the on-disk contents, or an empty string for a new file) and clears the modified flag when they match.

The revert check treated m_lastKnownFileText as authoritative even where the file behind it had gone or had been changed by someone else, so two paths reported a document with unsaved work as clean, and Main.qml's onClosing gates purely on backend.modified.

Deleting the file externally left the old baseline in place, because the watcher skipped the clear for deletions. Editing and undoing back to the deleted file's text then read as unmodified, set the status to "Saved <name>", and dropped the recovery snapshot for a file that no longer existed; closing the window discarded the only remaining copy without a prompt.

The other path is dismissal. ExternalChangeDialog closes on Escape without reaching keepExternalVersion() or reloadFromDisk(), and keepExternalVersion() also lands on an unknown baseline when the disk read fails, so the empty string stood in for "we do not know". Emptying the editor then matched it and cleared the modified flag.

The watcher now goes through setKnownFileContents() like every other writer, deletions included, and the comparison only trusts an empty baseline for a document with no file behind it -- which is the untitled case issue omacom-io#4 is about, and it still passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@omarchybot

Copy link
Copy Markdown
Collaborator

The baseline approach is right. Two paths let it call a document clean when we did not actually know what was on disk, so I pushed one fix to fix-4.

Pushed e08b2ff. Main.qml's onClosing gates purely on backend.modified, so a wrong answer there closes the window with no prompt.

  • External deletion kept the old baseline, because the new watcher block skips the clear when deleted. Delete the file from another terminal, dismiss the dialog, then edit and undo back to the old text: modified went false, the status read Saved <name>, clearRecovery() dropped the snapshot, and closing discarded the only copy left.
  • ExternalChangeDialog has closePolicy: Popup.CloseOnEscape, so Escape closes it without reaching keepExternalVersion() or reloadFromDisk() — and keepExternalVersion() itself ends on an unknown baseline when the disk read fails. In that state the empty string stood in for "we do not know", so emptying the editor matched it and cleared the modified flag.

The fix routes the watcher through setKnownFileContents() like every other writer, deletions included, and only trusts an empty baseline for a document with no file behind it — which keeps #4 working, since that is the untitled case. keepsADocumentUnsavedAfterItsFileDisappears covers both; I checked each half by reverting it alone and watching that test fail. 15/15 pass.

Still open, not pushed:

  • setKnownFileContents() normalises \r\n in the baseline, but the editor side comes from QTextDocument::toPlainText(), which also folds U+00A0 to a space and U+2028/U+2029 to \n. A file containing a non-breaking space can never match its own baseline, so the false prompt survives for that file — I measured baseline 61 c2 a0 62 e2 80 a8 63 against editor text 61 20 62 0a 63. Deriving the baseline through a QTextDocument instead of a string replace would cover it, but that is a design call rather than a patch.
  • Reverting a file that was opened and never saved sets the status to Saved <name>. Accurate about the bytes, misleading about the event.
  • QStandardPaths::setTestModeEnabled(true) is the right call, but ~/.qttest persists between runs and attachDocument() reaches restoreRecovery(), which claims any snapshot it finds there. An aborted run can leave one that fails the opening assertion of revertsEmptyDocumentToCleanState. A per-run data directory would close it.

Worth knowing about #13: it inserts an early return into Backend::open above the lines you replaced, and sets m_lastKnownFileContents/m_hasKnownFileContents by hand rather than through your helper, which leaves m_lastKnownFileText holding the previous file's text. The hunks do not overlap, so git merges them cleanly in either order and nothing flags it. Whichever lands second, those two assignments want to become a setKnownFileContents() call.

Separately and not yours: open() reads with QIODevice::Text so m_lastKnownFileContents holds LF, while the watcher re-reads the file raw and compares bytes. A CRLF file never matches itself there, so it raises the external-change dialog on any touch. That predates this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't offer to save empty document

2 participants