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

[webview_flutter_android] Fix Android lint warnings #3675

Merged
merged 18 commits into from
Apr 13, 2023

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Apr 10, 2023

Part of flutter/flutter#88011

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@bparrishMines bparrishMines changed the title Fix lint errors [webview_flutter_android] Fix Android lint warnings Apr 10, 2023
@@ -30,7 +32,7 @@ public void setCookie(String url, String value) {
* @param cookieManager The cookie manager to clear all cookies from.
* @return Whether any cookies were removed.
*/
@SuppressWarnings("deprecation")
@SuppressWarnings({"deprecation", "RedundantSuppression"})
Copy link
Contributor Author

@bparrishMines bparrishMines Apr 10, 2023

Choose a reason for hiding this comment

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

@stuartmorgan

This is the scenario where the javac linter and the Android gradle linter disagreed. The javac linter claims this is deprecated, but the Android lint believes this does not need a suppression.

After I did some investigation, It looks like the problem is that the javac linter doesn't understand the if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) or @RequiresApi(api = Build.VERSION_CODES.LOLLIPOP) logic. There are other instance of this throughout this plugin, and will probably show up in other plugins as well.

Potentially we should ignore the deprecation warning from the javac linter and allow the Android gradle linter to handle it?

@reidbaker FYI

Copy link
Contributor

@stuartmorgan stuartmorgan Apr 11, 2023

Choose a reason for hiding this comment

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

If the Android lint deprecation option covers everything the javac lint one does, but automatically handles the version checks, disabling deprecation for javac in all plugins sounds good to me. The fact that the javac version made us suppress everything even when we were already doing checks was really annoying.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, is in the Android lint that we run with android-lint (i.e., ./gradlew lint) that objects to the suppression, or just AS? I just hit this locally while experimenting with another plugin's warnings, and I see that in AS, but not in the android-lint lint report.

As far as I can tell, the AS linter is a third, different linter, that shows different things. (I also see things in the lint report that don't show up in AS.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I tried it again in Android Studio and I think you're right. I must have got the error mixed up with another one. The lint command does still pass if only @SuppressWarnings("deprecation") is used. I should be able to change my local AS settings to ignore this warning.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for all the cleanup!

  • I added new tests to check the change I am making, or this PR is test-exempt.

The PR should also update (or remove if empty) the lint-baseline.xml file, which would serve as a test for this.

@@ -118,9 +116,11 @@ private static ArrayList<DisplayListener> yoinkDisplayListeners(DisplayManager d
return new ArrayList<>();
}
try {
//noinspection JavaReflectionMemberAccess
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these directives for the AS linter? I've not seen them for the linters we run in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I didn't realize this was unique to AS. I went ahead and removed them

@@ -22,15 +22,39 @@
import java.util.Map;

/** Generated class from Pigeon. */
@SuppressWarnings({"unused", "unchecked", "CodeBlock2Expr", "RedundantSuppression"})
@SuppressWarnings({"unused", "unchecked", "CodeBlock2Expr", "RedundantSuppression", "serial"})
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own education did this suppression come from running pidgen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we added this to the Pigeon generator for Java recently.

@bparrishMines
Copy link
Contributor Author

@stuartmorgan I removed the baseline and finished fixing the rest of the lint errors

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM!

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 13, 2023
@auto-submit auto-submit bot merged commit d5801ea into flutter:main Apr 13, 2023
@bparrishMines bparrishMines deleted the fix_lint_errors branch April 13, 2023 19:08
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 14, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 14, 2023
flutter/packages@d01f4ea...e4ec155

2023-04-14 stuartmorgan@google.com [shared_preferences] Remove Android lint baseline (flutter/packages#3707)
2023-04-13 37607224+AmanNegi@users.noreply.github.com [shared_preferences] Ports SharedPreferencesAndroid to Pigeon (flutter/packages#3321)
2023-04-13 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android] Fix Android lint warnings (flutter/packages#3675)
2023-04-13 gautier.bayzelon@gmail.com [go_router_builder] Use only one .where() (flutter/packages#3698)
2023-04-13 engine-flutter-autoroll@skia.org Roll Flutter from 3b4b149 to be45eb2 (21 revisions) (flutter/packages#3701)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
flutter/packages@d01f4ea...e4ec155

2023-04-14 stuartmorgan@google.com [shared_preferences] Remove Android lint baseline (flutter/packages#3707)
2023-04-13 37607224+AmanNegi@users.noreply.github.com [shared_preferences] Ports SharedPreferencesAndroid to Pigeon (flutter/packages#3321)
2023-04-13 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android] Fix Android lint warnings (flutter/packages#3675)
2023-04-13 gautier.bayzelon@gmail.com [go_router_builder] Use only one .where() (flutter/packages#3698)
2023-04-13 engine-flutter-autoroll@skia.org Roll Flutter from 3b4b149 to be45eb2 (21 revisions) (flutter/packages#3701)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
[webview_flutter_android] Fix Android lint warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: webview_flutter platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants