Overall, this change looks pretty good. My comments inline.
G
On Nov 12, 2009, at 7:38 AM, [email protected] wrote:
Author: noelgrandin
Date: Thu Nov 12 12:38:13 2009
New Revision: 835368
URL: http://svn.apache.org/viewvc?rev=835368&view=rev
Log:
PIVOT-301: Add a "variableItemHeight" style to TerraListViewSkin
Modified:
incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/
TerraListViewSkin.java
Modified: incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/
terra/TerraListViewSkin.java
URL:
http://svn.apache.org/viewvc/incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java?rev=835368&r1=835367&r2=835368&view=diff
=
=
=
=
=
=
=
=
======================================================================
--- incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/
TerraListViewSkin.java (original)
+++ incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/
TerraListViewSkin.java Thu Nov 12 12:38:13 2009
@@ -63,6 +63,10 @@
private Color highlightColor;
private Color highlightBackgroundColor;
private boolean showHighlight;
+ private boolean variableItemHeight;
+ private ArrayList<Integer> variableItemHeightYCache = new
ArrayList<Integer>();
+ /* I can work out all of the heights from the YCache except for
the last one */
+ private int variableItemLastHeight;
I think you can do this all with an array, see below. I suggest
renaming the array to itemHeights. It can be set to null when
variableItemHeight is false.
private Insets checkboxPadding = new Insets(2, 2, 2, 0);
private int highlightedIndex = -1;
@@ -131,12 +135,29 @@
ListView listView = (ListView)getComponent();
List<Object> listData = (List<Object>)listView.getListData();
- preferredHeight = listData.getLength() * getItemHeight();
+ ListView.ItemRenderer itemRenderer =
listView.getItemRenderer();
+
+ if (variableItemHeight) {
+ int clientWidth = width;
+ if (listView.getCheckmarksEnabled()) {
+ clientWidth = Math.max(clientWidth -
(CHECKBOX.getWidth() + (checkboxPadding.left
+ + checkboxPadding.right)), 0);
+ }
+
+ int index = 0;
+ for (Object item : listData) {
+ itemRenderer.render(item, index++, listView, false,
false, false, false);
+ preferredHeight += itemRenderer.getPreferredHeight
(clientWidth);
+ }
+ } else {
+ preferredHeight = listData.getLength() *
getFixedItemHeight();
+ }
return preferredHeight;
}
@Override
+ @SuppressWarnings("unchecked")
public int getBaseline(int width, int height) {
ListView listView = (ListView)getComponent();
@@ -149,8 +170,20 @@
}
ListView.ItemRenderer itemRenderer = listView.getItemRenderer
();
- itemRenderer.render(null, -1, listView, false, false,
false, false);
- baseline = itemRenderer.getBaseline(clientWidth,
getItemHeight());
+ List<Object> listData = (List<Object>)listView.getListData();
+ if (variableItemHeight && listData.getLength()>0) {
There should be spaces around ">" operator. Also - we may want to
return -1 in the case where variableItemHeight is true, but
listData.getLength() returns 0, to indicate that there is no baseline.
+ itemRenderer.render(listData.get(0), 0, listView,
false, false, false, false);
+ int itemHeight = itemRenderer.getPreferredHeight
(clientWidth);
+ if (listView.getCheckmarksEnabled()) {
+ itemHeight = Math.max(CHECKBOX.getHeight() +
(checkboxPadding.top
+ + checkboxPadding.bottom), itemHeight);
+ }
+
+ baseline = itemRenderer.getBaseline(clientWidth,
itemHeight);
+ } else {
+ itemRenderer.render(null, -1, listView, false, false,
false, false);
+ baseline = itemRenderer.getBaseline(clientWidth,
getFixedItemHeight());
+ }
return baseline;
}
@@ -169,14 +202,19 @@
int width = getWidth();
int height = getHeight();
- int itemHeight = getItemHeight();
// Paint the background
if (backgroundColor != null) {
graphics.setPaint(backgroundColor);
graphics.fillRect(0, 0, width, height);
}
+
+ if (variableItemHeight) {
+ paintVariableItemHeight(graphics);
+ return;
+ }
It would be preferable to keep all the drawing code within the
existing paint() method. We don't have any other cases in the codebase
where we delegate to a "special" paint method, and I think it will be
easier to do given some slight modifications - see below.
+ int itemHeight = getFixedItemHeight();
// Paint the list contents
int itemStart = 0;
int itemEnd = listData.getLength() - 1;
@@ -247,6 +285,107 @@
}
}
+ @SuppressWarnings("unchecked")
+ private void paintVariableItemHeight(Graphics2D graphics) {
+ ListView listView = (ListView)getComponent();
+ List<Object> listData = (List<Object>)listView.getListData();
+ ListView.ItemRenderer itemRenderer =
listView.getItemRenderer();
+
+ int width = getWidth();
+
+ // Paint the list contents
+ int itemEnd = listData.getLength() - 1;
+
+ // Ensure that we only paint items that are visible
+ Rectangle clipBounds = graphics.getClipBounds();
+
+ int checkboxHeight = 0;
+ if (listView.getCheckmarksEnabled()) {
+ checkboxHeight = CHECKBOX.getHeight() +
(checkboxPadding.top
+ + checkboxPadding.bottom);
+ }
+
+ // if we haven't initialised the cache, we need to paint
everything
+ boolean computeCache = variableItemHeightYCache.getLength
() != listData.getLength();
+ int itemY = 0;
The layout() method is probably a better place to put the item height
calculation code. This method is guaranteed to be called any time the
component is flagged as invalid. If the component was invalidated by a
call to invalidateComponent() in the skin, it will also be repainted.
This way, you can rely on your item heights being valid by the time
paint() is called, because layout() will have been called first.
+
+ for (int itemIndex = 0; itemIndex <= itemEnd; itemIndex++) {
+ if (!computeCache && clipBounds!=null && itemY >
clipBounds.y + clipBounds.height) {
+ break;
+ }
+ Object item = listData.get(itemIndex);
+ boolean highlighted = (itemIndex == highlightedIndex
+ && listView.getSelectMode() !=
ListView.SelectMode.NONE);
+ boolean selected = listView.isItemSelected(itemIndex);
+ boolean disabled = listView.isItemDisabled(itemIndex);
+
+ int itemWidth = width;
+ int itemX = 0;
+
+ boolean checked = false;
+ if (listView.getCheckmarksEnabled()) {
+ checked = listView.isItemChecked(itemIndex);
+ itemX = CHECKBOX.getWidth() + (checkboxPadding.left
+ + checkboxPadding.right);
+ itemWidth -= itemX;
+ }
+
+ itemRenderer.render(item, itemIndex, listView,
selected, checked, highlighted, disabled);
+ int itemHeight = itemRenderer.getPreferredHeight
(itemWidth);
+ if (listView.getCheckmarksEnabled()) {
+ itemHeight = Math.max(itemHeight, checkboxHeight);
+ }
+
+ boolean paintItem = true;
+ if (clipBounds != null) {
+ paintItem = (itemY + itemHeight) >= clipBounds.y &&
itemY < (clipBounds.y + clipBounds.height);
+ }
+ if (paintItem) {
+ Color itemBackgroundColor = null;
+
+ if (selected) {
+ itemBackgroundColor = (listView.isFocused())
+ ? this.selectionBackgroundColor :
inactiveSelectionBackgroundColor;
+ } else {
+ if (highlighted && showHighlight && !disabled) {
+ itemBackgroundColor =
highlightBackgroundColor;
+ }
+ }
+
+ if (itemBackgroundColor != null) {
+ graphics.setPaint(itemBackgroundColor);
+ graphics.fillRect(0, itemY, width, itemHeight);
+ }
+
+ if (listView.getCheckmarksEnabled()) {
+ int checkboxY = (itemHeight - CHECKBOX.getHeight
()) / 2;
+ Graphics2D checkboxGraphics = (Graphics2D)
graphics.create(checkboxPadding.left,
+ itemY + checkboxY, CHECKBOX.getWidth(),
CHECKBOX.getHeight());
+
+ CHECKBOX.setSelected(checked);
+ CHECKBOX.setEnabled(!disabled && !
listView.isCheckmarkDisabled(itemIndex));
+ CHECKBOX.paint(checkboxGraphics);
+ checkboxGraphics.dispose();
+ }
+
+ // Paint the data
+ Graphics2D rendererGraphics = (Graphics2D)
graphics.create(itemX, itemY,
+ itemWidth, itemHeight);
+
+ itemRenderer.setSize(itemWidth, itemHeight);
+ itemRenderer.paint(rendererGraphics);
+ rendererGraphics.dispose();
+ }
+
+ if (computeCache) {
+ variableItemHeightYCache.add(itemY);
+ variableItemLastHeight = itemHeight;
+ }
+ itemY += itemHeight;
+ }
+ }
+
+
// List view skin methods
@Override
@SuppressWarnings("unchecked")
@@ -257,9 +396,17 @@
ListView listView = (ListView)getComponent();
- int index = (y / getItemHeight());
+ int index;
+ if (variableItemHeight) {
+ index = ArrayList.binarySearch
(variableItemHeightYCache, y);
+ if (index<0) {
+ index = -index-2;
Insert spaces around "<" and "-" operators.
+
+ }
+ } else {
+ index = (y / getFixedItemHeight());
+ }
List<Object> listData = (List<Object>)listView.getListData();
-
if (index >= listData.getLength()) {
index = -1;
}
@@ -269,8 +416,21 @@
@Override
public Bounds getItemBounds(int index) {
- int itemHeight = getItemHeight();
- return new Bounds(0, index * itemHeight, getWidth(),
itemHeight);
+ int itemY;
+ int itemHeight;
+ if (variableItemHeight) {
+ itemY = variableItemHeightYCache.get(index);
+ if (index < variableItemHeightYCache.getLength() - 1) {
+ itemHeight = variableItemHeightYCache.get(index+1)
- itemY;
Insert spaces around "+" operator.
+ } else {
+ itemHeight = variableItemLastHeight;
+ }
+ return new Bounds(0, itemY, getWidth(), itemHeight);
This return call doesn't appear to be necessary.
+ } else {
+ itemHeight = getFixedItemHeight();
+ itemY = index * itemHeight;
+ }
+ return new Bounds(0, itemY, getWidth(), itemHeight);
}
@Override
@@ -285,7 +445,7 @@
return itemIndent;
}
- public int getItemHeight() {
+ private int getFixedItemHeight() {
ListView listView = (ListView)getComponent();
ListView.ItemRenderer itemRenderer = listView.getItemRenderer
();
itemRenderer.render(null, -1, listView, false, false, false,
false);
@@ -568,6 +728,16 @@
setCheckboxPadding(Insets.decode(checkboxPadding));
}
+ public boolean getVariableItemHeight() {
+ return variableItemHeight;
+ }
Rename to isVariableItemHeight() ("getVariableItemHeight()" implies
that the method will return a number, especially since we also have a
"getFixedItemHeight()" method).
+
+ public void setVariableItemHeight(boolean variableItemHeight) {
+ this.variableItemHeight = variableItemHeight;
+ repaintComponent();
+ }
+
+
@Override
public boolean mouseMove(Component component, int x, int y) {
boolean consumed = super.mouseMove(component, x, y);
@@ -617,12 +787,12 @@
ListView listView = (ListView)getComponent();
List<Object> listData = (List<Object>)listView.getListData();
- int itemHeight = getItemHeight();
- int itemIndex = y / itemHeight;
+ int itemIndex = getItemAt(y);
- if (itemIndex < listData.getLength()
+ if (itemIndex != -1
+ && itemIndex < listData.getLength()
&& !listView.isItemDisabled(itemIndex)) {
- int itemY = itemIndex * itemHeight;
+ int itemY = getItemBounds(itemIndex).y;
if (!(listView.getCheckmarksEnabled()
&& x > checkboxPadding.left
@@ -693,12 +863,11 @@
List<Object> listData = (List<Object>)listView.getListData();
- int itemHeight = getItemHeight();
- int itemIndex = y / itemHeight;
+ int itemIndex = getItemAt(y);
if (itemIndex < listData.getLength()
&& !listView.isItemDisabled(itemIndex)) {
- int itemY = itemIndex * itemHeight;
+ int itemY = getItemBounds(itemIndex).y;
if (listView.getCheckmarksEnabled()
&& !listView.isCheckmarkDisabled(itemIndex)
@@ -855,11 +1024,13 @@
// List view events
@Override
public void listDataChanged(ListView listView, List<?>
previousListData) {
+ variableItemHeightYCache.clear();
The calls to clear the item height cache will not be necessary once
the height calculation is moved to layout().
invalidateComponent();
}
@Override
public void itemRendererChanged(ListView listView,
ListView.ItemRenderer previousItemRenderer) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@@ -875,6 +1046,7 @@
@Override
public void checkmarksEnabledChanged(ListView listView) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@@ -902,27 +1074,36 @@
// List view item events
@Override
public void itemInserted(ListView listView, int index) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@Override
public void itemsRemoved(ListView listView, int index, int
count) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@Override
public void itemUpdated(ListView listView, int index) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@Override
public void itemsCleared(ListView listView) {
+ variableItemHeightYCache.clear();
invalidateComponent();
}
@Override
public void itemsSorted(ListView listView) {
- repaintComponent();
+ variableItemHeightYCache.clear();
+ if (variableItemHeight) {
+ invalidateComponent();
+ } else {
+ repaintComponent();
+ }
}
// List view item state events
@@ -934,18 +1115,26 @@
// List view selection detail events
@Override
public void selectedRangeAdded(ListView listView, int
rangeStart, int rangeEnd) {
- // Repaint the area containing the added selection
- int itemHeight = getItemHeight();
- repaintComponent(0, rangeStart * itemHeight,
- getWidth(), (rangeEnd - rangeStart + 1) * itemHeight);
+ if (variableItemHeight) {
+ repaintComponent();
+ } else {
+ // Repaint the area containing the added selection
+ int itemHeight = getFixedItemHeight();
+ repaintComponent(0, rangeStart * itemHeight,
+ getWidth(), (rangeEnd - rangeStart + 1) *
itemHeight);
+ }
After you move the item height calculation to layout(), you can
probably consolidate both cases into a single case and simply repaint
the affected bounds.
}
@Override
public void selectedRangeRemoved(ListView listView, int
rangeStart, int rangeEnd) {
- // Repaint the area containing the removed selection
- int itemHeight = getItemHeight();
- repaintComponent(0, rangeStart * itemHeight,
- getWidth(), (rangeEnd - rangeStart + 1) * itemHeight);
+ if (variableItemHeight) {
+ repaintComponent();
+ } else {
+ // Repaint the area containing the removed selection
+ int itemHeight = getFixedItemHeight();
+ repaintComponent(0, rangeStart * itemHeight,
+ getWidth(), (rangeEnd - rangeStart + 1) *
itemHeight);
+ }
Same comment as above.
}
@Override
