Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #30395

Re: Could this be done better

Show all headers | View raw

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

Back to comp.lang.java.programmer | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-24 12:16 +0100
  Re: Could this be done better "Qu0ll" <Qu0llSixFour@gmail.com> - 2014-11-24 22:49 +1100
    Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-24 17:54 +0100
      Re: Could this be done better Knute Johnson <eternal@knutejohnson.com> - 2014-11-24 09:37 -0800
        Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-24 19:27 +0100
  Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-24 09:23 -0500
    Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-24 17:45 +0100
      Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-24 13:42 -0500
        Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-25 00:12 +0100
    Re: Could this be done better Knute Johnson <eternal@knutejohnson.com> - 2014-11-24 09:19 -0800
      Re: Could this be done better Knute Johnson <eternal@knutejohnson.com> - 2014-11-24 09:41 -0800
        Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-25 00:55 +0100
      Re: Could this be done better Cecil Westerhof <Cecil@decebal.nl> - 2014-11-25 00:11 +0100
        Re: Could this be done better Knute Johnson <eternal@knutejohnson.com> - 2014-11-24 16:26 -0800
  Re: Could this be done better Brixomatic <brixomatic@yahoo.com> - 2014-11-25 19:58 +0100
    Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-25 14:36 -0500
      Re: Could this be done better Brixomatic <brixomatic@yahoo.com> - 2014-11-26 01:58 +0100
        Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-26 08:48 -0500
          Re: Could this be done better Brixomatic <brixomatic@yahoo.com> - 2014-11-29 19:43 +0100
            Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-29 15:44 -0500
              Re: Could this be done better Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-11-29 15:47 -0500

csiph-web