Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add file open / save prompts in scripting API #3782

Merged
merged 8 commits into from
Jul 29, 2023

Conversation

dogboydog
Copy link
Contributor

Fix #3537

I didn't write the scripting doc yet but I figured I could put up the C++ code for people to try & for review before I do that.

@dogboydog dogboydog marked this pull request as ready for review July 8, 2023 20:48
@dogboydog
Copy link
Contributor Author

dogboydog commented Jul 9, 2023

Example of making a "choose directory" UI using a Dialog

var ChooseDirectoryDialog = {};

ChooseDirectoryDialog.testPromptAction = tiled.registerAction("ChooseDirectory", function (action) {

    var dialog = new Dialog();
    dialog.windowTitle = 'Choose a Directory';
    dialog.newRowMode =  Dialog.ManualRows;
    var dirDisplay =dialog.addTextInput('');
    dirDisplay.minimumWidth = 200;
    dirDisplay.enabled = false;
    var dirButton = dialog.addButton('Browse');
    dirButton.clicked.connect(()=>{
       var chosenDir = tiled.promptDirectory('', 'Directory for Dialog');
       if (chosenDir != ''){
           dirDisplay.text = chosenDir;
           
       }
    });
    dialog.exec();
});
ChooseDirectoryDialog.testPromptAction.text = "ChooseDirectory";

tiled.extendMenu("Edit", [
    { action: "ChooseDirectory", before: "SelectAll" },
    { separator: true }
]);

image

I did notice just now, if you have one of these file / directory dialogs up and then you reload scripts, Tiled seems to crash. I ran via command line to see if I could see a message and just got Segmentation fault. Not sure how to prevent that though. I think it could be from the file dialogs sticking around, then when they close due to the scripting reload, we can't return the chosen path since all the scripting state was reinitialized.

@eishiya
Copy link
Contributor

eishiya commented Jul 10, 2023

There used to be a similar issue with the existing prompts, because they were trying to return to a context that no longer existed. Commit 994492e fixed it by delaying reloading until the modal windows closed, maybe there's something of use you can find in there?

@dogboydog
Copy link
Contributor Author

Thanks, I added ScriptManager::ResetBlocker blocker; to the beginning of promptForDirectory locally, then reloaded scripts with 'open directory' dialog open, but Tiled still seems to crash when the dialog is canceled.

Here is where it looks to be crashing:

image

Added ResetBlocker instances to avoid script reload while a file dialog
is open.

A ResetBlocker instance was also needed in ScriptDialog::exec, to avoid
scripts reloading while a dialog is open in modal mode.
* promptForDirectory -> promptDirectory (seems more consistent)
* promptOpenMultipleFiles -> promptOpenFiles (seems less redundant)

I'm a little undecided about dropping the "Names" postfix that is used
by the Qt API. I think it might help to keep it to make it clear that
file names are returned, as opposed to some kind of file objects, but it
does also feel a little verbose.
@bjorn
Copy link
Member

bjorn commented Jul 27, 2023

but Tiled still seems to crash when the dialog is canceled.

This was due to the ScriptDialog::exec also opening a modal dialog that should block script reloads. I've pushed a fix for that.

I've done a few renames. I know @eishiya preferred "promptOpenMultipleFiles", but it really sounds too verbose to me, so I moved it back to "promptOpenFiles". I've removed the "For" in "promptForDirectory", since even though it reads fine, none of the other new "prompt" methods use "For" even though they technically could.

I almost added the "Name" postfix as used in the Qt API, but was undecided. Why did you go for promptOpenFile as opposed to promptOpenFileName?

@dogboydog
Copy link
Contributor Author

dogboydog commented Jul 27, 2023

Just my opinion, but putting "name" on the end sounds awkward to me. So that's why I didn't include it. Thanks for fixing the errors

Edit: to expand a bit, I feel like the names as is make sense as "prompt the user to open a file" where as promptOpenFileName grammatically sounds strange to me like "prompt the user to open a file name", even though the prompt itself just returns a name and does not 'open' the file in terms of getting a file pointer or similar.

@bjorn bjorn merged commit 17a56ff into mapeditor:master Jul 29, 2023
12 checks passed
@eishiya
Copy link
Contributor

eishiya commented Dec 1, 2023

The documentation for this change doesn't mention that the new methods are @since 1.10.2 xP

I wanted to make the fix myself, but even though this has been merged into master already, I couldn't find the methods in master to add it.

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.

Scripting: Provide a way to prompt user for a file or directory
3 participants