Re: Could this be done better
Op Monday 24 Nov 2014 15:23 CET schreef Eric Sosman:
On 11/24/2014 6:16 AM, Cecil Westerhof wrote:
I have mostly programmed back-ends. But I now want also to do some
front-end programming.
I wrote the following simple program. It does nothing useful, it
just displays a description in different colours with the colour
appended to the description.
I was just wondering if this could be done better. Especially
???updateColour???. First I did not removed the text, but then I could
see the colour changing before the text was changed. This way I
find more pleasing and I do not notice that the text disappears.
One rather important point you've overlooked: Swing is (mostly)
not thread-safe. Swing methods should only be invoked on the Event
Dispatching Thread (EDT), except for the very few methods that are
specifically documented as safe for use from other threads.
Your program violates this rule in two ways:
- The ColourCycling constructor builds a JFrame and invokes
assorted methods on it, and also builds a JLabel -- but the
constructor is called from a thread that is not the EDT.
- The updateColour() method manipulates the JLabel, but is
called from some random thread created by Timer -- again, not
the EDT.
The first problem is one of those "it almost always works
right" issues, the kind that will not bite you -- until the demo
for the Big Boss and the Big Customer. (Sun themselves did not
at first realize it was a problem, and many early Swing examples
illustrated exactly the same mistake you've made.) Fortunately,
it's easy to fix: Use the SwingUtilities.invokeLater() method to
run the ColourCycling constructor on the EDT instead of on the
initial thread. (For clarity's sake, I'd suggest moving the
JLabel construction inside the ColourCycling constructor, too:
That's where the byte code winds up, but let's be explicit, OK?)
Done.
The second problem is (most likely) the cause of the strange
display glitches you refer to: You're updating the display from
a non-EDT thread, and the EDT doesn't "know" right away that things
have changed. There are at least two ways to cure it:
- Inside the TimerTask, use SwingUtilities.invokeLater() to
launch a Runnable that calls updateColour(), so that the call runs
on the EDT instead of on the Timer's thread.
I choose for this solution: was the easiest to do. ;-)
- Use javax.swing.Timer instead of java.util.Timer. The
mechanics are a bit different -- you write an ActionListener
instead of subclassing TimerTask -- but the action listener
automatically runs on the EDT, where it's safe to manipulate
your GUI.
Or is there a reason why did would be better?
It looks OK now. The code has become:
import java.awt.*;
import java.util.*;
import javax.swing.*;
@SuppressWarnings("serial")
public class ColourCycling extends JFrame {
// public functions
public ColourCycling() {
java.util.Timer colourTimer = new java.util.Timer();
getContentPane().setBackground(background);
setLayout(new GridBagLayout());
label = new JLabel(description + " : " + "XXXXXXXXXXXXXXX");
label.setFont(new Font("serif", Font.BOLD, 25));
add(label);
pack();
setSize(getWidth(), 3 * getHeight());
label.setBackground(background);
updateColour(getCurrentColour());
colourTimer.schedule(new SwitchColour(), delay, delay);
setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
setTitle(description);
setVisible(true);
}
public static void main(String[] args) {
SwingUtilities.invokeLater(new Runnable() {
public void run() {
new ColourCycling();
}
});
}
// private classes
private class Colour {
// public
public Colour(Color color, String description) {
this.color = color;
this.description = description;
}
public Color getColor() {
return color;
}
public String getDescription() {
return description;
}
// private
private Color color;
private String description;
}
private class SwitchColour extends TimerTask {
public void run() {
SwingUtilities.invokeLater(new Runnable() {
public void run() {
updateColour(getNextColour());
}
});
}
}
//private functions
private Colour getCurrentColour() {
return colours[currentColour];
}
private Colour getNextColour() {
currentColour = (currentColour + 1) % colours.length;
return getCurrentColour();
}
private void updateColour(Colour newColour) {
label.setForeground(newColour.getColor());
label.setText(description + " : " + newColour.getDescription());
}
// private variables
private Color background = Color.lightGray;
private Colour[] colours = new Colour[] {
new Colour(Color.blue, "Blue"),
new Colour(Color.red, "Red"),
new Colour(Color.orange, "Orange"),
new Colour(Color.yellow, "Yellow"),
new Colour(Color.green, "Green"),
new Colour(Color.cyan, "Cyan"),
new Colour(Color.magenta, "Magenta"),
new Colour(new Color(0x9C, 0x5D, 0x52), "Brown"),
};
private int currentColour = 0;
private int delay = 4000;
private String description = "JTextArea Color Change Example";
private JLabel label;
}
By the way, the reason I use:
@SuppressWarnings("serial")
is that I compile with:
javac -Xlint ColourCycling.java
Well, I need to make a more interesting program.
Recommended reading:
http://docs.oracle.com/javase/tutorial/uiswing/concurrency/index.html
My reading list just became a little bigger again. :-D
--
Cecil Westerhof
Senior Software Engineer
LinkedIn: http://www.linkedin.com/in/cecilwesterhof