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

[3.x] Restrict the project data directory configuration #53779

Merged

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Oct 13, 2021

Limits the configuration range for the project data directory.

@@ -1990,7 +1990,8 @@ Error EditorFileSystem::_resource_import(const String &p_path) {
}

bool EditorFileSystem::_should_skip_directory(const String &p_path) {
if (p_path.begins_with(ProjectSettings::get_singleton()->get_project_data_path())) {
String project_data_path = ProjectSettings::get_singleton()->get_project_data_path();
if (p_path == project_data_path || p_path.begins_with(project_data_path + "/")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You were right about == problem here and begins_with with concatenated "/" should cover what we wanted to achieve, no need to do both (or I'm missing something here again, I've just tried debugging it and p_path always includes slash at the end).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to cover the case where p_path didn't include slash at the end.
If it's guaranteed to always include a / at the end, I can remove the == check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it's possible for the p_path to not have a trailing slash when this function is invoked.
See

if (EditorFileSystem::_should_skip_directory(cur_dir + dir)) {
for the case where the cur_dir is the root directory (e.g: res://).

editor/find_in_files.cpp Outdated Show resolved Hide resolved
@Listwon
Copy link
Contributor

Listwon commented Oct 13, 2021

Please also review EditorFileDialog::_item_list_item_rmb_selected change you've made previously as
if (item_meta["path"] == "res://.import") { (which I assume worked fine before)
was changed to
if (item_meta["path"] == ProjectSettings::get_singleton()->get_project_data_path()) { (looks reasonably)
and then to
if (String(item_meta["path"]).begins_with(ProjectSettings::get_singleton()->get_project_data_path())) { (not sure why)

The MRP covering edge cases would be appreciated.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 13, 2021

Please also review EditorFileDialog::_item_list_item_rmb_selected change you've made previously as if (item_meta["path"] == "res://.import") { (which I assume worked fine before) was changed to if (item_meta["path"] == ProjectSettings::get_singleton()->get_project_data_path()) { (looks reasonably) and then to if (String(item_meta["path"]).begins_with(ProjectSettings::get_singleton()->get_project_data_path())) { (not sure why)

The MRP covering edge cases would be appreciated.

This change was made in master prior to my changes. I merely update the logic to follow suit.
See

if (String(item_meta["path"]).begins_with(ProjectSettings::get_singleton()->get_project_data_path())) {
(the previous logic was String(item_meta["path"]).begins_with("res://.godot").

@m4gr3d m4gr3d force-pushed the restrict_project_data_dir_config_3x branch from e14ee7f to 2b6678c Compare October 13, 2021 20:56
@akien-mga akien-mga merged commit 0fb135d into godotengine:3.x Oct 18, 2021
@akien-mga
Copy link
Member

Thanks!

@m4gr3d m4gr3d deleted the restrict_project_data_dir_config_3x branch October 18, 2021 21:01
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.

3 participants