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

Don't invoke adb with no runnable Android preset #87823

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Feb 1, 2024

Android export plugin will periodically call adb.exe to check for available devices. It does this constantly while the editor is running, regardless if the project is targeted for Android or not. Performance implications aside, this check will sometimes fail and spit an error that reminds you that it runs in the background.

This PR tweaks this behavior. The adb will be called only if the project has an export preset for Android and it's marked runnable (AFAIK it's a requirement to run something on a device). I added a new signal that gets emitted when adding/removing preset and toggling Runnable.

@KoBeWi KoBeWi added this to the 4.x milestone Feb 1, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner February 1, 2024 15:06
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
Comment on lines 118 to 119
ADD_SIGNAL(MethodInfo(_export_presets_updated));
ADD_SIGNAL(MethodInfo(_export_presets_changed));
Copy link
Member

Choose a reason for hiding this comment

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

The names are quite similar, it's a bit hard to know at a glance which one means what.

Copy link
Member Author

@KoBeWi KoBeWi Feb 1, 2024

Choose a reason for hiding this comment

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

I wasn't sure how to name the new signal. It could be export_preset_list_changed, but that doesn't cover runnable change.
Would it be enough if I added a comment somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use _export_presets_updated to signal both an addition to the list and an update to the runnable state?

I don't think we need to introduce a new signal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to change what this signal does, but sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait no, the reason why I didn't use the old signal is that it's emitted too often for my purposes. There is no reason to run the update code on unrelated changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait no, the reason why I didn't use the old signal is that it's emitted too often for my purposes. There is no reason to run the update code on unrelated changes.

I think it may be a compromise worth making; after all, it'll end up being called an order of magnitude less than it's being invoked in the current codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you could create a new signal specifically for the runnable update.

@bruvzg
Copy link
Member

bruvzg commented Feb 1, 2024

The same is true for iOS, so it should have the same change as well.

Comment on lines 118 to 119
ADD_SIGNAL(MethodInfo(_export_presets_updated));
ADD_SIGNAL(MethodInfo(_export_presets_changed));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use _export_presets_updated to signal both an addition to the list and an update to the runnable state?

I don't think we need to introduce a new signal.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 1, 2024

Did similar change to iOS, but I didn't test it.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 15, 2024

Renamed the signal to export_presets_runnable_updated.

editor/export/editor_export.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 15, 2024
@akien-mga akien-mga merged commit e697774 into godotengine:master Feb 17, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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