[Fragment] Add lint warning for calling setOnCancelListener and setOnDismissListener in onCreateDialog() #171
Next
Next commit
Add lint warning for calling setOnCancelListener and setOnDismissList…
…ener in onCreateDialog()
- Loading branch information
commit b6a1858d11e3ad49e6d4f110df65e252a06e2575
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 1,139 @@ | ||
| /* | ||
| * Copyright 2021 The Android Open Source Project | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| @file:Suppress("UnstableApiUsage") | ||
|
|
||
| package androidx.fragment.lint | ||
|
|
||
| import com.android.tools.lint.client.api.UElementHandler | ||
| import com.android.tools.lint.detector.api.Category | ||
| import com.android.tools.lint.detector.api.Detector | ||
| import com.android.tools.lint.detector.api.Implementation | ||
| import com.android.tools.lint.detector.api.Issue | ||
| import com.android.tools.lint.detector.api.JavaContext | ||
| import com.android.tools.lint.detector.api.Scope | ||
| import com.android.tools.lint.detector.api.Severity | ||
| import com.android.tools.lint.detector.api.SourceCodeScanner | ||
| import com.android.tools.lint.detector.api.isKotlin | ||
| import com.intellij.psi.impl.source.PsiClassReferenceType | ||
| import org.jetbrains.kotlin.psi.psiUtil.getSuperNames | ||
| import org.jetbrains.uast.UCallExpression | ||
| import org.jetbrains.uast.UClass | ||
| import org.jetbrains.uast.UElement | ||
| import org.jetbrains.uast.kotlin.KotlinUClass | ||
| import org.jetbrains.uast.visitor.AbstractUastVisitor | ||
|
|
||
| /** | ||
| * When using a `DialogFragment`, the `setOnCancelListener` and `setOnDismissListener` callback | ||
| * functions within the `onCreateDialog` function __must not be used__ | ||
| * because the `DialogFragment` owns these callbacks. Instead the respective `onCancel` and | ||
| * `onDismiss` functions can be used to achieve the desired effect. | ||
| */ | ||
| class DialogFragmentCallbacksDetector : Detector(), SourceCodeScanner { | ||
|
|
||
| companion object Issues { | ||
| val ISSUE = Issue.create( | ||
| id = "DialogFragmentCallbacksDetector", | ||
| briefDescription = "Use onCancel() and onDismiss() callbacks to get the instead of " | ||
|
There was a problem hiding this comment. "Use onCancel() and onDismiss() instead of calling setOnCancelListener() and setOnDismissListener() from onCreateDialog()" |
||
| "calling setOnCancelListener() and setOnDismissListener() from onCreateDialog()", | ||
| explanation = """When using a `DialogFragment`, the `setOnCancelListener` and \ | ||
| `setOnDismissListener` callback functions within the `onCreateDialog` function \ | ||
| __must not be used__ because the `DialogFragment` owns these callbacks. \ | ||
| Instead the respective `onCancel` and `onDismiss` functions can be used to \ | ||
| achieve the desired effect.""", | ||
| category = Category.CORRECTNESS, | ||
| priority = 9, | ||
|
There was a problem hiding this comment. We don't normally set priority. You can remove this. |
||
| severity = Severity.WARNING, | ||
| implementation = Implementation( | ||
| DialogFragmentCallbacksDetector::class.java, | ||
| Scope.JAVA_FILE_SCOPE | ||
| ), | ||
| androidSpecific = true | ||
| ) | ||
| } | ||
|
|
||
| override fun getApplicableUastTypes(): List<Class<out UElement>>? { | ||
| return listOf(UClass::class.java) | ||
| } | ||
|
|
||
| override fun createUastHandler(context: JavaContext): UElementHandler? { | ||
| return UastHandler(context) | ||
| } | ||
|
|
||
| private inner class UastHandler(val context: JavaContext) : UElementHandler() { | ||
| override fun visitClass(node: UClass) { | ||
| if (isKotlin(context.psiFile) && (node as KotlinUClass).ktClass!!.getSuperNames()[0] != | ||
| DIALOG_FRAGMENT_CLASS | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| if (!isKotlin(context.psiFile) && | ||
| (node.uastSuperTypes[0].type as PsiClassReferenceType) | ||
| .className != DIALOG_FRAGMENT_CLASS | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| node.methods.forEach { | ||
| if (it.name == ENTRY_METHOD) { | ||
| val visitor = UastMethodsVisitor(context, it.name) | ||
| it.uastBody?.accept(visitor) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A UAST Visitor that explores all method calls within a | ||
| * [androidx.fragment.app.DialogFragment] callback to check for an incorrect method call. | ||
| * | ||
| * @param context The context of the lint request. | ||
| * @param containingMethodName The name of the originating Fragment lifecycle method. | ||
| */ | ||
| private class UastMethodsVisitor( | ||
| private val context: JavaContext, | ||
| private val containingMethodName: String | ||
| ) : AbstractUastVisitor() { | ||
| private val visitedMethods = mutableSetOf<UCallExpression>() | ||
|
|
||
| override fun visitCallExpression(node: UCallExpression): Boolean { | ||
| if (visitedMethods.contains(node)) { | ||
| return super.visitCallExpression(node) | ||
| } | ||
|
|
||
| val methodName = node.methodIdentifier?.name ?: return super.visitCallExpression(node) | ||
|
|
||
| if (callbacks.contains(methodName)) { | ||
| context.report( | ||
| issue = ISSUE, | ||
| location = context.getLocation(node), | ||
| message = "Use onCancel() and onDismiss() callbacks to get the instead of " | ||
|
There was a problem hiding this comment. Can we actually separate this and report in the message based on whether |
||
| "calling setOnCancelListener() and setOnDismissListener() " | ||
| "from onCreateDialog()", | ||
| quickfixData = null | ||
| ) | ||
|
|
||
| visitedMethods.add(node) | ||
| } | ||
| return super.visitCallExpression(node) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private const val ENTRY_METHOD = "onCreateDialog" | ||
| private const val DIALOG_FRAGMENT_CLASS = "DialogFragment" | ||
| private val callbacks = setOf("setOnCancelListener", "setOnDismissListener") | ||
|
There was a problem hiding this comment. Add an extra line at the end of the file. |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 1,176 @@ | ||
| /* | ||
| * Copyright 2021 The Android Open Source Project | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package androidx.fragment.lint | ||
|
|
||
| import com.android.tools.lint.checks.infrastructure.LintDetectorTest | ||
| import com.android.tools.lint.detector.api.Detector | ||
| import com.android.tools.lint.detector.api.Issue | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.junit.runners.JUnit4 | ||
|
|
||
| /* ktlint-disable max-line-length */ | ||
| @RunWith(JUnit4::class) | ||
| class DialogFragmentCallbacksDetectorTest : LintDetectorTest() { | ||
|
There was a problem hiding this comment. Once the reports are separated out, I think we should add individual tests for using single callbacks. |
||
|
|
||
| override fun getDetector(): Detector = DialogFragmentCallbacksDetector() | ||
|
|
||
| override fun getIssues(): MutableList<Issue> { | ||
| return mutableListOf(DialogFragmentCallbacksDetector.ISSUE) | ||
| } | ||
|
|
||
| private val dialogFragmentCorrectImplementationStubJava = java( | ||
| """ | ||
| package foo; | ||
| import android.app.Dialog; | ||
| import android.content.DialogInterface; | ||
| import android.os.Bundle; | ||
| import androidx.appcompat.app.AlertDialog; | ||
| import androidx.fragment.app.DialogFragment; | ||
| public class TestFragment extends DialogFragment { | ||
| @NonNull | ||
| @Override | ||
| public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) { | ||
| Dialog dialog = AlertDialog.Builder(requireActivity()); | ||
| return dialog.create(); | ||
| } | ||
| } | ||
| """ | ||
| ).indented() | ||
|
|
||
| private val dialogFragmentCorrectImplementationStubKotlin = kotlin( | ||
| """ | ||
| package foo | ||
| import android.app.Dialog | ||
| import android.content.DialogInterface | ||
| import android.os.Bundle | ||
| import androidx.appcompat.app.AlertDialog | ||
| import androidx.fragment.app.DialogFragment | ||
| class TestDialog : DialogFragment() { | ||
| override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { | ||
| val dialog = AlertDialog.Builder(requireActivity()) | ||
| return dialog.create() | ||
| } | ||
| override fun onCancel(dialog: DialogInterface) { | ||
| super.onCancel(dialog) | ||
| } | ||
|
|
||
| override fun onDismiss(dialog: DialogInterface) { | ||
| super.onDismiss(dialog) | ||
| } | ||
| } | ||
| """ | ||
| ).indented() | ||
|
|
||
| private val dialogFragmentStubJava = java( | ||
| """ | ||
| package foo; | ||
| import android.app.Dialog; | ||
| import android.content.DialogInterface; | ||
| import android.os.Bundle; | ||
| import androidx.appcompat.app.AlertDialog; | ||
| import androidx.fragment.app.DialogFragment; | ||
| public class TestFragment extends DialogFragment { | ||
| @Override | ||
| public Dialog onCreateDialog(Bundle savedInstanceState) { | ||
| Dialog dialog = AlertDialog.Builder(requireActivity()); | ||
| dialog.setOnCancelListener({ }); | ||
| dialog.setOnDismissListener({ }); | ||
| return dialog.create(); | ||
| } | ||
|
|
||
| @Override | ||
| public onDismiss(DialogInterface dialog) { | ||
| super.onDismiss(dialog); | ||
| } | ||
| } | ||
| """ | ||
| ).indented() | ||
|
|
||
| private val dialogFragmentStubKotlin = kotlin( | ||
| """ | ||
| package foo | ||
| import android.app.Dialog | ||
| import android.content.DialogInterface | ||
| import android.os.Bundle | ||
| import androidx.appcompat.app.AlertDialog | ||
| import androidx.fragment.app.DialogFragment | ||
| class TestDialog : DialogFragment() { | ||
| override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { | ||
| val dialog = AlertDialog.Builder(requireActivity()) | ||
| dialog.setOnCancelListener { } | ||
| dialog.setOnDismissListener { } | ||
| return dialog.create() | ||
| } | ||
|
|
||
| override fun onDismiss(dialog: DialogInterface) { | ||
| super.onDismiss(dialog) | ||
| } | ||
| } | ||
| """ | ||
| ).indented() | ||
|
|
||
| @Test | ||
| fun `java expect fail dialog fragment`() { | ||
| lint().files(dialogFragmentStubJava) | ||
| .run() | ||
| .expect( | ||
| """ | ||
| src/foo/TestFragment.java:11: Warning: Use onCancel() and onDismiss() callbacks to get the instead of calling setOnCancelListener() and setOnDismissListener() from onCreateDialog() [DialogFragmentCallbacksDetector] | ||
|
There was a problem hiding this comment. You will have to fix these to reflect the changed description above. |
||
| dialog.setOnCancelListener({ }); | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| src/foo/TestFragment.java:12: Warning: Use onCancel() and onDismiss() callbacks to get the instead of calling setOnCancelListener() and setOnDismissListener() from onCreateDialog() [DialogFragmentCallbacksDetector] | ||
| dialog.setOnDismissListener({ }); | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| 0 errors, 2 warnings | ||
| """ | ||
| ) | ||
| .expectWarningCount(2) | ||
| } | ||
|
|
||
| @Test | ||
| fun `java expect clean dialog fragment`() { | ||
| lint().files(dialogFragmentCorrectImplementationStubJava) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
|
|
||
| @Test | ||
| fun `kotlin expect fail dialog fragment`() { | ||
| lint().files(dialogFragmentStubKotlin) | ||
| .run() | ||
| .expect( | ||
| """ | ||
| src/foo/TestDialog.kt:10: Warning: Use onCancel() and onDismiss() callbacks to get the instead of calling setOnCancelListener() and setOnDismissListener() from onCreateDialog() [DialogFragmentCallbacksDetector] | ||
| dialog.setOnCancelListener { } | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| src/foo/TestDialog.kt:11: Warning: Use onCancel() and onDismiss() callbacks to get the instead of calling setOnCancelListener() and setOnDismissListener() from onCreateDialog() [DialogFragmentCallbacksDetector] | ||
| dialog.setOnDismissListener { } | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| 0 errors, 2 warnings | ||
| """ | ||
| ) | ||
| .expectWarningCount(2) | ||
| } | ||
|
|
||
| @Test | ||
| fun `kotlin expect clean dialog fragment`() { | ||
| lint().files(dialogFragmentCorrectImplementationStubKotlin) | ||
| .run() | ||
| .expectClean() | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be more specific in the naming here. Something like
OnCreateDialogIncorrectCallbackDetector