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

Fix get_base_dir windows top level directory logic #52049

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

theraot
Copy link
Contributor

@theraot theraot commented Aug 24, 2021

This is a fix for #52048

If we look at the diff for cddff04 we observe that before it was looking for a 3 character string ("://"), and now it is looking for a 2 character string (either ":/" or ":\\").

Edit: Updating the use of substr accordingly fixes windows top level directories (e.g. "C:\\puff") but breaks Godot paths (e.g. "res://puff"). Also, it is worth noting that the code already has an special case for the unix root directory ("/"). Thus, we need to handle:

  • Finding "://" (3 characters).
  • Finding ":\ (2 characters).
  • Finding ":/" (2 characters).
  • "/" at the start (1 character).

And we need to be careful with order in which we check because "://" contains ":/".


This code demonstrate the change:

func test(s:String):
	print("\"", s, "\" -> \"", s.get_base_dir(), "\"")

func _ready() -> void:
	test("C:")
	test("C:\\")
	test("C:\\puff")
	test("C:\\puff\\")

Before cddff04 (tested in Godot 3.2.3) the output is:

"C:" -> ""
"C:\" -> "C:"
"C:\puff" -> "C:"
"C:\puff\" -> "C:\puff"

After cddff04 (tested in Godot 3.3.3) the output is:

"C:" -> ""
"C:\" -> "C:\"
"C:\puff" -> "C:\p"
"C:\puff\" -> "C:\puff"

With this PR (tested in custom build of Godot 4.0) the output is:

"C:" -> ""
"C:\" -> "C:\"
"C:\puff" -> "C:\"
"C:\puff\" -> "C:\puff"

Bugsquad edit: Fixes #52048.

@theraot theraot requested a review from a team as a code owner August 24, 2021 03:36
@Calinou Calinou added bug platform:windows topic:core cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Aug 24, 2021
@Calinou Calinou added this to the 4.0 milestone Aug 24, 2021
@theraot theraot changed the title Fix get_base_dir windows top level directory logic [WIP] Fix get_base_dir windows top level directory logic Aug 24, 2021
@theraot
Copy link
Contributor Author

theraot commented Aug 24, 2021

I think this pull request at the time of writing introduces a new bug: "res://".get_base_dir() return "res:/". I'll address this. Edit: Done.

@theraot theraot force-pushed the master branch 2 times, most recently from e744b13 to 62c9a82 Compare August 24, 2021 08:26
The new tests cover:
- A file with empty extension.
- A file with only extension (a "hidden" file, unix style).
- A file directly at the windows top level directory.
- A file directly at the unix root directory.
- A file directly at the res:// base directory.
@theraot theraot requested a review from a team as a code owner August 24, 2021 09:26
@theraot theraot changed the title [WIP] Fix get_base_dir windows top level directory logic Fix get_base_dir windows top level directory logic Aug 24, 2021
@theraot
Copy link
Contributor Author

theraot commented Aug 24, 2021

Added new test cases. They pass on my machine.

@akien-mga akien-mga merged commit 41562b9 into godotengine:master Sep 13, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@theraot
Copy link
Contributor Author

theraot commented Sep 16, 2021

I opened a pull request for the 3.x branch (I had not noticed the file ustring.cpp was in a different path, which prevent cherrypicking it).

@akien-mga
Copy link
Member

I had not noticed the file ustring.cpp was in a different path, which prevent cherrypicking it

FYI, Git can handle file renames fine when cherry-picking, as long as the files are still sensibly similar:

$ git cherry-pick -x ef54d35
Performing inexact rename detection: 100% (3035668/3035668), done.
Auto-merging core/ustring.cpp
[3.x 1755591539] Fix get_base_dir windows top level directory logic
 Author: Theraot <Theraot@gmail.com>
 Date: Mon Aug 23 22:32:13 2021 -0500
 1 file changed, 27 insertions(+), 11 deletions(-)

But now that you made a PR, let's use that.

@akien-mga akien-mga removed cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.3 labels Sep 16, 2021
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.

get_base_dir returns wrong path with top level directories on windows
3 participants