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

Bump minSdkVersion to 22 and drop pre-Lollipop specific code #915

Merged
merged 5 commits into from
Apr 3, 2020

Conversation

BBosman
Copy link
Contributor

@BBosman BBosman commented Feb 5, 2020

Platforms affected

Android

Motivation and Context

As was discussed on the mailing list, the plan is to raise the minimum supported API level to 22, dropping support for Android 4.4 and 5.0 for the next major cordova-android release.

Description

This PR updates the defaultMinSdkVersion to 22.

There are also a couple of places in the codebase where we have specific logic for older (i.e. KitKat) API-levels which are now no longer needed and can therefore be removed.

I also renamed the needsKitKatContentUrlFix function to just needsContentUrlFix as it is no longer KitKat specific.

And lastly, I did some minor whitespace fixups and removed an obsolete piece of commented out code.

Testing

I ran npm run test without issues and tested the changes in one of my own apps.

Checklist

  • I've run the tests to see all new and existing tests pass

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #915 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #915   +/-   ##
=======================================
  Coverage   65.53%   65.53%           
=======================================
  Files          21       21           
  Lines        1828     1828           
=======================================
  Hits         1198     1198           
  Misses        630      630           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c93f93f...b85e28f. Read the comment docs.

@breautek breautek requested a review from erisu February 5, 2020 23:45
@breautek breautek added this to the 9.0.0 milestone Feb 5, 2020
@timbru31
Copy link
Member

timbru31 commented Feb 6, 2020

Great, thanks for the PR 👍. I've took a quick look, so far it seems fine. I'll give the PR a more detailed review later.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - LGTM, with a minor comment

It took me a few minutes to dig up the conversation where we did reach agreement to drop Android 5.0 as well as 4.4. Linking here.

My one minor comment is that cleanup items such as renaming of a function and formatting cleanups should be clear in the description and in the commit messages.

@BBosman
Copy link
Contributor Author

BBosman commented Mar 13, 2020

@brodybits Like this?

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @BBosman. Next issue is that the build seems to be broken for some reason.

@timbru31
Copy link
Member

timbru31 commented Apr 1, 2020

@BBosman could you rebase your PR? The CI should run again after that.

@BBosman
Copy link
Contributor Author

BBosman commented Apr 1, 2020

@timbru31 Done

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - tested the PR and SDK version was raised as expected. No stale code references found with a quick search for 19 or KitKat.

If no veto, I'm merging this 24 hours.

@MuhAssar
Copy link

would you please summarize the motivations for dropping android 5.0 support?
as you already know android 5.0 can update its webview to the latest version so it is very capable to run modern web apps.

@breautek
Copy link
Contributor

breautek commented May 16, 2020

would you please summarize the motivations for dropping android 5.0 support?
as you already know android 5.0 can update its webview to the latest version so it is very capable to run modern web apps.

The full discussion on this can be found at https://lists.apache.org/thread.html/rd6b0401570ddf18f241e565904e084197f8731238f72482938a80cb4%40%3Cdev.cordova.apache.org%3E

To Summarize:

  • Android 5.0 was deemed to have an insignificant market share
  • Android 5.0 was known to be buggy with severe memory leaks, which were resolved in 5.1
  • Apparently not all vendors implemented the updatable webview in Android 5.0.
  • Users can always override the minSdkVersion to 21 if they really wanted to have their app available to android 5.0, and Cordova will probably work.

@MuhAssar
Copy link

@breautek thank you, regarding:

Apparently not all vendors implemented the updatable webview in Android 5.0.

yup, I had this problem with many of my users as they were unable to update the webview even if they installed it from the playstore, it remains v39.
and that lead to 1 star reviews unfortunately.

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.

7 participants