You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pivot.apache.org by no...@apache.org on 2009/11/12 13:38:13 UTC

svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

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;
     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) {
+            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;
+        }
 
+        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;
+
+        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;
+                
+            }
+        } 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;
+            } else {
+                itemHeight = variableItemLastHeight;
+            }
+            return new Bounds(0, itemY, getWidth(), itemHeight);
+        } 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;
+    }
+
+    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();
         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);
+        }
     }
 
     @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);
+        }
     }
 
     @Override



Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Greg Brown <gk...@mac.com>.
> While working on this commit, I noted that to make full use of it,
> ListViewItemRenderer needs to have a mode where it allows the icon to
> assume it's "natural" size.
>
> But at the moment ListViewItemRenderer explicitly sets the size of the
> icon to 16x16 in it's constructor.
>
> What is the preferred solution here?

I think this is probably the best solution:

> Document that if you use the "variableItemHeight" you need to set
> preferred height and width to -1 on ListViewItemRenderer?

ListViewItemRenderer provides setIconWidth() and setIconHeight()  
methods that are a pass-through to the image view's preferred size.  
These properties can be set to -1. We could possibly provide a  
"variableHeight" boolean property that delegated to these values.



Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Noel Grandin <no...@gmail.com>.
Just checked in ListViewTest2 in the pivot/test module.


Greg Brown wrote:
> Do you have any test code you could check in? I would like to see this
> feature in action...  :-)
>
> On Nov 12, 2009, at 9:05 AM, Noel Grandin wrote:
>
>> ListViewItemEditor seems to work fine as-is.
>>
>> Greg Brown wrote:
>>> Just wondering, did you try this with the list view item editor yet? I
>>> am curious to know if the editor will need to be modified as well.
>>
>


Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Greg Brown <gk...@mac.com>.
Do you have any test code you could check in? I would like to see this  
feature in action...  :-)

On Nov 12, 2009, at 9:05 AM, Noel Grandin wrote:

> ListViewItemEditor seems to work fine as-is.
>
> Greg Brown wrote:
>> Just wondering, did you try this with the list view item editor  
>> yet? I
>> am curious to know if the editor will need to be modified as well.
>


Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Noel Grandin <no...@gmail.com>.
ListViewItemEditor seems to work fine as-is.

Greg Brown wrote:
> Just wondering, did you try this with the list view item editor yet? I
> am curious to know if the editor will need to be modified as well.


Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Greg Brown <gk...@mac.com>.
Just wondering, did you try this with the list view item editor yet? I  
am curious to know if the editor will need to be modified as well.

On Nov 12, 2009, at 7:41 AM, Noel Grandin wrote:

>
> While working on this commit, I noted that to make full use of it,
> ListViewItemRenderer needs to have a mode where it allows the icon to
> assume it's "natural" size.
>
> But at the moment ListViewItemRenderer explicitly sets the size of the
> icon to 16x16 in it's constructor.
>
> What is the preferred solution here?
> Do I remove the explicit setPreferredSize() call in the constructor -
> which might impact existing code?
> Document that if you use the "variableItemHeight" you need to set
> preferred height and width to -1 on ListViewItemRenderer?
>
> -- Noel.


Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Noel Grandin <no...@gmail.com>.
While working on this commit, I noted that to make full use of it,
ListViewItemRenderer needs to have a mode where it allows the icon to
assume it's "natural" size.

But at the moment ListViewItemRenderer explicitly sets the size of the
icon to 16x16 in it's constructor.

What is the preferred solution here?
Do I remove the explicit setPreferredSize() call in the constructor -
which might impact existing code?
Document that if you use the "variableItemHeight" you need to set
preferred height and width to -1 on ListViewItemRenderer?

-- Noel.

Re: svn commit: r835368

Posted by Greg Brown <gk...@mac.com>.
Overall, this change looks pretty good. My comments inline.
G

On Nov 12, 2009, at 7:38 AM, noelgrandin@apache.org 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
>
>


Re: svn commit: r835368 - /incubator/pivot/trunk/wtk/src/org/apache/pivot/wtk/skin/terra/TerraListViewSkin.java

Posted by Noel Grandin <no...@gmail.com>.
While working on this commit, I noted that to make full use of it,
ListViewItemRenderer needs to have a mode where it allows the icon to
assume it's "natural" size.

But at the moment ListViewItemRenderer explicitly sets the size of the
icon to 16x16 in it's constructor.

What is the preferred solution here?
Do I remove the explicit setPreferredSize() call in the constructor -
which might impact existing code?
Document that if you use the "variableItemHeight" you need to set
preferred height and width to -1 on ListViewItemRenderer?

-- Noel.