Wednesday, July 18, 2007

Reworking the reworking of the Icon demo

Java.net posted an article about the rewriting of some of the demo code in the java tutorial.

http://blogs.sun.com/thejavatutorials/entry/reworking_the_icondemo

I was not pleased with the overall quality of this "reworked" demo code. It turns out I'm not alone. Not to be one to criticize and not contribute I am posting my own revision. I'm sure someone can pick my code apart too, but I feel it's better then what sun has provided.

As far as I'm concerned demo code should do three things:
  1. Focus on the topic
  2. Be informative
  3. Exemplify best practices
To that effect I redesigned the demo a little bit. Instead of "Next" and "Previous" buttons I build a toolbar that has thumbnails of each image. This gives us the opportunity to make more Icons, which of course is the point. I had debated removing the background SwingWorker that loads the images. It just doesn't seem to be necessary to talk about that in the icon demo. In the end the "best practice" idea won out and I built a new SwingWorker that populates the buttons and creates the images and the thumbnails. Some of you more experienced Swing programmers may notice I call getScaledInstance() to create the thumbnail. I am aware that Chris Campbell has a blog entry called The Perils of Image.getScaledInstance(). In this demo though we are not resizing an image in a paint method that will be called lots of times, we do it one per image in a background thread. Performances wise I don't think it's an issue.

I have to admit I'm new to the JDK6 version of SwingWorker. I don't really like the use of Void in the parameters because I have to return null at the end of doInBackground(). The thing is all the work is done in the calls to process.

Also I've never done a webstart application so I'm not sure if I removed anything important to webstart.

All in all I think my demo more readable and concise. With spacing and javadoc comments it weighs in around 180 lines. The original was about 350 lines of code, and had almost no comments. So here is the code in all it's glory. I would gladly accept any feedback people have on how to make it even more clear for a beginner.



import java.awt.BorderLayout;
import java.awt.Image;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import javax.swing.BorderFactory;
import javax.swing.Box;
import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JToolBar;
import javax.swing.SwingUtilities;
import javax.swing.SwingWorker;

/**
 * Reworking of the IconDemoApp from the java tutorial.
 *
 * IconDemoApp.java requires the following files: <br>
 * The following files are copyright 2006 spriggs.net and licenced under a
 * Creative Commons Licence (http://creativecommons.org/licenses/by-sa/3.0/)
 * <br>
 * images/sunw01.jpg <br>
 * images/sunw02.jpg <br>
 * images/sunw03.jpg <br>
 * images/sunw04.jpg <br>
 * images/sunw05.jpg <br>
 */
public class IconDemoApp extends JFrame {

    /**
     * Hashmap to store the icons used in this example. The goal is to only load
     * them from the disk once.
     */
    private HashMap<String, ImageIcon> iconCache;
    private HashMap<String, ImageIcon> thumbnailCache;

    private JLabel photographLabel;
    private JToolBar buttonBar = new JToolBar();

    private String imagedir = "images/";

    /**
     * List of all the descriptions of the image files. These correspond one to
     * one with the image file names
     */
    private String[] imageCaptions = { "Original SUNW Logo", "The Clocktower",
            "Clocktower from the West", "The Mansion", "Sun Auditorium" };

    /**
     * List of all the image files to load.
     */
    private String[] imageFileNames = { "sunw01.jpg", "sunw02.jpg",
            "sunw03.jpg", "sunw04.jpg", "sunw05.jpg" };

    /**
     * Main entry point to the demo. Loads the Swing elements on the "Event
     * Dispatch Thread".
     *
     * @param args
     */
    public static void main(String args[]) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                IconDemoApp app = new IconDemoApp();
                app.setVisible(true);
            }
        });
    }

    /**
     * Default constructor for the demo.
     */
    public IconDemoApp() {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setTitle("Icon Demo: Please Select an Image");

        // A label for displaying the pictures
        photographLabel = new JLabel();
        photographLabel.setVerticalTextPosition(JLabel.BOTTOM);
        photographLabel.setHorizontalTextPosition(JLabel.CENTER);
        photographLabel.setHorizontalAlignment(JLabel.CENTER);
        photographLabel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));

        // we add glue on two sides so that when we add stuff in between later
        // it will be centered
        buttonBar.add(Box.createGlue());
        buttonBar.add(Box.createGlue());

        add(buttonBar, BorderLayout.SOUTH);
        add(photographLabel, BorderLayout.CENTER);

        setSize(400, 300);
        // this centers the frame on the screen
        setLocationRelativeTo(null);

        loadimages.execute();
    }

    /**
     * SwingWorker class that loads the images a background thread and published
     * when a new one is ready to be displayed.
     *
     * We use Void as the first SwingWroker param as we do not need to return anything from do in background.
     */
    private SwingWorker<Void, String> loadimages = new SwingWorker<Void, String>() {

        /**
         * Creates full size and thumbnail versions of the target image files.
         */
        @Override
        protected Void doInBackground()
                throws Exception {

            iconCache = new HashMap<String, ImageIcon>();
            thumbnailCache = new HashMap<String, ImageIcon>();

            for (int i = 0; i < imageCaptions.length; i++) {
                ImageIcon icon;
                icon = new ImageIcon(IconDemoApp.class
                        .getResource((imagedir + imageFileNames[i])));
                iconCache.put(imageCaptions[i], icon);
                thumbnailCache.put(imageCaptions[i], new ImageIcon(icon
                        .getImage().getScaledInstance(32, 32,
                                Image.SCALE_AREA_AVERAGING)));
                publish(imageCaptions[i]);
            }
            // unfortunately we must return something, and only null is valid to return when the return type is void.
            return null;
        }

        /**
         * Process all loaded images.
         */
        @Override
        protected void process(List<String> chunks) {
            for (String caption : chunks) {
                JButton thumbButton = new JButton(thumbnailCache.get(caption));
                thumbButton.setToolTipText(caption);
                thumbButton.addActionListener(new ThumbnailActionListener(
                        caption));
                // add the new button BEFORE the last glue
                // this centers the buttons in the toolbar
                buttonBar.add(thumbButton, buttonBar.getComponentCount() - 1);
            }
        }
    };

    /**
     * changes the currently displayed image
     *
     * @param caption -
     *            the caption is also the key
     */
    private void setCurrentImage(String caption) {
        photographLabel.setIcon(iconCache.get(caption));
        photographLabel.setToolTipText(caption);
        setTitle("Icon Demo: " + caption);
    }

    /**
     * Action listener that shows the image specified in it's constructor. For
     * use on the thumbnail buttons.
     */
    private class ThumbnailActionListener implements ActionListener {
        private String imageCaption;

        public ThumbnailActionListener(String caption) {
            imageCaption = caption;
        }

        @Override
        public void actionPerformed(ActionEvent e) {
            setCurrentImage(imageCaption);
        }
    };

}

12 comments:

atripp said...

A few suggestions:
* combine imports with "import java.util.*;" - best practice and also brevity.

* declare collection class variables to be of the most general type possible (i.e. make iconCache and thumbnailCache be type Map).

* get rid of the commented-out PLAF code - it's irrelevent.

* The code is just overwhelmed by the SwingWorker stuff. I'd actually take it out, at the expense of the demo actually being "wrong" because it will confuse the newbie reader.

* be consistent: You used the JDK1.5 foreach construct in process(), also use it in doInBackground().

Mr Bob Dobolina said...

It looks nice, and for "A more complex example" I think it's OK to do things the "right" way (load images in a different Thread) and SwingWorker is obviously a good choice.

However there is a bug using SwingWorker in the example.
It should be loadimages.execute();
Calling run won't do it in a different Thread.

I wouldn't extend JFrame but rather create one in the App class. (Favor composition over inheritance)

Of course we could have gone all out with classes like an ImageFactory, PhotoAlbum, PhotoPanel et.c. but I agree that the focus should probably be on how to load/show icons/images here.
To load them from different sources like a file, a web page and a jar would be a nice addition.

I would put some notification in place in the event the image files weren't downloaded with the example, or the sources can't be reached et.c. I'd say that should always be part of "best practice" examples.

@atripp: Is using wildcards in import statements really best practice?
I agree with the reasons against wildcards from this link:
http://www.javaperformancetuning.com/news/qotm031.shtml

aberrant said...

Thanks for the comments.

@Mr Bob Dobolina

Thanks for the bug catch. I have changed it to execute. We use a home grown SwingWorker here and we end up passing it to an executor. I think a more advanced demo would be a great idea. Maybe I'll attempt that soon.

@atripp:
I removed the PLAF code. I can't use the foreach loop in doInBackground() because I need the loop counter to access the two arrays.

opinali said...

atripp: No, the best practice is avoiding demand-import (.*) completely. There are even rules to catch this sin in tools like Checkstyle (see its rule "AvoidStarImport"). Rationale from Checkstyle's docs: "Importing all classes from a package leads to tight coupling between packages and might lead to problems when a new version of a library introduces name clashes.". Moreover, star imports make javac slower (resolution of identifier becomes much harder, I've implemented this myself), and it allows certain bugs (even if admittedly rare) like wanting class X which you never used before in a given source file, typing X in editor, and the editor doesn't complain because X is already imported thanks to some star import, so you accept and save it although you really want a similar class in another package (and nobody looks at the import section of sources these days, thanks to editors with "import completion" and code folding...).

cpedros said...

I know I'm stating what everyone thinks, but SUN's reliance on NetBeans in their tutorials is something that infuriates me. It's like a driving instructor saying "I'll teach you how to drive but only if your car is a Ford." However, I must admit that the whole process of seeing a badly written program reworked has probably been of more benefit than aim of the tutorial itself! As a learner working through a tutorial, you can take on board code comments that tell you why a specific piece of code exists, but you don't truly understand why until you see it done badly. Is this a practice SUN should continue to follow ;-) ?

I have the following questions (not criticisms) regarding the example. Hopefully I'm asking what other newbies would be asking:

- atripp already raised this but would it better to declare the private members iconCache and thumbnailCache using the interface rather than the Map implementation?
- Why use a LinkedHashMap?
- What are the reasons against the String arrays for imageCaptions and imageFileNames being declared as static?

aberrant said...

@cpedros - thanks for the feedback.

Why aren't the String arrays static? Humm... well I thought about it and since it's a demo and I am demonstrating generically how to do something I didn't want to make the reader think static was necessary for the example. This very simple example can use static, but thats not always the case. Is that enough of a reason? I'll think about it some more. Since there is only one instance of the app data is not duplicated anyways.

LinkedHashMap? Where? Ok you got me, it had been a LinkedHashMap because in one of my revisions I was iterating threw the keys. A LinkedHashMap guarantees the iteration order is the same as the insertion order. A regular HashMap can have the order of the keys change at any random time. In any case I no longer need that functionality and so I'll change it to a plain HashMap.

As for the declaration changing to a plain Map. I tend not to do that for member data declarations. There are times to do it, like when designing an interface or API, but only if the API is truly implementation agnostic. When I look at all the implementations of the Map interface I see very specific interaction rules. LinkedHashMap has a constant key iteration order, TreeMap is sorted, WeakHashMap does not hold on to it's keys and so on. By declaring the variable as exactly what I need I force the next person who modifies my code to think about what they assign to that variable.

If you went threw all the trouble to build a TreeMap with a string comparator that sorts in reverse alphabetical order and then someone later came along and reinitialized your Map with a HashMap things could get rather buggy if you depend on that sorted order. Declare it as a TreeMap and then someone will at least get a compile time error when trying to assign a HashMap to it.

Thanks again.

Sharon said...

Hey aberrant,

I think it's great that you actually reworked the demo.

Would you be interested in having your version featured in the tutorial? We'd run it past engineering and then replace the current demo with this one. (The text on the icon page would need some updating as well.)

Of course you would get full credit for the new version.

Let me know.

Sharon Zakhour

Chris said...

Complete JavaDoc comments would be nice.
- A sentence should end with a dot.
- @param and @return
But I share your general critic about the examples. Learners need shining examples.

Spencer said...

I'm trying to run this at home and have got the .java to compile in NetBeans. However when I run it, I get "Cannot Find File: images/Slide01.JPG" (I've changed the filename array to represent files I have in images\"

What am I doing wrong?

Thanks...

aberrant said...

@Spencer

If your images are located outside the Netbeans project then you need to use the full pathname in the array.

c:\yourfullpath\yourimage.jpg

If you use the images directory then you need to place the desired file in the Netbeans project. Since the path is "images" you need to add a package named images. Then place your images in that package.

Actually a more simplified version of this demo got into the actual java tutorial.

http://java.sun.com/docs/books/tutorial/uiswing/components/icon.html

There is a even a Netbeans project to download.

Good luck I hope you find the demo helpful.

Coldstone said...

Nice example. However, I am not sure if I am missing something, but what if someone uses this example with more than 5 images?

Seems like it would be good to keep the next/previous buttons.

aberrant said...

@Coldstone

I agree that the next and previous buttons might make sense if this was a general purpose application. My intent was to shove as much icon related code into as little a space as possible. If I was building a general purpose image viewer the code would be very different.