Use `io.flutter.Build.API_LEVELS` rather than `android.os.Build.VERSION_CODES` by dnfield · Pull Request #51171 · flutter/engine

dnfield · 2024-03-04T18:48:42Z

Updates the linting script to ban the use of VERSION_CODES.

We currently have a mish-mash of using the integers, using VERSION_CODES, and even how we import the version codes. This makes it more confusing when doing things like #51070 - I think it is clearer to see 22 than LOLLIPOP_MR1.

I'd like to get LGTM (or at least no opinion) from all the requested reviewers here.

matanlurey

As long as we're consistent one way or another (and it works with our linting strategy), I'm LGTM.

gmackall

I'm fine with either way (but agreed we should have only one way). I find the integers more readable but I also like that the codes are more easily searchable

shell/platform/android/io/flutter/view/AccessibilityBridge.java

camsim99

Also LGTM! Glad to have something consistent moving forward.

johnmccutchan · 2024-03-04T22:26:47Z

This seems like a step backwards in terms of code readability.

Before: "Build.VERSION_FOO"
After: "18"

We lose our ability to search for something meaningful.

Why are we making this change?

At the very least, why don't we introduce our own "AndroidBuild.VERSION_N" for all N and use that instead of the raw integer?

johnmccutchan

See my comment on the issue.

dnfield · 2024-03-05T18:34:09Z

At the very least, why don't we introduce our own "AndroidBuild.VERSION_N" for all N and use that instead of the raw integer?

I think this works for me - but I'd want the raw integer in there, e.g. io.flutter.Build.VERSION_CODES.API_24, not whatever the Android team decided to use as a maybe single letter or maybe whole word or maybe service release code in a given year.

dnfield · 2024-03-05T18:36:07Z

I have two reasons for wanting this change:

Consistency. Right now we use a mixture of integers and VERSION_CODES. We also use different style for how we import the version codes.
The numbers correspond to what the Android docs actually use - e.g. they will say Introduced in API 28, not Introduced in Android P.
It will be harder to lint that we do not use integers and keep introducing the inconsistency - although we could at least lint that you use our special class and not the VERSION_CODES class from Android.

gmackall

LGTM! Glad we found a solution that maximizes both searchability and readability

johnmccutchan · 2024-03-05T19:49:33Z

Thanks for making my requested changes @dnfield

johnmccutchan · 2024-03-06T03:05:22Z

shell/platform/android/io/flutter/Build.java

+
+/** A replacement of utilities from android.os.Build. */
+public class Build {
+  /** For use in place of the Android VERSION CODES class. */


Rewrite the comment so if someone searched for the official class they would find this:

/** For use in place of Build.VERSION_CODES class */

I can do that, but I'll have to special case the forbidden imports script to allow it in this file.

Can the linter detect that text is contained inside of a comment?

johnmccutchan · 2024-03-06T03:06:04Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursorPlugin.java

 /** A mandatory plugin that handles mouse cursor requests. */
-@TargetApi(Build.VERSION_CODES.N)
-@RequiresApi(Build.VERSION_CODES.N)
+@TargetApi(24)


Fix here and below

I lost track of what these were - I'll update them all.

johnmccutchan · 2024-03-06T03:06:30Z

shell/platform/android/io/flutter/plugin/text/ProcessTextPlugin.java

   */
-  @TargetApi(Build.VERSION_CODES.M)
-  @RequiresApi(Build.VERSION_CODES.M)
+  @TargetApi(23)


fix here and below

johnmccutchan · 2024-03-06T03:06:43Z

shell/platform/android/io/flutter/view/FlutterView.java

  @Override
-  @TargetApi(Build.VERSION_CODES.N)
-  @RequiresApi(Build.VERSION_CODES.N)
+  @TargetApi(24)


fix here and here

johnmccutchan · 2024-03-06T03:06:57Z

shell/platform/android/io/flutter/embedding/android/FlutterImageView.java

@@ -253,7 +255,7 @@ private void closeCurrentImage() {

  @TargetApi(29)


johnmccutchan · 2024-03-06T03:07:24Z

shell/platform/android/test/io/flutter/plugin/mouse/MouseCursorPluginTest.java

+    minSdk = API_LEVELS.API_24,
    shadows = {})
 @RunWith(AndroidJUnit4.class)
 @TargetApi(24)


johnmccutchan · 2024-03-06T03:07:35Z

shell/platform/android/test/io/flutter/plugin/platform/SingleViewPresentationTest.java

 @Config(manifest = Config.NONE)
 @RunWith(AndroidJUnit4.class)
-@TargetApi(P)
+@TargetApi(28)


johnmccutchan · 2024-03-06T03:07:44Z

shell/platform/android/test/io/flutter/plugin/text/ProcessTextPluginTest.java

 @RunWith(AndroidJUnit4.class)
-@TargetApi(Build.VERSION_CODES.N)
-@RequiresApi(Build.VERSION_CODES.N)
+@TargetApi(24)


fix here and below

reidbaker · 2024-03-07T14:21:09Z

shell/platform/android/test/io/flutter/view/AccessibilityBridgeTest.java

@@ -81,7 +82,7 @@ public void itDescribesNonTextFieldsWithAContentDescription() {
  }

  @Config(sdk = 28)


I think all the @config values in this file should change to API_LEVELS.*

reidbaker · 2024-03-07T14:22:01Z

...nario_app/android/app/src/androidTest/java/dev/flutter/scenariosui/ExternalTextureTests.java


  @Test
-  @SdkSuppress(minSdkVersion = VERSION_CODES.M)
+  @SdkSuppress(minSdkVersion = 23)


Why are these not API_LEVELS?

reidbaker · 2024-03-07T14:23:05Z

tools/android_illegal_imports.py

+                                FLUTTER_LOG_CLASS) or CheckBadFiles(
+                                    bad_trace_files, 'android[x].tracing.Trace', FLUTTER_TRACE_CLASS
+                                ) or CheckBadFiles(
+                                    bad_version_codes_files, 'android.os.Build.VERSION_CODES',


Consider adding either a description here or a pointer to why these are considered bad files.

Added some comments.

auto-submit · 2024-03-08T18:21:18Z

auto label is removed for flutter/engine/51171, due to - The status or check suite Linux linux_android_debug_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ld.VERSION_CODES` (flutter/engine#51171)

flutter/engine@05fdf94...e099277 2024-03-08 dnfield@google.com Use `io.flutter.Build.API_LEVELS` rather than `android.os.Build.VERSION_CODES` (flutter/engine#51171) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

dnfield added 3 commits March 4, 2024 08:49

Guard against API 22

297270a

Ban android.Build.VERSION_CODES

feaeae9

merge

d516bb0

dnfield requested review from camsim99, gmackall, johnmccutchan, matanlurey and reidbaker March 4, 2024 18:48

github-actions bot added the platform-android label Mar 4, 2024

matanlurey reviewed Mar 4, 2024

View reviewed changes

gmackall reviewed Mar 4, 2024

View reviewed changes

shell/platform/android/io/flutter/view/AccessibilityBridge.java Outdated Show resolved Hide resolved

camsim99 reviewed Mar 4, 2024

View reviewed changes

dnfield added 2 commits March 4, 2024 13:45

unintended change

b245c6a

missing symbol

5d824e2

johnmccutchan suggested changes Mar 4, 2024

View reviewed changes

dnfield added 2 commits March 5, 2024 10:43

Merge remote-tracking branch 'upstream/main' into version_Codes

c419ebf

Use class

da04a27

gmackall approved these changes Mar 5, 2024

View reviewed changes

missing imports

0bd7854

dnfield requested a review from johnmccutchan March 5, 2024 23:55

johnmccutchan reviewed Mar 6, 2024

View reviewed changes

Doc update, checker update, TargetApi/RequiresApi update

262afee

dnfield requested a review from johnmccutchan March 6, 2024 03:33

dnfield changed the title ~~Use integer version codes rather than Build.VERSION_CODES~~ Use io.flutter.Build.API_LEVELS rather than android.os.Build.VERSION_CODES Mar 6, 2024

johnmccutchan approved these changes Mar 6, 2024

View reviewed changes

more imports

da7f0c2

Merge branch 'main' into version_Codes

ced3f92

reidbaker reviewed Mar 8, 2024

View reviewed changes

reid review

eb297be

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

missing files

71f3490

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

one last missing import

9586efb

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

auto-submit bot merged commit e099277 into flutter:main Mar 8, 2024

engine-flutter-autoroll mentioned this pull request Mar 8, 2024

Roll Flutter Engine from 05fdf947f81b to e099277ce061 (1 revision) flutter/flutter#144844

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2024

e099277 Use io.flutter.Build.API_LEVELS rather than `android.os.Bui…

28ecb15

…ld.VERSION_CODES` (flutter/engine#51171)

justinmc mentioned this pull request Mar 11, 2024

Platform channel for predictive back in route transitions on android #49093

Merged

8 tasks

		@@ -253,7 +255,7 @@ private void closeCurrentImage() {

		@TargetApi(29)

		@@ -81,7 +82,7 @@ public void itDescribesNonTextFieldsWithAContentDescription() {
		}

		@Config(sdk = 28)

Conversation

dnfield commented Mar 4, 2024

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

gmackall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camsim99 left a comment

Choose a reason for hiding this comment

Uh oh!

johnmccutchan commented Mar 4, 2024

Uh oh!

johnmccutchan left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 5, 2024

Uh oh!

dnfield commented Mar 5, 2024

Uh oh!

gmackall left a comment

Choose a reason for hiding this comment

Uh oh!

johnmccutchan commented Mar 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gmackall left a comment •

edited

Loading