Re: Could this be done better
On 11/24/2014 06:23, Eric Sosman wrote:
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?)
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.
- 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.
Recommended reading:
http://docs.oracle.com/javase/tutorial/uiswing/concurrency/index.html
Eric gave you all the biggies, I'll nit-pick a little.
From the docs, JFrame:
"As a convenience, the add, remove, and setLayout methods of this class
are overridden, so that they delegate calls to the corresponding methods
of the ContentPane. For example, you can add a child component to a
frame as follows:
frame.add(child);"
so you can skip the call to getContentPane().
You first call pack() and the call setSize() on your frame. Only one is
really necessary. It is often easier to use your content to control the
size of your frame. In this case making the JLabel much larger, adding
a border and then calling pack(). You can also set the preferredSize of
the frame or a component and call pack(); Getting labels and other
components to be the right size can be tricky. Using GridBagLayout as
you did, causes the component to be sized to its preferredSize and then
put in the middle of the frame. Using BorderLayout, the default, will
cause the content to fill the area of the frame that is available.
Very small nit: The frame title is a parameter of the constructor. You
could have used:
super(description) as the first line.
In updateColour() you call label.setText(""), then change the color and
then set the text again. What you were seeing with the text changing
and then the color changing were very likely artifacts from updating the
JLabel from other than the event dispatch thread.
Another very small nit: you used the older versions of color constants
with the lower case field names. In 1.4 new constants were added with
upper case names to match current convention.
Here is a JLabel that displays the current time. Looks very similar to
your example. I added an empty border to give you an example of how to
do that too.
//package com.knutejohnson.components;
import java.awt.*;
import java.awt.event.*;
import java.text.*;
import java.util.*;
import javax.swing.*;
import javax.swing.border.*;
/**
* JTimeLabel is a JLabel that displays the current time.
*
* @author Knute Johnson
*/
public class JTimeLabel extends JLabel implements ActionListener {
private final SimpleDateFormat sdf;
private final javax.swing.Timer timer = new
javax.swing.Timer(250,this);
/**
* Constructs a JTimeLabel
*
* @param text The initial text.
* @param horizontalAlignment One of the following; LEFT, CENTER,
* RIGHT, LEADING or TRAILING.
* @param pattern The time display pattern.
*
* @see JLabel
* @see SimpleDateFormat
*/
public JTimeLabel(String text, int horizontalAlignment, String
pattern) {
super(text, horizontalAlignment);
sdf = new SimpleDateFormat(pattern);
}
/**
* Constructs a JTimeLabel with a format of {@code HH:mm:ss}.
*/
public JTimeLabel() {
this("00:00:00",JLabel.CENTER,"HH:mm:ss");
}
/**
* Constructs a JTimeLabel with the specified initial text and a format
* of {@code HH:mm:ss}.
*
* @param text The initial text.
*/
public JTimeLabel(String text) {
this(text,JLabel.CENTER,"HH:mm:ss");
}
/**
* Constructs a JTimeLabel with the specified intitial text and
horizontal
* alignment.
*
* @param text The initial text.
* @param horizontalAlignment One of the following; LEFT, CENTER,
* RIGHT, LEADING or TRAILING.
*/
public JTimeLabel(String text, int horizontalAlignment) {
this(text,horizontalAlignment,"HH:mm:ss");
}
/**
* Starts the clock running
*/
public void start() {
timer.start();
}
/**
* Stops the clock
*/
public void stop() {
timer.stop();
}
public void actionPerformed(ActionEvent ae) {
setText(sdf.format(new Date()));
}
/**
* For testing only
*
* @param args the command line arguments, unused
*/
public static void main(String[] args) {
EventQueue.invokeLater(new Runnable() {
public void run() {
JFrame f = new JFrame();
f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
JTimeLabel tl = new JTimeLabel();
tl.setBorder(new EmptyBorder(40,40,40,40));
tl.setFont(new Font("Arial",Font.BOLD,32));
f.add(tl);
tl.start();
f.pack();
f.setVisible(true);
}
});
}
}
--
Knute Johnson