Refine highlighting and error reporting in Lint rules (#693)

README.md

@@ -39,7 +39,7 @@ implementation "com.bluelinelabs:conductor-viewpager2:$conductorVersion"
 ```
 
 ### 4.0 Preview
-Use `4.0.0-preview-2` as your version number in any of the dependencies above.
+Use `4.0.0-preview-4` as your version number in any of the dependencies above.
 
 ### SNAPSHOT
 Use `4.0.0-SNAPSHOT` as your version number in any of the dependencies above and add the url to the snapshot repository:

conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerIssueDetector.java

@@ -18,7 +18,6 @@ import org.jetbrains.uast.UMethod;
 import java.util.Collections;
 import java.util.List;
 
-@SuppressWarnings("UnstableApiUsage")
 public final class ControllerChangeHandlerIssueDetector extends Detector implements Detector.UastScanner {
 
     static final Issue ISSUE =
@@ -32,7 +31,7 @@ public final class ControllerChangeHandlerIssueDetector extends Detector impleme
 
     @Override
     public List<Class<? extends UElement>> getApplicableUastTypes() {
-        return Collections.<Class<? extends UElement>>singletonList(UClass.class);
+        return Collections.singletonList(UClass.class);
     }
 
     @Override
@@ -47,42 +46,41 @@ public final class ControllerChangeHandlerIssueDetector extends Detector impleme
                     return;
                 }
 
-                final boolean hasSuperType = evaluator.extendsClass(node.getPsi(), CLASS_NAME, true);
+                final boolean hasSuperType = evaluator.extendsClass(node.getJavaPsi(), CLASS_NAME, true);
                 if (!hasSuperType) {
                     return;
                 }
 
                 if (!evaluator.isPublic(node)) {
                     String message = String.format("This ControllerChangeHandler class should be public (%1$s)", node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message);
                     return;
                 }
 
                 if (node.getContainingClass() != null && !evaluator.isStatic(node)) {
                     String message = String.format("This ControllerChangeHandler inner class should be static (%1$s)", node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message);
                     return;
                 }
 
-                boolean hasConstructor = false;
+                UMethod constructor = null;
                 boolean hasDefaultConstructor = false;
                 for (UMethod method : node.getMethods()) {
                     if (method.isConstructor()) {
-                        hasConstructor = true;
-                        if (evaluator.isPublic(method) && method.getUastParameters().size() == 0) {
+                        constructor = method;
+                        if (evaluator.isPublic(method) && method.getUastParameters().isEmpty()) {
                             hasDefaultConstructor = true;
                             break;
                         }
                     }
                 }
 
-                if (hasConstructor && !hasDefaultConstructor) {
+                if (constructor != null && !hasDefaultConstructor) {
                     String message = String.format(
                             "This ControllerChangeHandler needs to have a public default constructor (`%1$s`)", node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(constructor)), message);
                 }
             }
         };
     }
-
 }

conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerIssueDetector.java

@@ -10,6 +10,7 @@ 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.intellij.psi.PsiType;
 
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.uast.UClass;
@@ -20,7 +21,6 @@ import org.jetbrains.uast.UParameter;
 import java.util.Collections;
 import java.util.List;
 
-@SuppressWarnings("UnstableApiUsage")
 public final class ControllerIssueDetector extends Detector implements Detector.UastScanner {
 
     static final Issue ISSUE =
@@ -48,52 +48,53 @@ public final class ControllerIssueDetector extends Detector implements Detector.
                     return;
                 }
 
-                final boolean hasSuperType = evaluator.extendsClass(node.getPsi(), CLASS_NAME, true);
+                final boolean hasSuperType = evaluator.extendsClass(node.getJavaPsi(), CLASS_NAME, true);
                 if (!hasSuperType) {
                     return;
                 }
 
                 if (!evaluator.isPublic(node)) {
                     String message = String.format("This Controller class should be public (%1$s)", node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message);
                     return;
                 }
 
                 if (node.getContainingClass() != null && !evaluator.isStatic(node)) {
                     String message = String.format("This Controller inner class should be static (%1$s)", node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message);
                     return;
                 }
 
-                boolean hasConstructor = false;
+                UMethod constructor = null;
                 boolean hasDefaultConstructor = false;
                 boolean hasBundleConstructor = false;
                 for (UMethod method : node.getMethods()) {
                     if (method.isConstructor()) {
-                        hasConstructor = true;
+                        constructor = method;
                         if (evaluator.isPublic(method)) {
                             List<UParameter> parameters = method.getUastParameters();
-                            if (parameters.size() == 0) {
+                            if (parameters.isEmpty()) {
                                 hasDefaultConstructor = true;
                                 break;
-                            } else if (parameters.size() == 1 &&
-                                    (parameters.get(0).getType().equalsToText(SdkConstants.CLASS_BUNDLE)) ||
-                                    parameters.get(0).getType().equalsToText("Bundle")) {
-                                hasBundleConstructor = true;
+                            } else if (parameters.size() == 1) {
+                                PsiType type = parameters.get(0).getType();
+                                if (type.equalsToText(SdkConstants.CLASS_BUNDLE) || type.equalsToText("Bundle")) {
+                                    hasBundleConstructor = true;
+                                    break;
+                                }
                             }
                         }
                     }
                 }
 
-                if (hasConstructor && !hasDefaultConstructor && !hasBundleConstructor) {
+                if (constructor != null && !hasDefaultConstructor && !hasBundleConstructor) {
                     String message = String.format(
                             "This Controller needs to have either a public default constructor or a" +
                                     " public single-argument constructor that takes a Bundle. (`%1$s`)",
                             node.getQualifiedName());
-                    context.report(ISSUE, node, context.getLocation((UElement) node), message);
+                    context.report(ISSUE, node, context.getLocation(Identify.byName(constructor)), message);
                 }
             }
         };
     }
-
 }

conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/Identify.java

@@ -0,0 +1,15 @@
+package com.bluelinelabs.conductor.lint;
+
+import com.intellij.psi.PsiElement;
+import com.intellij.psi.PsiNameIdentifierOwner;
+
+final class Identify {
+    private Identify() {}
+
+    static PsiElement byName(PsiNameIdentifierOwner psiNameIdentifierOwner) {
+        if (psiNameIdentifierOwner.getNameIdentifier() != null) {
+            return psiNameIdentifierOwner.getNameIdentifier();
+        }
+        return psiNameIdentifierOwner;
+    }
+}

conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerDetectorTest.java

@@ -8,15 +8,8 @@ import com.android.tools.lint.checks.infrastructure.TestFile;
 import org.intellij.lang.annotations.Language;
 import org.junit.Test;
 
-@SuppressWarnings("UnstableApiUsage")
 public class ControllerChangeHandlerDetectorTest {
 
-    private static final String CONSTRUCTOR =
-            "src/test/SampleHandler.java:2: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n"
-                    + "public class SampleHandler extends com.bluelinelabs.conductor.ControllerChangeHandler {\n"
-                    + "^\n"
-                    + "1 errors, 0 warnings\n";
-
     private final TestFile controllerChangeHandlerStub = java(
             "package com.bluelinelabs.conductor;\n"
                     + "abstract class ControllerChangeHandler {}"
@@ -63,7 +56,12 @@ public class ControllerChangeHandlerDetectorTest {
                 .files(controllerChangeHandlerStub, java(source))
                 .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE)
                 .run()
-                .expect(CONSTRUCTOR);
+                .expect(""
+                        + "src/test/SampleHandler.java:3: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n"
+                        + "    public SampleHandler(int number) { }\n"
+                        + "           ~~~~~~~~~~~~~\n"
+                        + "1 errors, 0 warnings\n"
+                );
     }
 
     @Test
@@ -94,7 +92,12 @@ public class ControllerChangeHandlerDetectorTest {
                 .files(controllerChangeHandlerStub, java(source))
                 .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE)
                 .run()
-                .expect(CONSTRUCTOR);
+                .expect(""
+                        + "src/test/SampleHandler.java:3: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n"
+                        + "    private SampleHandler() { }\n"
+                        + "            ~~~~~~~~~~~~~\n"
+                        + "1 errors, 0 warnings\n"
+                );
     }
 
     @Test
@@ -111,7 +114,7 @@ public class ControllerChangeHandlerDetectorTest {
                 .run()
                 .expect("src/test/SampleHandler.java:2: Error: This ControllerChangeHandler class should be public (test.SampleHandler) [ValidControllerChangeHandler]\n"
                         + "private class SampleHandler extends com.bluelinelabs.conductor.ControllerChangeHandler {\n"
-                        + "^\n"
+                        + "              ~~~~~~~~~~~~~\n"
                         + "1 errors, 0 warnings\n");
     }
 
@@ -131,7 +134,7 @@ public class ControllerChangeHandlerDetectorTest {
                 .run()
                 .expect("src/test/SampleHandler.java:2: Error: This ControllerChangeHandler class should be public (test.SampleHandler) [ValidControllerChangeHandler]\n" +
                         "private class SampleHandler extends test.BaseChangeHandler {}\n" +
-                        "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" +
+                        "              ~~~~~~~~~~~~~\n" +
                         "1 errors, 0 warnings");
     }
 }

conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerDetectorTest.java

@@ -8,18 +8,12 @@ import com.android.tools.lint.checks.infrastructure.TestFile;
 import org.intellij.lang.annotations.Language;
 import org.junit.Test;
 
-@SuppressWarnings("UnstableApiUsage")
 public class ControllerDetectorTest {
 
-    private static final String CONSTRUCTOR_ERROR =
-            "src/test/SampleController.java:2: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n"
-                    + "public class SampleController extends com.bluelinelabs.conductor.Controller {\n"
-                    + "^\n"
-                    + "1 errors, 0 warnings\n";
     private static final String CLASS_ERROR =
             "src/test/SampleController.java:2: Error: This Controller class should be public (test.SampleController) [ValidController]\n"
                     + "private class SampleController extends com.bluelinelabs.conductor.Controller {\n"
-                    + "^\n"
+                    + "              ~~~~~~~~~~~~~~~~\n"
                     + "1 errors, 0 warnings\n";
 
     private final TestFile controllerStub = java(
@@ -69,7 +63,12 @@ public class ControllerDetectorTest {
                 .files(controllerStub, java(source))
                 .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE)
                 .run()
-                .expect(CONSTRUCTOR_ERROR);
+                .expect(""
+                        + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n"
+                        + "    public SampleController(int number) { }\n"
+                        + "           ~~~~~~~~~~~~~~~~\n"
+                        + "1 errors, 0 warnings\n"
+                );
     }
 
     @Test
@@ -106,11 +105,11 @@ public class ControllerDetectorTest {
                 .files(controllerStub, java(baseClass), java(source))
                 .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE)
                 .run()
-                .expect(
-                        "src/test/SampleController.java:2: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" +
-                                "public class SampleController extends BaseController {\n" +
-                                "^\n" +
-                                "1 errors, 0 warnings"
+                .expect(""
+                        + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n"
+                        + "    private SampleController() { }\n"
+                        + "            ~~~~~~~~~~~~~~~~\n"
+                        + "1 errors, 0 warnings"
                 );
     }
 
@@ -126,7 +125,12 @@ public class ControllerDetectorTest {
                 .files(controllerStub, java(source))
                 .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE)
                 .run()
-                .expect(CONSTRUCTOR_ERROR);
+                .expect(""
+                        + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n"
+                        + "    private SampleController() { }\n"
+                        + "            ~~~~~~~~~~~~~~~~\n"
+                        + "1 errors, 0 warnings\n"
+                );
     }
 
     @Test

conductor/src/main/java/com/bluelinelabs/conductor/Controller.java

@@ -1387,11 +1387,17 @@ public abstract class Controller {
         if (isDetachFrozen != frozen) {
             isDetachFrozen = frozen;
 
+            boolean detach = !frozen && view != null && viewWasDetached;
+
             for (ControllerHostedRouter router : childRouters) {
+                if (detach) {
+                    router.prepareForHostDetach();
+                }
+
                 router.setDetachFrozen(frozen);
             }
 
-            if (!frozen && view != null && viewWasDetached) {
+            if (detach) {
                 View aView = view;
                 detach(view, false, false);
                 if (view == null && aView.getParent() == router.container) {

conductor/src/main/java/com/bluelinelabs/conductor/ControllerChangeHandler.kt

@@ -23,6 +23,29 @@ abstract class ControllerChangeHandler {
    */
   open val isReusable: Boolean = false
 
+  /**
+   * Returns whether or not this handler removes the `from` view from the container when performing a push.
+   *
+   * If this is true:
+   * - This handler's implementation of [performChange] should remove `from` from `container`
+   *   before calling `changeListener.onChangeCompleted()`
+   * - When a controller is pushed, the previous controller will be detached and its view will be destroyed
+   *
+   * If this is false:
+   * - This handler's implementation of [performChange] should only remove `from` from `container`
+   *   when `isPush` is false
+   * - When a controller is pushed, the previous controller will stay attached and its view will remain created
+   * - When a view is recreated (e.g. after a configuration change), any controllers underneath a transaction
+   *   using this handler will have their view recreated and attached, even though they're not the top-most
+   *   controller
+   *
+   * If a controller pushed onto the backstack will completely cover the previous controller,
+   * using a change handler with [removesFromViewOnPush] true should result in no visual interruption
+   * to the user, while allowing the previous controller's view to be destroyed to reclaim resources.
+   * If instead, the previous controller should still be visible after the new controller is pushed,
+   * using a change handler with [removesFromViewOnPush] false will keep the previous controller's
+   * view in the view hierarchy, where it can still be seen (and even interacted with).
+   */
   open val removesFromViewOnPush: Boolean = true
 
   private var hasBeenUsed = false