-
Notifications
You must be signed in to change notification settings - Fork 13
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
Support Android Nougat #5 #6
Conversation
Thanks, @iainalba . I would have done a PR, but I haven't had much response with my previous ones ;) Also, note the link provided above appears to be broken. |
I have used the above fix and it started working on Android N but now it crashes on Android M. The error is same as before but now it's on Android M. |
Yeh I realised I made a stupid mistake, but then never pushed my changes. I'll push them now. |
Thank you for the prompt reply. Can you update once you push changes? Thanks |
@iainalba The latest committed changes work. Thank you. |
Hi @jhansche, are there any future plans to merge this PR and doing a new release of the library? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the compileSdkVersion should be updated, so that you can use Build.VERSION_CODES.N
instead of a hard-coded 24
@@ -43,10 +43,8 @@ | |||
private static final FontManagerImpl IMPL; | |||
|
|||
static { | |||
// TODO: SDK_INT == VERSION_CODES.N | |||
if (Build.VERSION.SDK_INT == Build.VERSION_CODES.M && Build.VERSION.PREVIEW_SDK_INT > 0) { | |||
if (Build.VERSION.SDK_INT >= 24) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use Build.VERSION_CODES.N, by updating compileSdkVersion to 24+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, IIRC, there were differences even within the Nougat preview releases. It's been a while, but I thought I had to make changes as newer releases were coming out.
Granted, those were previews, and ideally should not be shipped on production devices, so arguably not worth digging too far into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with using version codes provided by framework and not worrying about preview releases. I already pulled in the source and did a fix for this to support a project I was on last year. I'm supposed to jump back on that project some time next week and can update this. But hopefully @iainalba can get around to it before that.
Thanks for getting back to us @jhansche !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward a solution here :)
Sorry all for not getting back to these... I don't recall seeing any notifications for the existing issues and PRs, until this morning. 😬 As for a new release @rogercotrina, I do think it makes sense to do a new release when these are merged. I'll just have to remember how to do the release 😕 |
Someone accept this merge for the love of God! |
@jhansche can this be merged if everything is fine? Thanks. Also need a new release after that. |
Thanks. Need a new release for it. @jhansche |
As described in this issue:
#5
the FontManager was only including Android N Preview versions. As a result a Font Manager Implementation class was not being assigned to
IMPL
when running with Android N. Therefore, the logic was altered to allow Android versions>=
Marshmallow to useFontManagerImpl24()
.