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

Check Android API level #856

Closed
wants to merge 1 commit into
base: master
from

Conversation

5 participants
@tiaanl
Contributor

tiaanl commented Mar 31, 2015

We check the API level before using constants that use API levels that your device doesn't support.

Check Android API level
We check the API level before using constants that use API levels that
your device doesn't support.
@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 1, 2015

Member

Okay, this is interesting. So far I've had the impression that you could just request flags and the API would handle the rest (including workarounds from the compatibility layer). If that's the recommended way to do it, I don't see any problem adding this.

Member

MarioLiebisch commented Apr 1, 2015

Okay, this is interesting. So far I've had the impression that you could just request flags and the API would handle the rest (including workarounds from the compatibility layer). If that's the recommended way to do it, I don't see any problem adding this.

@tiaanl

This comment has been minimized.

Show comment
Hide comment
@tiaanl

tiaanl Apr 1, 2015

Contributor

We kept seeing crashes on 4.0 devices. The error being that the FULLSCREEN constant is not valid. Then after checking the other constants, I noticed that the others also have minimum API levels, so I added this.

Not really sure if this is the "recommended" way, but it doesn't crash any more and the flags still get passed in.

Contributor

tiaanl commented Apr 1, 2015

We kept seeing crashes on 4.0 devices. The error being that the FULLSCREEN constant is not valid. Then after checking the other constants, I noticed that the others also have minimum API levels, so I added this.

Not really sure if this is the "recommended" way, but it doesn't crash any more and the flags still get passed in.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 1, 2015

Member

Okay, great. I really thought that would be handled by the compatibility layer depending on the target NDK/SDK. Doing those additional checks shouldn't hurt.

Member

MarioLiebisch commented Apr 1, 2015

Okay, great. I really thought that would be handled by the compatibility layer depending on the target NDK/SDK. Doing those additional checks shouldn't hurt.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 2, 2015

Member

Tried current master and it didn't crash on my 4.0 tablet (although I remember it crashing earlier this year; possibly related to the target API level as well). Either way, I'd give it a go for 2.3 inclusion, nothing wrong in doing some more checks.

Member

MarioLiebisch commented Apr 2, 2015

Tried current master and it didn't crash on my 4.0 tablet (although I remember it crashing earlier this year; possibly related to the target API level as well). Either way, I'd give it a go for 2.3 inclusion, nothing wrong in doing some more checks.

@MarioLiebisch MarioLiebisch added s:accepted and removed s:undecided labels Apr 2, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 2, 2015

Member

I'm not an Android expert, but as a general advice, I think you'd better be sure before complicating the code this way. Adding something to the code base "just because it may be safer" is not what we usually do in SFML; you have to know what's needed and what's not ;)

Member

LaurentGomila commented Apr 2, 2015

I'm not an Android expert, but as a general advice, I think you'd better be sure before complicating the code this way. Adding something to the code base "just because it may be safer" is not what we usually do in SFML; you have to know what's needed and what's not ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 2, 2015

Member

Even if it has no impact on the latest SDK/NDK levels, it will make the code compile on older versions that don't know the new constants (if that counts for you :)). Trying to do some more tests later today.

Member

MarioLiebisch commented Apr 2, 2015

Even if it has no impact on the latest SDK/NDK levels, it will make the code compile on older versions that don't know the new constants (if that counts for you :)). Trying to do some more tests later today.

@tiaanl

This comment has been minimized.

Show comment
Hide comment
@tiaanl

tiaanl Apr 2, 2015

Contributor

One thing to keep in mind is that one 4.0 device is not the same as the next 4.0 device. I also ran the original code on 4.0 without a problem, but running it on a less well known tablet caused the crash saying it couldn't find the constants. Hence the fix.

Contributor

tiaanl commented Apr 2, 2015

One thing to keep in mind is that one 4.0 device is not the same as the next 4.0 device. I also ran the original code on 4.0 without a problem, but running it on a less well known tablet caused the crash saying it couldn't find the constants. Hence the fix.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 2, 2015

Member

Yeah, the whole API level stuff is kind of a mess unfortunately. Maybe some custom dashboard or similar acting up as well?

Member

MarioLiebisch commented Apr 2, 2015

Yeah, the whole API level stuff is kind of a mess unfortunately. Maybe some custom dashboard or similar acting up as well?

@tiaanl

This comment has been minimized.

Show comment
Hide comment
@tiaanl

tiaanl Apr 15, 2015

Contributor

We still can't run on some 4.0 devices without this fix applied. Any more feedback/testing required before a merge?

Contributor

tiaanl commented Apr 15, 2015

We still can't run on some 4.0 devices without this fix applied. Any more feedback/testing required before a merge?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Apr 15, 2015

Member

Yeah, think the PR is fine and gets a clear 👍 from me. It adds some complexity, but this also ensures SFML can be used with API levels lower than 19. Otherwise you'd be missing the proper definitions. Considering the code only runs when switching to fullscreen, I don't see any issues with the added overhead either.

Member

MarioLiebisch commented Apr 15, 2015

Yeah, think the PR is fine and gets a clear 👍 from me. It adds some complexity, but this also ensures SFML can be used with API levels lower than 19. Otherwise you'd be missing the proper definitions. Considering the code only runs when switching to fullscreen, I don't see any issues with the added overhead either.

@MarioLiebisch MarioLiebisch added this to the 2.3.1 milestone May 12, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 12, 2015

Member

This PR has been added to my merge list, meaning it will be merged into 2.3.x soon, unless someone raises any concerns.

Member

eXpl0it3r commented May 12, 2015

This PR has been added to my merge list, meaning it will be merged into 2.3.x soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 12, 2015

Member

Since this is a feature and even adds a new function to the API, it can't be merged into 2.3.x.
But I moved it to the 2.4 merge list.

Member

eXpl0it3r commented May 12, 2015

Since this is a feature and even adds a new function to the API, it can't be merged into 2.3.x.
But I moved it to the 2.4 merge list.

@eXpl0it3r eXpl0it3r modified the milestones: 2.4, 2.3.1 May 12, 2015

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 12, 2015

Member

It doesn't? It's essentially a bugfix. The version retrieval is just a helper that isn't exported, so the API doesn't change (it's even likely to be optimized out I guess).

Member

MarioLiebisch commented May 12, 2015

It doesn't? It's essentially a bugfix. The version retrieval is just a helper that isn't exported, so the API doesn't change (it's even likely to be optimized out I guess).

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 12, 2015

Member

Like @MarioLiebisch said, this is a function that is local to the translation unit it is defined in, not even having an internal header definition. It will never be seen by anything outside the TU, so it is not allowed to have any effects on the API/ABI.

Member

binary1248 commented May 12, 2015

Like @MarioLiebisch said, this is a function that is local to the translation unit it is defined in, not even having an internal header definition. It will never be seen by anything outside the TU, so it is not allowed to have any effects on the API/ABI.

@eXpl0it3r eXpl0it3r modified the milestones: 2.3.1, 2.4 May 18, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 18, 2015

Member

Well then: This PR has been added to my merge list, meaning it will be merged into 2.3.x soon, unless someone raises any concerns.

Member

eXpl0it3r commented May 18, 2015

Well then: This PR has been added to my merge list, meaning it will be merged into 2.3.x soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 22, 2015

Member

Merged in d2adccf on branch 2.3.x

Member

eXpl0it3r commented May 22, 2015

Merged in d2adccf on branch 2.3.x

@eXpl0it3r eXpl0it3r closed this May 22, 2015

@eXpl0it3r eXpl0it3r added this to Done in Android Backlog Jan 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment