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

Revert finalizing GodotHost#getCommandLine() public API #102669

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Feb 10, 2025

Fix API regression issue reported in GodotVR/godot_openxr_vendors#264

@m4gr3d m4gr3d added the bug label Feb 10, 2025
@m4gr3d m4gr3d added this to the 4.4 milestone Feb 10, 2025
@m4gr3d m4gr3d requested a review from a team as a code owner February 10, 2025 18:20
@m4gr3d m4gr3d requested a review from dsnopek February 10, 2025 18:20
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Feb 10, 2025

@dsnopek Note that I'm keeping the update to DeviceUtils#isNativeXRDevice(context: Context) given the recency of the api, so we'll still need to do one update for the vendors plugin.

@AThousandShips

This comment was marked as resolved.

@m4gr3d m4gr3d changed the title Address API regression from https://github.com/godotengine/godot/pull/101050 Revert finalizing GodotHost#getCommandLine() public API Feb 10, 2025
@m4gr3d m4gr3d force-pushed the fix_method_signature_change branch from fe228c9 to d6c8c02 Compare February 11, 2025 16:00
@dsnopek
Copy link
Contributor

dsnopek commented Feb 11, 2025

@m4gr3d This still leaves the return type of getCommandLine() changing from MutableList<String> to ArrayList<String>. Do we want that?

If our goal is that we don't want Android plugins written for Godot 4.3 to require source changes to compile, then we should probably revert the change to the return type as well.

@dsnopek
Copy link
Contributor

dsnopek commented Feb 11, 2025

When building godot_openxr_vendors against these changes, I'm also getting an error about MissingSuperCall that I had to @Suppress(...) which would be another source change.

UPDATE: You can see all the changes needed to godot_openxr_vendors in PR GodotVR/godot_openxr_vendors#266

@m4gr3d m4gr3d force-pushed the fix_method_signature_change branch from d6c8c02 to 2a66335 Compare February 11, 2025 18:20
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Feb 11, 2025

@dsnopek I've addressed your feedback!

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! The latest version works great in my testing

@akien-mga akien-mga merged commit 8d909f8 into godotengine:master Feb 11, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@m4gr3d m4gr3d deleted the fix_method_signature_change branch February 12, 2025 13:59
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.

4 participants