Merge change Id7515f9a into eclair

* changes:
  New rules for layoutopt: IncorrectHeight/WidthIn[Horizontal]ScrollView, UselessView.
This commit is contained in:
Android (Google) Code Review
2009-10-06 17:56:15 -04:00
18 changed files with 220 additions and 238 deletions

View File

@@ -16,9 +16,13 @@
package com.android.layoutopt.uix;
import org.w3c.dom.Node;
import java.util.List;
import java.util.ArrayList;
import com.android.layoutopt.uix.groovy.LayoutAnalysisCategory;
/**
* Contains the results of a layout analysis. Instances of this class are
* generated by {@link com.android.layoutopt.uix.LayoutAnalyzer}.
@@ -40,6 +44,7 @@ public class LayoutAnalysis {
private final List<Issue> mIssues = new ArrayList<Issue>();
private String mName;
private boolean mAnalyzed;
private Node mNode;
/**
* Creates a new analysis. An analysis is always considered invalid by default.
@@ -77,16 +82,16 @@ public class LayoutAnalysis {
* @param description Description of the issue.
*/
public void addIssue(String description) {
mIssues.add(new Issue(description));
mIssues.add(new Issue(mNode, description));
}
/**
* Adds an issue to the layout analysis.
*
* @param node The layout node containing the issue.
* @param node The node containing the issue.
* @param description Description of the issue.
*/
public void addIssue(LayoutNode node, String description) {
public void addIssue(Node node, String description) {
mIssues.add(new Issue(node, description));
}
@@ -111,10 +116,22 @@ public class LayoutAnalysis {
/**
* Validates the analysis. This must be call before this analysis can
* be considered valid.
* be considered valid. Calling this method resets the current node to null.
*
* @see #setCurrentNode(org.w3c.dom.Node)
*/
void validate() {
mAnalyzed = true;
mNode = null;
}
/**
* Sets the current node to be automatically added to created issues.
*
* @param node An XML node.
*/
void setCurrentNode(Node node) {
mNode = node;
}
/**
@@ -123,7 +140,7 @@ public class LayoutAnalysis {
*/
public static class Issue {
private final String mDescription;
private final LayoutNode mNode;
private final Node mNode;
/**
* Creates a new issue with the specified description.
@@ -144,7 +161,7 @@ public class LayoutAnalysis {
* @param node The node in which the issue was found.
* @param description The description of the issue.
*/
public Issue(LayoutNode node, String description) {
public Issue(Node node, String description) {
mNode = node;
if (description == null) {
throw new IllegalArgumentException("The description must be non-null");
@@ -168,7 +185,7 @@ public class LayoutAnalysis {
* @return The start line or -1 if the line is unknown.
*/
public int getStartLine() {
return mNode == null ? -1 : mNode.getStartLine();
return LayoutAnalysisCategory.getStartLine(mNode);
}
/**
@@ -177,7 +194,7 @@ public class LayoutAnalysis {
* @return The end line or -1 if the line is unknown.
*/
public int getEndLine() {
return mNode == null ? -1 : mNode.getEndLine();
return LayoutAnalysisCategory.getEndLine(mNode);
}
}
}

View File

@@ -241,9 +241,9 @@ public class LayoutAnalyzer {
}
private void applyRules(LayoutAnalysis analysis, Node node) {
LayoutNode layoutNode = new LayoutNode(node);
analysis.setCurrentNode(node);
for (Rule rule : mRules) {
rule.run(analysis, layoutNode, node);
rule.run(analysis, node);
}
}
}

View File

@@ -1,184 +0,0 @@
/*
* Copyright (C) 2009 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 com.android.layoutopt.uix;
import org.w3c.dom.Node;
import org.w3c.dom.Element;
import org.w3c.dom.NamedNodeMap;
import org.w3c.dom.Attr;
import org.w3c.dom.NodeList;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.HashMap;
import com.android.layoutopt.uix.xml.XmlDocumentBuilder;
/**
* Wrapper class for W3C Node objects. Provides extra utilities specific
* to Android XML layouts.
*/
public class LayoutNode {
private static final String ANDROID_LAYOUT_WIDTH = "android:layout_width";
private static final String ANDROID_LAYOUT_HEIGHT = "android:layout_height";
private static final String VALUE_FILL_PARENT = "fill_parent";
private static final String VALUE_WRAP_CONTENT = "wrap_content";
private Map<String, String> mAttributes;
private final Element mNode;
private LayoutNode[] mChildren;
LayoutNode(Node node) {
if (node == null) throw new IllegalArgumentException("The node cannot be null");
if (node.getNodeType() != Node.ELEMENT_NODE) {
throw new IllegalArgumentException("The node must be an element type");
}
mNode = (Element) node;
}
/**
* Returns the start line of this node.
*
* @return The start line or -1 if the line is unknown.
*/
public int getStartLine() {
final Object data = mNode.getUserData(XmlDocumentBuilder.NODE_START_LINE);
return data == null ? -1 : (Integer) data;
}
/**
* Returns the end line of this node.
*
* @return The end line or -1 if the line is unknown.
*/
public int getEndLine() {
final Object data = mNode.getUserData(XmlDocumentBuilder.NODE_END_LINE);
return data == null ? -1 : (Integer) data;
}
/**
* Returns the wrapped W3C XML node object.
*
* @return An XML node.
*/
public Node getNode() {
return mNode;
}
/**
* Indicates whether the node is of the specified type.
*
* @param name The name of the node.
*
* @return True if this node has the same name as tagName, false otherwise.
*/
public boolean is(String name) {
return mNode.getNodeName().equals(name);
}
/**
* Indicates whether the node has declared the specified attribute.
*
* @param attribute The name of the attribute to check.
*
* @return True if the attribute is specified, false otherwise.
*/
public boolean has(String attribute) {
return mNode.hasAttribute(attribute);
}
/**
* Returns whether this node is the document root.
*
* @return True if the wrapped node is the root of the document,
* false otherwise.
*/
public boolean isRoot() {
return mNode == mNode.getOwnerDocument().getDocumentElement();
}
/**
* Returns whether this node's width is fill_parent.
*/
public boolean isWidthFillParent() {
return mNode.getAttribute(ANDROID_LAYOUT_WIDTH).equals(VALUE_FILL_PARENT);
}
/**
* Returns whether this node's width is wrap_content.
*/
public boolean isWidthWrapContent() {
return mNode.getAttribute(ANDROID_LAYOUT_WIDTH).equals(VALUE_WRAP_CONTENT);
}
/**
* Returns whether this node's height is fill_parent.
*/
public boolean isHeightFillParent() {
return mNode.getAttribute(ANDROID_LAYOUT_HEIGHT).equals(VALUE_FILL_PARENT);
}
/**
* Returns whether this node's height is wrap_content.
*/
public boolean isHeightWrapContent() {
return mNode.getAttribute(ANDROID_LAYOUT_HEIGHT).equals(VALUE_WRAP_CONTENT);
}
/**
* Returns a map of all the attributes declared for this node.
*
* The name of the attributes contains the namespace.
*
* @return A map of [name, value] describing the attributes of this node.
*/
public Map<String, String> getAttributes() {
if (mAttributes == null) {
NamedNodeMap attributes = mNode.getAttributes();
int count = attributes.getLength();
mAttributes = new HashMap<String, String>(count);
for (int i = 0; i < count; i++) {
Node node = attributes.item(i);
Attr attribute = (Attr) node;
mAttributes.put(attribute.getName(), attribute.getValue());
}
}
return mAttributes;
}
/**
* Returns all the children of this node.
*/
public LayoutNode[] getChildren() {
if (mChildren == null) {
NodeList list = mNode.getChildNodes();
int count = list.getLength();
List<LayoutNode> children = new ArrayList<LayoutNode>(count);
for (int i = 0; i < count; i++) {
Node child = list.item(i);
if (child.getNodeType() == Node.ELEMENT_NODE) {
children.add(new LayoutNode(child));
}
}
mChildren = children.toArray(new LayoutNode[children.size()]);
}
return mChildren;
}
}

View File

@@ -17,11 +17,12 @@
package com.android.layoutopt.uix.groovy;
import com.android.layoutopt.uix.LayoutAnalysis;
import com.android.layoutopt.uix.LayoutNode;
import com.android.layoutopt.uix.xml.XmlDocumentBuilder;
import java.util.Map;
import java.util.List;
import java.util.ArrayList;
import java.util.Arrays;
import groovy.lang.GString;
import groovy.xml.dom.DOMCategory;
@@ -34,6 +35,38 @@ import org.w3c.dom.Element;
* to {@link com.android.layoutopt.uix.LayoutAnalysis} and {@link org.w3c.dom.Node}.
*/
public class LayoutAnalysisCategory {
private static final String ANDROID_LAYOUT_WIDTH = "android:layout_width";
private static final String ANDROID_LAYOUT_HEIGHT = "android:layout_height";
private static final String VALUE_FILL_PARENT = "fill_parent";
private static final String VALUE_WRAP_CONTENT = "wrap_content";
private static final String[] sContainers = new String[] {
"FrameLayout", "LinearLayout", "RelativeLayout", "SlidingDrawer",
"AbsoluteLayout", "TableLayout", "Gallery", "GridView", "ListView",
"RadioGroup", "ScrollView", "HorizontalScrollView", "Spinner",
"ViewSwitcher", "ViewFlipper", "ViewAnimator", "ImageSwitcher",
"TextSwitcher", "android.gesture.GestureOverlayView", "TabHost"
};
static {
Arrays.sort(sContainers);
}
/**
* xmlNode.isContainer()
*
* @return True if the specified node corresponds to a container widget.
*/
public static boolean isContainer(Element element) {
return Arrays.binarySearch(sContainers, element.getNodeName()) >= 0;
}
/**
* xmlNode.all()
*
* Same as xmlNode.'**' but excludes xmlNode from the results.
*
* @return All descendants, this node excluded.
*/
public static List<Node> all(Element element) {
NodeList list = DOMCategory.depthFirst(element);
int count = list.getLength();
@@ -43,9 +76,62 @@ public class LayoutAnalysisCategory {
}
return nodes;
}
/**
* Returns the start line of this node.
*
* @return The start line or -1 if the line is unknown.
*/
public static int getStartLine(Node node) {
final Object data = node == null ? null :
node.getUserData(XmlDocumentBuilder.NODE_START_LINE);
return data == null ? -1 : (Integer) data;
}
/**
* Returns the end line of this node.
*
* @return The end line or -1 if the line is unknown.
*/
public static int getEndLine(Node node) {
final Object data = node == null ? null :
node.getUserData(XmlDocumentBuilder.NODE_END_LINE);
return data == null ? -1 : (Integer) data;
}
/**
* Returns whether this node's width is fill_parent.
*/
public static boolean isWidthFillParent(Element element) {
return element.getAttribute(ANDROID_LAYOUT_WIDTH).equals(VALUE_FILL_PARENT);
}
/**
* Returns whether this node's width is wrap_content.
*/
public static boolean isWidthWrapContent(Element element) {
return element.getAttribute(ANDROID_LAYOUT_WIDTH).equals(VALUE_WRAP_CONTENT);
}
/**
* Returns whether this node's height is fill_parent.
*/
public static boolean isHeightFillParent(Element element) {
return element.getAttribute(ANDROID_LAYOUT_HEIGHT).equals(VALUE_FILL_PARENT);
}
/**
* Returns whether this node's height is wrap_content.
*/
public static boolean isHeightWrapContent(Element element) {
return element.getAttribute(ANDROID_LAYOUT_HEIGHT).equals(VALUE_WRAP_CONTENT);
}
/**
* xmlNode.isRoot()
*
* @return True if xmlNode is the root of the document, false otherwise
*/
public static boolean isRoot(Node node) {
return node.getOwnerDocument().getDocumentElement() == node;
@@ -53,6 +139,8 @@ public class LayoutAnalysisCategory {
/**
* xmlNode.is("tagName")
*
* @return True if xmlNode.getNodeName().equals(name), false otherwise.
*/
public static boolean is(Node node, String name) {
return node.getNodeName().equals(name);
@@ -60,6 +148,8 @@ public class LayoutAnalysisCategory {
/**
* xmlNode.depth()
*
* @return The maximum depth of the node.
*/
public static int depth(Node node) {
int maxDepth = 0;
@@ -75,17 +165,31 @@ public class LayoutAnalysisCategory {
/**
* analysis << "The issue"
*
* @return The analysis itself to chain calls.
*/
public static LayoutAnalysis leftShift(LayoutAnalysis analysis, GString description) {
analysis.addIssue(description.toString());
return analysis;
}
/**
* analysis << "The issue"
*
* @return The analysis itself to chain calls.
*/
public static LayoutAnalysis leftShift(LayoutAnalysis analysis, String description) {
analysis.addIssue(description);
return analysis;
}
/**
* analysis << [node: node, description: "The issue"]
*
* @return The analysis itself to chain calls.
*/
public static LayoutAnalysis leftShift(LayoutAnalysis analysis, Map issue) {
analysis.addIssue((LayoutNode) issue.get("node"), issue.get("description").toString());
analysis.addIssue((Node) issue.get("node"), issue.get("description").toString());
return analysis;
}
}

View File

@@ -21,7 +21,6 @@ import groovy.lang.Binding;
import groovy.lang.Closure;
import groovy.xml.dom.DOMCategory;
import com.android.layoutopt.uix.LayoutAnalysis;
import com.android.layoutopt.uix.LayoutNode;
import com.android.layoutopt.uix.groovy.LayoutAnalysisCategory;
import org.w3c.dom.Node;
import org.codehaus.groovy.runtime.GroovyCategorySupport;
@@ -59,10 +58,9 @@ public class GroovyRule implements Rule {
return mName;
}
public void run(LayoutAnalysis analysis, LayoutNode node, Node xml) {
public void run(LayoutAnalysis analysis, Node node) {
mBinding.setVariable("analysis", analysis);
mBinding.setVariable("node", node);
mBinding.setVariable("xml", xml);
GroovyCategorySupport.use(mCategories, mClosure);
}

View File

@@ -17,7 +17,6 @@
package com.android.layoutopt.uix.rules;
import com.android.layoutopt.uix.LayoutAnalysis;
import com.android.layoutopt.uix.LayoutNode;
import org.w3c.dom.Node;
/**
@@ -36,8 +35,7 @@ public interface Rule {
* issue to the analysis.
*
* @param analysis The resulting analysis.
* @param node The layout node to analyse.
* @param xml The original XML node.
* @param node The original XML node.
*/
void run(LayoutAnalysis analysis, LayoutNode node, Node xml);
void run(LayoutAnalysis analysis, Node node);
}

View File

@@ -0,0 +1,11 @@
// Rule: IncorrectHeightInScrollView
//
// Description: Checks whether a scrollview's child has the wrong dimension.
//
// Conditions:
// - The node has a ScrollView parent
// - The node has a height set to fill_parent
if (node.'..'.is("ScrollView") && node.isHeightFillParent()) {
analysis << "This ${node.name()} tag should use android:layout_height=\"wrap_content\""
}

View File

@@ -0,0 +1,11 @@
// Rule: IncorrectWidthInScrollView
//
// Description: Checks whether a scrollview's child has the wrong dimension.
//
// Conditions:
// - The node has a HorizontalScrollView parent
// - The node has a width set to fill_parent
if (node.'..'.is("HorizontalScrollView") && node.isWidthFillParent()) {
analysis << "This ${node.name()} tag should use android:layout_width=\"wrap_content\""
}

View File

@@ -7,13 +7,13 @@
// - The node is the only sibling with a weight
// - The node has a height/width != 0
def parent = xml.'..'
if (parent.is("LinearLayout") && xml.'@android:layout_weight' &&
def parent = node.'..'
if (parent.is("LinearLayout") && node.'@android:layout_weight' &&
parent.'*'.findAll{ it.'@android:layout_weight' }.size() == 1) {
def dimension = parent.'@android:orientation' == "vertical" ?
"android:layout_height" : "android:layout_width"
if (xml."@${dimension}"[0] != 0) {
analysis << [node: node, description: "Use an ${dimension} of 0dip instead of " +
"${xml."@${dimension}"} for better performance"]
if (node."@${dimension}"[0] != 0) {
analysis << "Use an ${dimension} of 0dip instead of ${node."@${dimension}"} " +
"for better performance"
}
}

View File

@@ -9,9 +9,8 @@
// - The node is fill_parent in both orientation *or* it has no layout_gravity
// - The node does not have a background nor a foreground
if (xml.isRoot() && xml.is("FrameLayout") && !xml.'@android:background' &&
!xml.'@android:foreground' && ((node.isWidthFillParent() &&
node.isHeightFillParent()) || !xml.'@android:layout_gravity')) {
analysis << [node: node, description: "The root-level <FrameLayout/> can be " +
"replaced with <merge/>"]
if (node.isRoot() && node.is("FrameLayout") && !node.'@android:background' &&
!node.'@android:foreground' && ((node.isWidthFillParent() &&
node.isHeightFillParent()) || !node.'@android:layout_gravity')) {
analysis << "The root-level <FrameLayout/> can be replaced with <merge/>"
}

View File

@@ -7,13 +7,13 @@
// - The node has a descendant who is also a scrolling widget
def widgets = ["ScrollView", "ListView", "GridView"]
if (xml.name() in widgets && xml.all().any{ it.name() in widgets }) {
analysis << [node: node, description: "The vertically scrolling ${xml.name()} should " +
"not contain another vertically scrolling widget"]
if (node.name() in widgets && node.all().any{ it.name() in widgets }) {
analysis << "The vertically scrolling ${node.name()} should not contain another " +
"vertically scrolling widget"
}
widgets = ["HorizontalScrollView", "Gallery"]
if (xml.name() in widgets && xml.all().any{ it.name() in widgets }) {
analysis << [node: node, description: "The horizontally scrolling ${xml.name()} should " +
"not contain another horizontally scrolling widget"]
if (node.name() in widgets && node.all().any{ it.name() in widgets }) {
analysis << "The horizontally scrolling ${node.name()} should not contain another " +
"horizontally scrolling widget"
}

View File

@@ -6,10 +6,10 @@
// - The layout is a ScrollView and has more than 1 child
// - The layout is a list or grid ans has at least 1 child
if (xml.name() in ["ScrollView", "HorizontalScrollView"] && xml.'*'.size() > 1) {
analysis << [node: node, description: "A scroll view can have only one child"]
if (node.name() in ["ScrollView", "HorizontalScrollView"] && node.'*'.size() > 1) {
analysis << "A scroll view can have only one child"
}
if (xml.name() in ["ListView", "GridView"] && xml.'*'.size() > 0) {
analysis << [node: node, description: "A list/grid should have no children declared in XML"]
if (node.name() in ["ListView", "GridView"] && node.'*'.size() > 0) {
analysis << "A list/grid should have no children declared in node"
}

View File

@@ -5,6 +5,7 @@
// Conditions:
// - The depth of the layout is > 10
if (xml.isRoot() && (depth = xml.depth()) > 10) {
analysis << "This layout has too many nested layouts: ${depth} levels, it should have <= 10!"
if (node.isRoot() && (depth = node.depth()) > 10) {
analysis << [node: null, description: "This layout has too many nested layouts: " +
"${depth} levels, it should have <= 10!"]
}

View File

@@ -5,6 +5,7 @@
// Conditions:
// - The document contains more than 80 views
if (xml.isRoot && (size = xml.'**'.size()) > 80) {
analysis << "This layout has too many views: ${size} views, it should have <= 80!"
if (node.isRoot && (size = node.'**'.size()) > 80) {
analysis << [node: null,
description: "This layout has too many views: ${size} views, it should have <= 80!"]
}

View File

@@ -8,8 +8,8 @@
// - The node has two children, ImageView and TextView
// - The ImageView does not have a weight
if (xml.is("LinearLayout") && xml.'*'.size() == 2 && xml.'TextView'.size() == 1 &&
xml.'ImageView'.size() == 1 && !xml.'ImageView'[0].'@android:layout_weight') {
analysis << [node: node, description: "This tag and its children can be replaced by one " +
"<TextView/> and a compound drawable"]
if (node.is("LinearLayout") && node.'*'.size() == 2 && node.'TextView'.size() == 1 &&
node.'ImageView'.size() == 1 && !node.'ImageView'[0].'@android:layout_weight') {
analysis << "This tag and its children can be replaced by one <TextView/> and " +
"a compound drawable"
}

View File

@@ -10,11 +10,10 @@
// background or neither the node and its parent have a background
// - The parent is not a <merge/>
if (!xml.isRoot() && !(xml['..'].name() in ["ScrollView", "HorizontalScrollView"]) &&
xml['..']['*'].size() == 1 && xml['*'].size() > 0 && ((xml.'@android:background' ||
xml['..'].'@android:background') || (!xml.'@android:background' &&
!xml['..'].'@android:background'))) {
analysis << [node: node, description: "This ${xml.name()} layout or " +
"its ${xml['..'].name()} parent is " +
"${xml['..'].'@android:id' ? "possibly useless" : "useless"}"]
if (!node.isRoot() && !(node['..'].name() in ["ScrollView", "HorizontalScrollView"]) &&
node['..']['*'].size() == 1 && node['*'].size() > 0 && ((node.'@android:background' ||
node['..'].'@android:background') || (!node.'@android:background' &&
!node['..'].'@android:background'))) {
analysis << "This ${node.name()} layout or its ${node['..'].name()} parent is " +
"${node['..'].'@android:id' ? "possibly useless" : "useless"}"
}

View File

@@ -0,0 +1,14 @@
// Rule: UselessView
//
// Description: Checks whether a container view can be removed.
//
// Conditions:
// - The node is a container view (LinearLayout, etc.)
// - The node has no id
// - The node has no background
// - The node has no children
if (node.isContainer() && node.'*'.size() == 0 && !node.'@android:id' &&
!node.'@android:background') {
analysis << "This ${node.name()} view is useless (no children, no background, no id)"
}

View File

@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="utf-8"?>
<HorizontalScrollView
xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="fill_parent"
android:layout_height="fill_parent">
<LinearLayout
android:layout_width="fill_parent"
android:layout_height="fill_parent" />
</HorizontalScrollView>