Skip to content

A save that doesn't happen leaves the close pending, so a later Ctrl+S closes the window #23

Description

@rodgco

On master, a save that does not happen leaves the window's pending close behind, so an ordinary save minutes later closes the window.

What happens

  1. Open a document and change it.
  2. Close the window. The unsaved-changes dialog appears; answer Save.
  3. The save fails — the footer reports Could not save doc.md. and the window stays open, which is right.
  4. Carry on writing. Whatever caused the failure goes away.
  5. Press Ctrl+S. The save succeeds — and the window closes.

The close is one the writer asked for in step 2 and that visibly did not happen. Nothing since then has asked to close anything.

Why

Main.qml's unsaved-changes handler starts the save and leaves the intent standing:

onSaveRequested: {
    win.awaitingPendingSave = true;
    backend.save();
}

pendingAction is only ever cleared in three places: completePendingAction(), the Save As dialog being rejected, and the dialog's Cancel. A save that ends without saving passes none of them, so pendingAction stays "close". The next saveSucceeded — from any later save at all — reaches

function onSaveSucceeded() {
    win.awaitingPendingSave = false;
    if (win.pendingAction !== "")
        win.completePendingAction();
}

and completes the close.

The backend already gets this right for its own latch: Backend::saveTo() clears m_closeAfterSave when QSaveFile::open() fails (src/backend.cpp:476). The QML side has no equivalent, so the two disagree about whether a close is still pending.

Reproduced

Against master at 8f98892, in a window driven by the test harness. A directory is put on the document's path so the save fails for any uid:

after the failed save:        status = "Could not save doc.md."  pendingAction = "close"  closeConfirmed = false
after an ordinary later save: closeConfirmed = true              pendingAction = ""

Step 5 uses the same call the dialog's Save handler makes.

Notes

Today the only route in is a failed save, which is rare. It is worth fixing on its own account, but also because any future path where save() declines to write — asking a question instead of failing, say — would make the same latch reachable from a deliberate answer rather than an error. PR #13 adds one such path, which is how this was found.

No fix proposed here, since where the clear belongs is a design question: mirroring the backend (clear the pending action wherever a save ends without writing) and reporting the outcome to the window (so it can decide) both seem reasonable, and they differ in how much the window is told.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions