Skip to content

[lua] Avoid setting window bounds when the app is closing (fix #5384)#5389

Merged
dacap merged 1 commit into
aseprite:mainfrom
ckaiser:bounds-on-exit
Oct 24, 2025
Merged

[lua] Avoid setting window bounds when the app is closing (fix #5384)#5389
dacap merged 1 commit into
aseprite:mainfrom
ckaiser:bounds-on-exit

Conversation

@ckaiser

@ckaiser ckaiser commented Aug 27, 2025

Copy link
Copy Markdown
Member

Reverts the previous fix and attempts something slightly more generic (previous fix also failed when fetching parent display.

Here's a script to reproduce the crash(es):

dlg1 = Dialog {
    id = "dlg1",
    title = "First Dialog",
    parent=nil,
    onclose = function()
        dlg1:modify{ id="dlg1_close_fit",  visible = true }
    end
}
dlg1:label { id = "dlg1_close_fit", text = "!!!!!!!!!!!!!!", visible=false }

dlg1:button{ id="dlg1_button", label="Dialog 2:", text="Show", onclick=function()
    dlg2 = Dialog {
        id = "dlg2",
        title = "Dialog",
        onclose = function()
            dlg2:modify{ id="dlg2_close_fit",  visible = true }
        end
    }
    dlg2:label { id = "dlg2_close_fit", text = "!!!!!!!!!!!!!!", visible=false }
    dlg2:button{ id= "exit", text = "Exit", onclick=function()
        app.command.Exit()
    end}
    dlg2:show{}
end}

dlg1:show {}

Basically any bound changes (in this case triggered by :modify, but it could just be a call to .bounds, would crash since we'd already cleaned up the main window.

@ckaiser ckaiser requested a review from dacap as a code owner August 27, 2025 21:01
@ckaiser ckaiser assigned ckaiser and dacap and unassigned ckaiser Aug 27, 2025
@aseprite-bot

Copy link
Copy Markdown
Collaborator

Hi there!

One or more of the commit messages in this PR do not match our code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@aseprite-bot aseprite-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/app/script/dialog_class.cpp Outdated
@dacap

dacap commented Oct 23, 2025

Copy link
Copy Markdown
Member

I'm testing this and it works great 👍

As a side note it looks like ee1e745 is not required, should I cherry pick c056f92 only? Or do you want to update this PR without the revert commit?

@dacap dacap assigned ckaiser and unassigned dacap Oct 23, 2025
@ckaiser ckaiser assigned dacap and unassigned ckaiser Oct 24, 2025
@aseprite-bot

Copy link
Copy Markdown
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@dacap dacap merged commit 827c29f into aseprite:main Oct 24, 2025
12 checks passed
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.

3 participants