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

Ignore trailing slashes in new project’s path when disabling Create Folder #94015

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

Grublady
Copy link
Contributor

@Grublady Grublady commented Jul 6, 2024

In the “Create New Project” dialog, disabling the “Create Folder” slider with a trailing slash in the project path would cause it to incorrectly identify the last path component as the substring following the slash, in other words, an empty string.

By first discarding any trailing slashes when disabling the slider, the project dialog will now identify and pop the last path component as usual even when a trailing slash is present.

Fixes #93973

@AThousandShips
Copy link
Member

Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@Grublady
Copy link
Contributor Author

Grublady commented Jul 7, 2024

Apologies! I have added the commit header email address to my GitHub account, please let me know if there are any other issues

Comment on lines 319 to 321
while (target_path.ends_with("/") || target_path.ends_with("\\")) {
target_path = target_path.substr(0, target_path.length() - 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while (target_path.ends_with("/") || target_path.ends_with("\\")) {
target_path = target_path.substr(0, target_path.length() - 1);
}
target_path = target_path.rstrip("/");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, much nicer! Any reason not to include backslash?

Suggested change
while (target_path.ends_with("/") || target_path.ends_with("\\")) {
target_path = target_path.substr(0, target_path.length() - 1);
}
target_path = target_path.rstrip("/\\");

Copy link
Member

Choose a reason for hiding this comment

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

rstrip replaces all occurrences of a pattern from the right side, so it would only replace it if it ends with /\ with the proposed change.
If you want to strip both, you have to do them separately: target_path.rstrip("/").rstrip("\\")

Godot shouldn't be using backslashes in paths though, but I guess users might input them manually / copy from the Windows explorer, so that makes sense.

@@ -315,6 +315,8 @@ void ProjectDialog::_create_dir_toggled(bool p_pressed) {
target_path = target_path.path_join(last_custom_target_dir);
}
} else {
// Strip any trailing slash.
target_path = target_path.rstrip("/").rstrip("\\")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't a semi-colon ; needed at the end of the line?

Suggested change
target_path = target_path.rstrip("/").rstrip("\\")
target_path = target_path.rstrip("/").rstrip("\\");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely 😅 I can't believe I missed that

@Grublady
Copy link
Contributor Author

Grublady commented Jul 8, 2024

Amended with the suggested change, however I'm still uncertain whether it requires one or two calls to rstrip

On my machine it seems to successfully strip any combination of forward & back slashes with just target_path = target_path.rstrip("/\\");, but since two calls was suggested I would like to confirm this behavior.

If this is not intended it may be another issue and/or documentation discrepancy.

@akien-mga
Copy link
Member

Amended with the suggested change, however I'm still uncertain whether it requires one or two calls to rstrip

On my machine it seems to successfully strip any combination of forward & back slashes with just target_path = target_path.rstrip("/\\");, but since two calls was suggested I would like to confirm this behavior.

If this is not intended it may be another issue and/or documentation discrepancy.

Hm you are right, I commented from memory but indeed rstrip replaces any of the chars found in the string, so your suggestion was correct (and more efficient).

…older

In the “Create New Project” dialog, disabling the “Create Folder” slider with a trailing slash in the project path would cause it to incorrectly identify the last path component as the substring following the slash, in other words, an empty string.

By first discarding any trailing slashes when disabling the slider, the project dialog will now identify and pop the last path component as usual even when a trailing slash is present.
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, it works as expected. Creating a project when the project path has a trailing slash also works (it's in the correct newly created folder).

Before

project_manager_create_slash_before.mp4

After

project_manager_create_slash_after.mp4

@akien-mga akien-mga merged commit cead80e into godotengine:master Jul 11, 2024
18 checks passed
@akien-mga
Copy link
Member

akien-mga commented Jul 11, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

@Grublady Grublady deleted the strip_slash branch August 11, 2024 16:43
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.

Create new project with default "Create folder" selected, not working in Godot 4.3.*
5 participants