-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
Streamline the project import workflow. #51478
Streamline the project import workflow. #51478
Conversation
@LightningAA I linked the proposal, but are you sure the PR fully solves it? |
Huh, I guess maybe it only partially addresses it. It probably wouldn't be too much work to add that to this PR, but it could be done in a follow-up PR as well. |
The feature itself looks good. @starry-abyss Would you be up for rebasing this PR on the latest |
@Calinou Thank you for visiting, I'm going to do it myself! |
885a3dd
to
22b16e1
Compare
Rebased, updated the original post. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
031f6de), it works as expected.
To further streamline the workflow, it would be interesting to automatically determine a target folder name when importing a ZIP file (and create a folder automatically), but that can be done in a future PR.
22b16e1
to
4b81635
Compare
Thanks for your contribution, and for keeping up with a long and slow review cycle. |
This has caused a regression, the file path is broken: |
@@ -264,17 +264,19 @@ void EditorFileDialog::update_dir() { | |||
} | |||
dir->set_text(dir_access->get_current_dir(false)); | |||
|
|||
file->set_text(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line breaks things and clears the file each time, meaning file paths provided by the user do not work, I'm unsure what this change was meant to solve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also clear the file if you change directory, which I don't think is desired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also clear the file if you change directory
It was like that before this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? If you remove that line it doesn't do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I remember testing this specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AThousandShips From what I remember this line was for clearing the file name of a file selected in one directory when moving to another one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended to work with opening files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed an alternative fix that should keep this behavior, please test the PR #81226 which fixes the broken behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, one can select a directory, a .zip file or a project.godot file for the import to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part makes sense, was just too general in this PR then :)
The goal of this PR is to ease the steps required to import a project.
After clicking Import, the file selection dialog is immediately shown (previously one needed an extra button click).
The project file selection dialog now also supports selecting a directory, similar to Scan mode. Changes to Any mode of the selection dialog are because I wanted it to offer full functionality of both File and Directory selection modes.
Notes:
Maybe we should change the hierarchy of the dialogs to be siblings?
I was going to also merge Import and Scan, but not sure about the best approach to show Scan option in the UI of Import. Maybe it's for after this PR is merged.
The lines below are because in the original PR there was no issue described in 1). If we decide to ignore 1), then I think they can be removed:
Relaled: godotengine/godot-proposals#2483