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

Add autocompletion for OS.has_feature() #86747

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

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 3, 2024

Closes godotengine/godot-proposals#4240.

image

The implementation borrows from ProjectSettingsEditor, putting into EditorExport for OS to access.

But this causes a dependency on the editor class...

core/core_bind.cpp Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

I was answering back but the comment was deleted.
It's like this because it's a copy-paste from ProjectSettingsEditor. There's no real place where these are "stored", quoting Vnen:

This should be easy to add, but I don't think the list of features is available anywhere. So either it should be hardcoded into GDScript auto-completion or we need add a central list of possible features in the editor to harness for the completion list.

Maybe and ideally, the two need to be united somehow.

@AThousandShips
Copy link
Member

Indeed, noticed that it was just as I had commented, my bad 🙂

I'd say to either unite it with the other one, or use the list directly to avoid the step

@Mickeon Mickeon force-pushed the autocompletion-os-has-feature branch from d5bc8a5 to 92c0fee Compare January 3, 2024 16:40
@Mickeon Mickeon marked this pull request as ready for review January 3, 2024 16:40
@Mickeon Mickeon requested a review from a team as a code owner January 3, 2024 16:40
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

I think I came with a nice compromise, but the OS needs to know about EditorExport which is still not nice, so perhaps as a last resort I would have to hardcode it like #76591 has done, into gdscript_editor.cpp. I don't find it to be neatly organized, but it would not be... dependency-d... dependent?

core/core_bind.cpp Show resolved Hide resolved
core/core_bind.cpp Show resolved Hide resolved
@Mickeon Mickeon force-pushed the autocompletion-os-has-feature branch 2 times, most recently from 6a37e25 to 4bf1fc5 Compare January 3, 2024 16:57
@Mickeon Mickeon force-pushed the autocompletion-os-has-feature branch from 4bf1fc5 to 89b48b8 Compare January 3, 2024 17:00
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

Thank you. It'll probably compile now, but we'll then see what are the thoughts on code quality

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.

Add autocompletion for OS.has_feature()'s parameter
2 participants