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

Streamline the project import workflow. #51478

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

starry-abyss
Copy link
Contributor

@starry-abyss starry-abyss commented Aug 10, 2021

The goal of this PR is to ease the steps required to import a project.

Screenshot 2023-07-30 at 17 17 55

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:

  1. I had to still show the original dialog together in the background of file selection dialog, because otherwise the content of file selection dialog isn't rendered. The drawback is that when hitting Cancel in the file selection dialog, we see the original dialog, while ideally it shouldn't be shown on the first Cancel after clicking Import.

Maybe we should change the hierarchy of the dialogs to be siblings?

  1. 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.

  2. 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:

// if not already shown
show_dialog();

Relaled: godotengine/godot-proposals#2483

@AaronRecord
Copy link
Contributor

Closes godotengine/godot-proposals#2483

scene/gui/file_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager.cpp Outdated Show resolved Hide resolved
scene/gui/file_dialog.cpp Outdated Show resolved Hide resolved
scene/gui/file_dialog.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Dec 10, 2021

@LightningAA I linked the proposal, but are you sure the PR fully solves it? Importing a project also shouldn't force you to edit it - this part isn't addressed here.

@AaronRecord
Copy link
Contributor

@LightningAA I linked the proposal, but are you sure the PR fully solves it? Importing a project also shouldn't force you to edit it - this part isn't addressed here.

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.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023
@Calinou
Copy link
Member

Calinou commented Jul 28, 2023

The feature itself looks good.

@starry-abyss Would you be up for rebasing this PR on the latest master branch? If you don't have time, let us know and another contributor can take a look at it 🙂

@starry-abyss
Copy link
Contributor Author

@Calinou Thank you for visiting, I'm going to do it myself!

@starry-abyss
Copy link
Contributor Author

Rebased, updated the original post.

editor/project_manager.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga requested a review from Calinou August 28, 2023 10:08
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 28, 2023
Copy link
Member

@Calinou Calinou left a 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.

@akien-mga akien-mga merged commit 31cfa60 into godotengine:master Aug 29, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks for your contribution, and for keeping up with a long and slow review cycle.

@AThousandShips
Copy link
Member

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("");
Copy link
Member

@AThousandShips AThousandShips Sep 1, 2023

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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 :)

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

Successfully merging this pull request may close these issues.

7 participants