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

[BUG] android_native_app_glue APP_CMD_WINDOW_RESIZED / APP_CMD_WINDOW_REDRAW_NEEDED / APP_CMD_CONTENT_RECT_CHANGED #1139

Closed
tsuoranta opened this issue Nov 26, 2019 · 10 comments
Assignees
Labels
Milestone

Comments

@tsuoranta
Copy link

Description

android_native_app_glue.h has a few APP_CMDs which are never created by android_native_app_glue.c:

  • APP_CMD_WINDOW_RESIZED
  • APP_CMD_WINDOW_REDRAW_NEEDED
  • APP_CMD_CONTENT_RECT_CHANGED

Looking at base/core/jni/android_app_NativeActivity.cpp and base/core/java/android/app/NativeActivity.java it seems callbacks such as code->callbacks.onNativeWindowResized would be called, if android_native_app_glue.c would bother to set up such callback function.

How should I deal with these APP_CMDs? I could use modified android_native_app_glue and wire them. Or is it safe to ignore these?

Environment Details

  • NDK Version: 20.1.5948944
  • Build system: gradlew - see ndk_samples
  • Host OS: Linux, Ubuntu 19.10
  • ABI:
  • NDK API level: platformVersion = 12, compileSdkVersion 28, targetSdkVersion 28
  • Device API level: N/A
@tsuoranta tsuoranta added the bug label Nov 26, 2019
@DanAlbert DanAlbert added this to the r22 milestone Dec 2, 2019
@enh-google
Copy link
Collaborator

oops. i can't explain that from the history --- onContentRectChanged, for example, has been there since 2012.

I could use modified android_native_app_glue and wire them.

yes, feel free to do that.

i don't know what @DanAlbert thinks, but i'm torn about fixing this. it's obviously wrong, but i'm wary of suddenly sending folks messages they didn't receive in the past.

@DanAlbert
Copy link
Member

but i'm wary of suddenly sending folks messages they didn't receive in the past.

Can we do this in a targetSdkVersion gated manner?

@enh-google
Copy link
Collaborator

no, that's the kicker --- all of this stuff is actually ancient, it's just a bug that it's still not wired up here in the NDK glue. (so although we could make it depend on targetSdkVersion, but that would be weird and mostly pointless. we may as well just say "yeah, fork your own copy".)

given that you'd have to rebuild your app with a newer NDK to start seeing this stuff work like the docs claim it should work, maybe it's okay to change?

@enh-google
Copy link
Collaborator

dan explained in person that what he meant was checking target sdk version at run-time, which makes this a run-time behavior difference (of the kind that app developers expect when incrementing target sdk version) rather than a build-time difference ("because i used a newer NDK") which app developers expect to manifest as a compile-time difference (aka build break :-) ).

anyway, https://android-review.googlesource.com/c/platform/ndk/+/1180321 is a possible implementation (without the run-time check). i still need to find the time to actually work that into a sample app and test it properly though...

@DanAlbert
Copy link
Member

@tsuoranta any thoughts on whether we should gate the new behavior behind targetSdkVersion, or whether using a newer NDK is a sufficient enough opt-in barrier?

@magestik
Copy link

@DanAlbert I think the proposed fix is fine and adding run-time check would make the fix harder to get and add more confusion. We all know that we should thoroughly test our apps after upgrading the NDK anyway.

For what it’s worth I've been using this workaround successfully :

void android_main(struct android_app* state)
{
	state->onAppCmd = handle_cmd;
	state->onInputEvent = handle_input;

	state->activity->callbacks->onContentRectChanged = myOnContentRectChanged;

	...
}

@DanAlbert
Copy link
Member

SGTM. Thanks for the feedback.

@Andreyogld3d
Copy link

@DanAlbert, will be this fix included in NDK 21 ?

@DanAlbert
Copy link
Member

No. The sidebar on the right shows which release a bug is triaged for, btw.

@enh-google
Copy link
Collaborator

https://android-review.googlesource.com/c/platform/ndk/+/1180321 merged for r22.

disigma pushed a commit to wimal-build/ndk that referenced this issue Dec 18, 2019
Also reduce duplication very slightly.

Bug: android/ndk#1139
Test: builds
Test: https://github.com/android/ndk-samples/tree/master/native-activity
Change-Id: I6658af95828f0532232616d621a4883d538c4c76
gongminmin pushed a commit to gongminmin/android_native_app_glue that referenced this issue Apr 13, 2020
Also reduce duplication very slightly.

Bug: android/ndk#1139
Test: builds
Test: https://github.com/android/ndk-samples/tree/master/native-activity
Change-Id: I6658af95828f0532232616d621a4883d538c4c76
pazos added a commit to pazos/android-luajit-launcher that referenced this issue Oct 19, 2020
…tform/ndk/+/1180321

some APP_CMDs callbacks were never triggered: affects
APP_CMD_WINDOW_RESIZED / APP_CMD_WINDOW_REDRAW_NEEDED / APP_CMD_CONTENT_RECT_CHANGED

See android/ndk#1139
pazos added a commit to pazos/android-luajit-launcher that referenced this issue Oct 24, 2020
…tform/ndk/+/1180321

some APP_CMDs callbacks were never triggered:
APP_CMD_WINDOW_RESIZED / APP_CMD_WINDOW_REDRAW_NEEDED / APP_CMD_CONTENT_RECT_CHANGED

See android/ndk#1139
pazos added a commit to koreader/android-luajit-launcher that referenced this issue Nov 3, 2020
…tform/ndk/+/1180321

some APP_CMDs callbacks were never triggered:
APP_CMD_WINDOW_RESIZED / APP_CMD_WINDOW_REDRAW_NEEDED / APP_CMD_CONTENT_RECT_CHANGED

See android/ndk#1139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants