PanelScrollable - a reusable class for rendering a changeable number of panels

Question

This class (PanelScrollable) is part of my view package, in an MVP designed Java Swing system. I am not using any MVP frameworks, it's all custom.

The reason I'm posting this code is that I fear too much of the rendering logic is directly accessing Swing components, making it difficult to unit test; as a result this class has probably had more bugs making it to production than any other single class in the entire application.

What I'd like to do is tease apart the logic that decides what actions should be done on Swing components from the actual calls to Swing methods, but I'm finding that a bit challenging.

First, some custom classes you need to understand the PanelScrollable class. These are used to encapsulate Swing behavior away from the Presenter classes, generally speaking. Provider is from Guice.

public interface Provider<T> {
    T get();
}

abstract class Componentable {
    abstract Component getComponent();
}

public abstract class DataComponentable<E> extends Componentable {
    public abstract void setData(E data);
    public abstract boolean isEnabled();
    public abstract void setEnabled(boolean enabled);
}

And here's PanelScrollable:

import static com.google.common.base.Preconditions.checkArgument;

import java.awt.Component;
import java.awt.Dimension;
import java.awt.Rectangle;
import java.util.*;

import javax.swing.*;
import javax.swing.border.EmptyBorder;

import com.google.common.collect.Lists;
import com.google.inject.Provider;

abstract class PanelScrollable<E> extends Componentable {
    private static final int STRUT_SIZE = 2;

    private JScrollPane outerPane;
    private ScrollPanel innerPanel;

    private List<DataComponentable<E>> panels = Lists.newArrayList();
    private List<Component> struts = Lists.newArrayList();

    private Provider<? extends DataComponentable<E>> panelFactory;

    PanelScrollable(Provider<? extends DataComponentable<E>> panelFactory) {
        this.panelFactory = panelFactory;

        innerPanel = new ScrollPanel();

        outerPane = new JScrollPane(innerPanel);
        outerPane.setVerticalScrollBarPolicy(ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);

        addPanel();
    }

    // In general this is the contract of Componentable, to allow for Swing encapsulation
    @Override
    final Component getComponent() {
        return outerPane;
    }

    private void addPanelAndStrut() {
        synchronized(innerPanel.getTreeLock()) {
            Component newStrut = Box.createVerticalStrut(STRUT_SIZE); 
            struts.add(newStrut);
            innerPanel.add(newStrut);

            addPanel();
        }
    }

    private void addPanel() {
        synchronized(innerPanel.getTreeLock()) {
            DataComponentable<E> newPanel = panelFactory.get();
            innerPanel.add(newPanel.getComponent());
            panels.add(newPanel);
        }
    }

    public void setData(Collection<E> data) {
        boolean dirty = false;

        dirty = addAndUpdatePanels(data, dirty);
        dirty |= removeExtraPanels(data.size());
        if(dirty) {
            redrawPanels();
        }
    }

    private void redrawPanels() {
        innerPanel.validate();
        innerPanel.repaint();
    }

    private boolean addAndUpdatePanels(Collection<E> data, boolean dirty) {
        Iterator<E> it = data.iterator();
        int i = 0;
        while(it.hasNext()) {
            if(notEnoughPanels(i)) {
                addPanelAndStrut();
                dirty = true;
            }
            E next = it.next();
            panels.get(i).setData(next);
            i++;
        }
        return dirty;
    }

    private boolean notEnoughPanels(int i) {
        return i == panels.size();
    }

    private final boolean removeExtraPanels(int index) {
        checkArgument(index > -1, "Cannot remove more panels than exist!");
        if(index >= panels.size()) return false;
        synchronized(innerPanel.getTreeLock()) {
            do {
                removePanelAndStrut();
            } while(index >= panels.size());

            if(index == 0) addPanel();
        }
        return true;
    }

    private void removePanelAndStrut() {
        synchronized(innerPanel.getTreeLock()) {
            DataComponentable<E> removedPanel = panels.remove(panels.size() - 1);
            removedPanel.setEnabled(false);
            innerPanel.remove(removedPanel.getComponent());

            // This is where the bug WAS that made me decide to post this class on code review
            // if setData was called with an empty collection, this code would (previously)
            // call struts.remove(-1)
            if(struts.size() > 0) {
                Component removedStrut = struts.remove(struts.size() - 1);
                innerPanel.remove(removedStrut);
            } else if (panels.size() > 0) {
                throw new IllegalStateException(
                        String.format("There are too many panels (%d) and no struts!",
                                panels.size()));
            }
        }
    }

    private class ScrollPanel extends JPanel implements Scrollable {
        private static final long serialVersionUID = 3823479389541763224L;

        public ScrollPanel() {
            setLayout(new BoxLayout(this, BoxLayout.Y_AXIS));
            setBorder(new EmptyBorder(3,3,3,3));
        }

        @Override
        public final Dimension getPreferredScrollableViewportSize() {
            return getPreferredSize();
        }

        @Override
        public final int getScrollableUnitIncrement(Rectangle visibleRect,
                int orientation, int direction) {
            Dimension dim = panels.get(0).getComponent().getPreferredSize();
            int size;
            if (orientation == SwingConstants.HORIZONTAL) {
                size = dim.width;
            } else {
                int height = dim.height + STRUT_SIZE;
                if (direction > 0)
                    size = height - visibleRect.y % (height);
                else
                    // 1 to height, not 0 to height - 1
                    size = (visibleRect.y + visibleRect.height - 1) % (height) + 1;
            }
            return size;
        }

        @Override
        public final int getScrollableBlockIncrement(Rectangle visibleRect,
                int orientation, int direction) {
            Dimension dim = panels.get(0).getComponent().getPreferredSize();
            int size;
            if (orientation == SwingConstants.HORIZONTAL) {
                size = dim.width;
            } else {
                int height = dim.height + STRUT_SIZE;
                size = (visibleRect.height / height) * height;
            }
            return size;
        }

        @Override
        public final boolean getScrollableTracksViewportWidth() {
            return true;
        }

        @Override
        public final boolean getScrollableTracksViewportHeight() {
            if(outerPane.getViewportBorderBounds().height > getMinimumSize().height) {
                return true;
            }
            return false;
        }
    }
}

Answer 1

This code is awesomely written. I still have a few really small remarks:

Ordering members:

If openness separates concepts, then vertical density implies close association. So lines of code that are tightly related should appear vertically dense.
- Robert C. Martin, Clean Code, Ch. 5 - Vertical Density (p. 79)

When I skimmed your code from top to bottom (as usually done), I instantly wondered: addPanelAndStrut() - Where does this get called, why is there no parameter and why is it private?

btw. It gets called 5 methods later, and only there... I prefer to have the called method below the calling method, this makes skimming the code much easier, as you don't have to scroll up to see what the call to method xy does.

Reorder your methods a little. Usually you have from top to bottom:
fields, constructors, public methods, private methods, getters and setters

Unneeded parameters:

Your addAndUpdatePanels is only called once, namely from within setData. And It's always called with dirty = false;

Why are you passing in dirty? You can assume that it's not dirty for the purpose of your method. You only need to return whether your method dirties the paint area. The method needn't concern itself with the current state of the paint-area. Remove the parameter and instead just call addAndUpdatePanels(data);

Multiline Conditionals:

This may be only my preference, but I really prefer to assign booleans only once, or rather as seldom as possible.

I'd rewrite your setData as follows:

public void setData(Collection<E> data) {
    boolean dirty = false;

    dirty = addAndUpdatePanels(data) || removeExtraPanels(data.size());
    if(dirty) {
        redrawPanels();
    }
}

Finally:

Very nicely and descriptively named variables and methods make reading your class a joy ;) Keep it up. The ordering of your methods is a little confusing to me. All else equal: Good work!