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

Partially fix directory bug on Android #96711

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

shahriarlabib000
Copy link
Contributor

@shahriarlabib000 shahriarlabib000 commented Sep 8, 2024

Similar fix like #95086 for _browse_install_path() also updated _browse_project_path to account for local path.

But I couldn't fix for a directory that doesn't exist.

For example something like /storage/emulated/0/Documents/folderThatDoesntExist will still cause the same similar issue as mentioned in #94570

@Chaosus Chaosus added this to the 4.4 milestone Sep 8, 2024
@fire fire changed the title partially fix directory bug on Android Partially fix directory bug on Android Sep 8, 2024
@fire fire requested review from m4gr3d and a team September 8, 2024 16:53
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good enough to me to address the described issue.

Reiterating I'm not very familiar with the logic in project_dialog.cpp, but from #95086 and this fix, it seems we may need to revisit how its path validation is done.

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 8, 2024
@akien-mga akien-mga requested a review from KoBeWi September 8, 2024 21:19
@shahriarlabib000
Copy link
Contributor Author

I'd like to add that this bug exist in the 4.2.0 as well (at least on my device Redmi 10 2022)

@KoBeWi
Copy link
Member

KoBeWi commented Sep 9, 2024

When can this bug appear? It's weird that the path would be relative, unless it's some bug related only to Android? Like the path is dependent on system version or something. In #94570 it looks like the default should be /.

@shahriarlabib000
Copy link
Contributor Author

This bug appears when importing from a ZIP file. relative path would be provided by the user.
https://youtu.be/_GNJwqWBsy0?si=Kc-jRdIaQ2Rq8JNN

@KoBeWi
Copy link
Member

KoBeWi commented Sep 9, 2024

Yeah can't reproduce on Windows.
Although in your video the path changes to /, which is absolute. Does your fix handle it properly?

@shahriarlabib000
Copy link
Contributor Author

I don't think I quite understand what you are saying.But i think its working
https://youtu.be/fut4N1qYeMM?si=VtS2ruvSqsyNpBa_
Although it doesn't work for a directory that doesn't exist hence the partial 'fix'
Maybe it could also be handled if we did something like dirAccess->dir_exists() check?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 9, 2024

Maybe it could also be handled if we did something like dirAccess->dir_exists() check?

That could work. You can use the dir_exists_absolute static method.

@shahriarlabib000

This comment was marked as off-topic.

@shahriarlabib000 shahriarlabib000 force-pushed the fileNotFound branch 2 times, most recently from e88f409 to 9b538d6 Compare September 10, 2024 04:07
@shahriarlabib000
Copy link
Contributor Author

Updated
only if(!DirAccess::dir_exists_absolute(path)) didn't seem to work had to write
if(path.is_relative_path() || !DirAccess::dir_exists_absolute(path))

@KoBeWi
Copy link
Member

KoBeWi commented Sep 10, 2024

Sooo when importing ZIP file, the installation path is auto-filled with the ZIP-name, which is a new folder, which means it never exists. Clicking Browse will always reset path. Not sure what's the expected behavior tbh.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I can't reproduce the problem (didn't test on Android though), but the fix seems harmless.

@shahriarlabib000
Copy link
Contributor Author

shahriarlabib000 commented Sep 10, 2024

Even tho it is similar issue as #94570 but the cause is different. This happens due to incorrect user input

@akien-mga akien-mga merged commit 63021b0 into godotengine:master Sep 11, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@shahriarlabib000 shahriarlabib000 deleted the fileNotFound branch September 11, 2024 11:17
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

5 participants