Skip to content

Add OS::get_cwd virtual #87789

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

L2750558108
Copy link
Contributor

@L2750558108 L2750558108 commented Jan 31, 2024

It seems that we get current working directory by creating a DirAccess without path init and use get_current_dir(). It doesn't look intuitive.
In fact, the DirAccess finally calls the cwd getting function of specific platform. This PR move them to the OS::get_cwd virtual.

@L2750558108 L2750558108 requested review from a team as code owners January 31, 2024 15:54
@Calinou Calinou added this to the 4.x milestone Jan 31, 2024
@Calinou
Copy link
Member

Calinou commented Jan 31, 2024

Please generate documentation by running the compiled editor with --doctool, then fill in the changes made to doc/classes/OS.xml.

@L2750558108
Copy link
Contributor Author

Please generate documentation by running the compiled editor with --doctool, then fill in the changes made to doc/classes/OS.xml.

Do functions that are not registered in the bind_method also need it?

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Also take a look at our code style guidelines, there are some issues with it.

Comment on lines 181 to +182
virtual Error set_cwd(const String &p_cwd);
virtual String get_cwd() const;
Copy link
Member

Choose a reason for hiding this comment

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

Note: neither set_cwd, not newly added get_cwd are bound, and therefore it will be accessible only from the engine C++ code (not from GDScript or GDExtension). If you want't it to be accessible, a wrapper should be added to the CoreBind (OS is a special case and its methods can't be bound directly).

Here's links to one of such wrappers, for example (OS.get_name):

Wrapper definition:

String get_name() const;

Wrapper implementation:

godot/core/core_bind.cpp

Lines 351 to 353 in 9adb7c7

String OS::get_name() const {
return ::OS::get_singleton()->get_name();
}

Wrapper binding:

ClassDB::bind_method(D_METHOD("get_name"), &OS::get_name);

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 noticed that set_cwd is not bound, so I don't plan to get_cwd the corresponding one.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's the purpose of adding get_cwd() if it can't be used from script and not used by the engine.

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 hope it will provide an intuitive way to get the current working directory in future engine development, and it will correspond to set_cwd.
Maybe it could also be used to gradually replace the original code that used DirAccess to get the working path?

@L2750558108 L2750558108 requested a review from a team as a code owner March 22, 2025 16:30
@L2750558108 L2750558108 deleted the Add-get_cwd branch March 22, 2025 16:35
@L2750558108 L2750558108 restored the Add-get_cwd branch March 22, 2025 16:35
@L2750558108 L2750558108 reopened this Mar 22, 2025
@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Mar 22, 2025

Can OS::get_current_working_directory be a better alternative naming-wise? Or is cwd a common knowledge

@L2750558108
Copy link
Contributor Author

Can OS::get_current_working_directory be a better alternative naming-wise? Or is cwd a common knowledge

The name comes from another set_cwd method that already exists. I'm not sure if this should be changed

@Delsin-Yu
Copy link
Contributor

I'm not sure if this should be changed

Oh, then no.

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.

6 participants