Iterate / recurse through Containers and Components to find objects of a given class?

Question

I've written a MnemonicsBuilder class for JLabels and AbstractButtons. I would like to write a convenience method setMnemonics( JFrame f ) that will iterate through every child of the JFrame and select out the JLabels and AbstractButtons. How can I obtain access to everything contained in the JFrame? I've tried:

LinkedList<JLabel> harvestJLabels( Container c, LinkedList<JLabel> l ) {
    Component[] components = c.getComponents();
    for( Component com : components )
    {
     if( com instanceof JLabel )
     {
      l.add( (JLabel) com );
     } else if( com instanceof Container )
     {
      l.addAll( harvestJLabels( (Container) com, l ) );
     }
    }
    return l;
}

In some situations, this works just fine. In others, it runs out of memory. What am I not thinking of? Is there a better way to search for child components? Is my recursion flawed? Is this not a picture of how things "Contain" other things in Swing - e.g., is Swing not a Rooted Tree?

JFrame
|
|\__JMenuBar
|   |
|    \__JMenu
|       |
|        \__JMenuItem
|
|\__JPanel
|   |
|   |\__JButton 
|   |
|   |\__JLabel
|   |
|   |\__ ... JCheckBoxes, other AbstractButtons, etc.

Answer 1

What if you had two components that each had one another in their components collection? It would infinetly recurse through them, adding to your collection.

You could have a circular reference somewhere, that may not be as obvious or simple as what I described. I'm not familiar with JFrame's, so I'm not sure if this is possible.

When doing this sort of thing you might need a "visited" property of some sort, so that you can mark objects as visited, and if so, don't do the recursive call on them.

Answer 2

You can try to remove recursion to release the arrays of components:

LinkedList<JLabel> harvestJLabels( Container c, LinkedList<JLabel> l ) {
    List<Container> containers = new ArrayList<Container();
    containers.add(c);
    while (!containers.isEmpty()) {
        Container cont = containers.remove(0);
        Component[] components = cont.getComponents();
        for( Component com : components )
        {
            if( com instanceof JLabel )
            {
                    l.add( (JLabel) com );
            } else if( com instanceof Container )
            {
                containers.add((Container)com);
            }
        }
    }
    return l;
}

Answer 3

Here's your problem:

LinkedList<JLabel> harvestJLabels( Container c, LinkedList<JLabel> l ) {
    ...
                l.addAll( harvestJLabels( (Container) com, l ) );
    ...
    return l;
}

You only have a single list. You are appending a list to another list. Therefore you are adding a list to itself. That may work in some sense, but you are going to have a doubling of length (exponential growth).

Either have a single List (usually there is no need to specify an algorithm in declarations) or create a new list instance each time the method is called. Avoid returning a reference you don't need to - it's just misleading.

Also ArrayList would be more appropriate than LinkedList. LinkedList is almost always the wrong choice.

Answer 4

Agree with Tom here... Your problem is that you're already passing the List to add the JLabels down to your recursive method AND you're also returning it - thus adding the same items to your list more than once. In more politically correct terms - the List is your accumulator.

Your method should instead look like this:

public void harvestJLabels(Container c, List<JLabel> l) {
    Component[] components = c.getComponents();
    for(Component com : components) {
        if(com instanceof JLabel) {
            l.add((JLabel) com);
        } else if(com instanceof Container) {
            harvestJLabels((Container) com, l));
        }
    }
}

Then you can have a helper method to initiate this harvesting:

public List<JLabel> harvestJLabels(Container c) {
    List<JLabel> jLabels = new ArrayList<JLabel>();
    harvestJLabels(c, jLabels);
    return jLabels;
}