Skip to content

FIX: Editor closes when clicking cancel in a save dialog prompt (UUM-134748)#2366

Open
Darren-Kelly-Unity wants to merge 4 commits intodevelopfrom
bugfix/uum-134748-editor-still-closes-on-dialog-cancel
Open

FIX: Editor closes when clicking cancel in a save dialog prompt (UUM-134748)#2366
Darren-Kelly-Unity wants to merge 4 commits intodevelopfrom
bugfix/uum-134748-editor-still-closes-on-dialog-cancel

Conversation

@Darren-Kelly-Unity
Copy link
Collaborator

@Darren-Kelly-Unity Darren-Kelly-Unity commented Mar 4, 2026

Description

When there is a pending change that could be saved a dialog is prompted asking if you would like to save, don't save or cancel the closing of Unity.

When clicking cancel Unity would close anyway without saving the asset.

Ticket:
https://jira.unity3d.com/browse/UUM-134748

Testing status & QA

Manually tested, not easy to add an automated test for.

How to reproduce:

  1. Create and open a new UPR project
  2. Open the "InputSystem_Actions" asset in the Input Actions Editor window
  3. Make any change to the file
  4. Attempt to close the Editor
  5. In the "Input Action Asset has been modified" dialog box, press Cancel
  6. Observe the Editor

Overall Product Risks

  • Complexity: 1
  • Halo Effect: 1

Comments to reviewers

I tried to re-use existing code by extending the method, I am not sure about the naming, let me know what you think.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@Darren-Kelly-Unity Darren-Kelly-Unity changed the title Editor closes when clicking cancel in a save dialog prompt. FIX: Editor closes when clicking cancel in a save dialog prompt (UUM-134748) Mar 4, 2026
@u-pr
Copy link
Contributor

u-pr bot commented Mar 4, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

UUM-134748 - Partially compliant

Compliant requirements:

  • If the user presses Cancel in that dialog, the Unity Editor must stay open (quitting must be aborted).

Non-compliant requirements:

  • Repro steps should no longer result in the Editor closing on Cancel.

Requires further human verification:

  • Repro steps should no longer result in the Editor closing on Cancel.
  • When closing the Unity Editor with unsaved changes in the Input Actions Editor, Unity should show the "Input Action Asset has been modified" dialog.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

Small, localized change adding a quit-hook and refactoring the existing OnDestroy prompt logic, with a few lifecycle/edge-case behaviors to sanity-check.

🏅 Score: 84

The fix direction is sound (blocking quit via EditorApplication.wantsToQuit), but there are a couple of lifecycle/flag semantics that could cause subtle regressions and merit tightening.

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

State Flag

m_IsEditorQuitting is used as a guard to skip prompting, but it is set based on the dialog result rather than “is currently in an editor-quit flow”; consider renaming (e.g., m_QuitAllowed / m_IsInQuitPrompt) and/or resetting it deterministically so the intent is clear and repeated wantsToQuit invocations can’t accidentally bypass prompting.

private bool m_IsEditorQuitting;

private StateContainer m_StateContainer;
private InputActionsEditorView m_View;

private InputActionsEditorSessionAnalytic m_Analytics;
Lifecycle Logic

OnDestroy() calls OnDestroyIsApplicationAllowedToQuit(true) but ignores the return value; ensure this cannot lead to odd behavior in non-quit window destruction paths (e.g., window being closed/teardown happening while the cancel path is trying to “reshow”).

private bool OnDestroyIsApplicationAllowedToQuit(bool rebuildUIOnCancel)
{
    // Do we have unsaved changes that we need to ask the user to save or discard?
    if (!m_IsDirty)
        return true;

    // Get target asset path from GUID, if this fails file no longer exists and we need to abort.
    var assetPath = AssetDatabase.GUIDToAssetPath(m_AssetGUID);
    if (string.IsNullOrEmpty(assetPath))
        return true;

    if (!m_IsEditorQuitting)
    {
        // Prompt user with a dialog
        var result = Dialog.InputActionAsset.ShowSaveChanges(assetPath);
        switch (result)
        {
            case Dialog.Result.Save:
                Save(isAutoSave: false);
                return true;
            case Dialog.Result.Cancel:
                if (rebuildUIOnCancel)
                {
                    // Cancel editor quit. (open new editor window with the edited asset)
                    ReshowEditorWindowWithUnsavedChanges();
                }

                return false;
            case Dialog.Result.Discard:
                // Don't save, quit - reload the old asset from the json to prevent the asset from being dirtied
                return true;
            default:
                throw new ArgumentOutOfRangeException(nameof(result));
        }
    }

    return true;
}

private void OnDestroy()
{
    OnDestroyIsApplicationAllowedToQuit(true);
Prompt Reentrancy

Verify EditorApplication.wantsToQuit isn’t invoked multiple times per quit attempt causing multiple dialogs; the guard helps, but it’s worth confirming the flow when multiple windows register handlers and quit is cancelled then retried.

private void OnEnable()
{
    analytics.Begin();
    EditorApplication.wantsToQuit += OnWantsToQuit;
}

private void OnDisable()
{
    analytics.End();
    EditorApplication.wantsToQuit -= OnWantsToQuit;
}

private bool OnWantsToQuit()
{
    // Here the user will be prompted
    bool isAllowedToQuit = OnDestroyIsApplicationAllowedToQuit(false);
    m_IsEditorQuitting = isAllowedToQuit;
    return m_IsEditorQuitting;
}
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@u-pr
Copy link
Contributor

u-pr bot commented Mar 4, 2026

PR Code Suggestions ✨

No code suggestions found for the PR.

@Darren-Kelly-Unity Darren-Kelly-Unity requested a review from ekcoh March 4, 2026 10:21
@codecov-github-com
Copy link

codecov-github-com bot commented Mar 4, 2026

Codecov Report

Attention: Patch coverage is 39.13043% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...Editor/UITKAssetEditor/InputActionsEditorWindow.cs 39.13% 14 Missing ⚠️
@@             Coverage Diff             @@
##           develop    #2366      +/-   ##
===========================================
- Coverage    77.90%   77.89%   -0.02%     
===========================================
  Files          476      476              
  Lines        97613    97642      +29     
===========================================
+ Hits         76048    76056       +8     
- Misses       21565    21586      +21     
Flag Coverage Δ
inputsystem_MacOS_2022.3 5.52% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_2022.3_project 75.38% <0.00%> (-0.02%) ⬇️
inputsystem_MacOS_6000.0 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.0_project 77.28% <39.13%> (-0.01%) ⬇️
inputsystem_MacOS_6000.3 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.3_project 77.28% <39.13%> (-0.01%) ⬇️
inputsystem_MacOS_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.4_project 77.29% <39.13%> (-0.01%) ⬇️
inputsystem_MacOS_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.5_project 77.29% <39.13%> (-0.02%) ⬇️
inputsystem_MacOS_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.6_project 77.29% <39.13%> (-0.01%) ⬇️
inputsystem_Ubuntu_2022.3_project 75.18% <0.00%> (-0.02%) ⬇️
inputsystem_Ubuntu_6000.0 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.0_project 77.09% <39.13%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.3 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.3_project 77.09% <39.13%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.4_project 77.10% <39.13%> (-0.02%) ⬇️
inputsystem_Ubuntu_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.5_project 77.09% <39.13%> (-0.02%) ⬇️
inputsystem_Ubuntu_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.6_project 77.09% <39.13%> (-0.02%) ⬇️
inputsystem_Windows_2022.3 5.52% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_2022.3_project 75.51% <0.00%> (-0.02%) ⬇️
inputsystem_Windows_6000.0 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.0_project 77.41% <39.13%> (-0.01%) ⬇️
inputsystem_Windows_6000.3 5.30% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.3_project 77.41% <39.13%> (-0.01%) ⬇️
inputsystem_Windows_6000.4 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.4_project 77.42% <39.13%> (-0.01%) ⬇️
inputsystem_Windows_6000.5 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.5_project 77.42% <39.13%> (-0.01%) ⬇️
inputsystem_Windows_6000.6 5.31% <0.00%> (-0.01%) ⬇️
inputsystem_Windows_6000.6_project 77.42% <39.13%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...Editor/UITKAssetEditor/InputActionsEditorWindow.cs 53.51% <39.13%> (-1.14%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@MorganHoarau MorganHoarau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions, but does the work!

private bool HandleOnDestroyIfApplicationIsAllowedToQuit(bool rebuildUIOnCancel)
{
// Do we have unsaved changes that we need to ask the user to save or discard?
if (!m_IsDirty)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code could be simplify.

Suggested change
if (!m_IsDirty)
// Early out if asset up to date or editor closing.
if (!m_IsDirty || m_IsEditorQuitting)

/// Shows a dialog when trying to close an input asset without saving changes.
/// </summary>
/// <returns> Returns true if you should allow the Unity Editor to close. </returns>
private bool HandleOnDestroyIfApplicationIsAllowedToQuit(bool rebuildUIOnCancel)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having issue with the semantic mismatch of what the method is doing versus what it is returning.
The method is executing a command (should return void) but returns a query (return a value).

What about a change of signature:

Suggested change
private bool HandleOnDestroyIfApplicationIsAllowedToQuit(bool rebuildUIOnCancel)
private bool CanCloseWindow(bool promptIfDirty, bool rebuildUIOnCancel)

Or

Suggested change
private bool HandleOnDestroyIfApplicationIsAllowedToQuit(bool rebuildUIOnCancel)
private bool CheckCanCloseAndPromptIfDirty(bool rebuildUIOnCancel)

/// <summary>
/// Shows a dialog when trying to close an input asset without saving changes.
/// </summary>
/// <returns> Returns true if you should allow the Unity Editor to close. </returns>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <returns> Returns true if you should allow the Unity Editor to close. </returns>
/// <param name="rebuildUIOnCancel">If true, reopens the editor window when user cancels.</param>
/// <returns> Returns true if you should allow the Unity Editor to close. </returns>

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.

2 participants