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

Support Android Nougat #5 #6

Merged
merged 4 commits into from
Jun 15, 2017
Merged

Support Android Nougat #5 #6

merged 4 commits into from
Jun 15, 2017

Conversation

iainalba
Copy link
Contributor

@iainalba iainalba commented Sep 1, 2016

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 use FontManagerImpl24().

@iainalba iainalba mentioned this pull request Sep 1, 2016
@troyjperales
Copy link
Contributor

troyjperales commented Sep 1, 2016

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.

@Aditya-Sh
Copy link

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.
java.lang.NoSuchMethodError: No virtual method addFontWeightStyle(Ljava/nio/ByteBuffer;ILjava/util/List;IZ)Z in class Landroid/graphics/FontFamily; or its super classes (declaration of 'android.graphics.FontFamily' appears in /system/framework/framework.jar)

@iainalba
Copy link
Contributor Author

iainalba commented Nov 7, 2016

Yeh I realised I made a stupid mistake, but then never pushed my changes. I'll push them now.

@Aditya-Sh
Copy link

Aditya-Sh commented Nov 7, 2016

Thank you for the prompt reply. Can you update once you push changes? Thanks

@Aditya-Sh
Copy link

@iainalba The latest committed changes work. Thank you.

@rogercotrina
Copy link

Hi @jhansche, are there any future plans to merge this PR and doing a new release of the library?

Copy link
Contributor

@jhansche jhansche left a 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) {
Copy link
Contributor

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+

Copy link
Contributor

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.

Copy link
Contributor

@troyjperales troyjperales Jan 20, 2017

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 !

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 :)

@jhansche
Copy link
Contributor

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 😕

@llleodeleon
Copy link

Someone accept this merge for the love of God!

@jaggs6
Copy link

jaggs6 commented Jun 15, 2017

@jhansche can this be merged if everything is fine? Thanks. Also need a new release after that.

@jhansche jhansche merged commit 8714ec8 into MeetMe:master Jun 15, 2017
@fangzhzh
Copy link

fangzhzh commented Jul 30, 2017

Thanks. Need a new release for it. @jhansche

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants