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

Fix Android 2.2 compatibility #124

Closed
wants to merge 1 commit into from
Closed

Fix Android 2.2 compatibility #124

wants to merge 1 commit into from

Conversation

goncalossilva
Copy link
Contributor

Android 2.2 (FroYo) doesn't support some of Arrays' functions, such as copyOf() and copyOfRange().

Version 2.3.0 worked properly on Android 2.2, but 2.3.1 broke things when it introduced the usage of Arrays.copyOf() and Arrays.copyOfRange(). I added a new class, ArraysCompat, which implements all variants of these methods. This way compatibility is ensured.

Also, I think it's OK to break compatibility with Android 2.2, as it's not massively used anymore, but I don't think it should be done on a point release (ie. from 2.3.0 to 2.3.1). Maybe in 2.4? :-)

FroYo doesn't support some of Arrays' functions, such as copyOf() and
copyOfRange(). ArraysCompat implements all variants of those methods,
adding FroYo compatibility back
@goncalossilva goncalossilva mentioned this pull request Jan 25, 2014
@goncalossilva
Copy link
Contributor Author

I should add: Most of the ArraysCompat.copyOf() and ArraysCompat.copyOfRange() variants are not used/needed. I just added them for completion. I can remove all unused variants, if you'd like. Just let me know.

@cowtowncoder
Copy link
Member

Ok. I will have to think about this. Couple of things we need to consider:

  • 'master' is now 2.4.0-SNAPSHOT, so I think change would need to go in 2.3 (to fix 2.3.2). But it might not be needed for 2.4, as you suggest.
  • I have tried to trim out number of classes, just because from jar size perspective there is high per-class overhead (since zip compression is on per-file basis). I understand why it makes sense here, but just something to know.

So perhaps what we can do is to merge this in 2.3 branch instead, send PR; and I'll send a note on Jackson dev mailing list, suggest that we do not support Android 2.2 beyond Jackson 2.3? (i.e. no need to add this for 2.4).

Does this make sense?

Thanks again for the contribution!

@cowtowncoder
Copy link
Member

Oh and yes, breakage was not planned: I wasn't aware of these missing, and since Jackson JDK baseline was raised to 1.6 for Jackson 2.3, these methods may now be used. I am also aware that Google has had lots of trouble leaving bits and pieces out without obvious reason behind it, so I am not (alas!) surprised there are such issues.

So anyway this is just agreeing in that patch versions really should not break backwards compatibility, and we should fix it for 2.3.2.

@goncalossilva
Copy link
Contributor Author

Yes, of course, makes total sense. I'll close this pull request and make a new one for the 2.3 branch.

If the dev mailing list agrees with dropping support for Android 2.2, then there's nothing left to do. If they don't agree, this commit will have to be cherry-picked to master.

@cowtowncoder
Copy link
Member

Awesome. What I suggest is actually that 2.3.2 should be fixed, and I don't expect anyone to disagree.
I rather just want to confirm that dropping support in 2.4 is ok, so that anyone who needs compatibility with older Android versions can keep using 2.3 (or earlier), but going forward we can simplify code base slightly.

I hope this makes sense.

For contribution I will also need a CLA, I'll add details to the new PR.

Thank you again; Android support is one important area where I definitely depend on help by other developers.

@goncalossilva
Copy link
Contributor Author

No problem, I'm glad to help.

I hit this problem because I (unfortunately) support 2.2 on an Android app which uses Jackson and suddenly it started crashing on 2.3.1.

Just made a new PR: #126. I hope it targets the correct branch this time.

@yhpark
Copy link

yhpark commented Jan 25, 2014

@goncalossilva Are you sure this started showing up on 2.3.1?
I found Arrays.copyOf in 2.3.0 while looking for which version I should go back to.
https://github.com/FasterXML/jackson-core/blob/jackson-core-2.3.0/src/main/java/com/fasterxml/jackson/core/io/CharacterEscapes.java

@goncalossilva
Copy link
Contributor Author

Woops. It seems compatibility was broken on 2.3.0, indeed. I probably thought otherwise because we just finished a long development cycle and despite having used 2.3.0 for a while, 2.3.1 was the only one we really tested, and I didn't recall having issues with 2.3.0. Clearly I'd have them anyway.

@goncalossilva
Copy link
Contributor Author

I guess FroYo support was dropped in 2.3 then :-) Feel free to close the other PR as well, unless you want to add it back.

@yhpark
Copy link

yhpark commented Jan 25, 2014

Sorry for confusion, I'm not one of the maintainers here. I was just pointing out the fact for other people who might run into this thread. Surely I didn't mean we should drop Froyo support from 2.3. I hope it stays until 2.3!

@cowtowncoder
Copy link
Member

I think we can definitely make 2.3.2 Froyo supporting, if these are the only changes needed.
But let me know how valuable this would be.

FWIW, I wondered about use of Arrays.copyOf() since I thought it was used from 2.3.0 onwards.

So... I am open to suggestions either way. I don't think anyone would be strongly against doing these changes for 2.3.2, and letting 2.4 require full suite of 1.6 methods.

@goncalossilva
Copy link
Contributor Author

Well, for me personally it'd be great to have FroYo compatibility, since as I've said I'm working on an Android project which also targets FroYo.

These should be the only changes needed. Still, I'll leave it up to you guys.

@cowtowncoder
Copy link
Member

Ok let's plan on doing this for 2.3.2 then; and letting 2.4 drop the support (unless there is massive support on dev list for continuing support).

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.

3 participants