7
\$\begingroup\$

Preamble: the default frame positioning with Swing JFrames has always bothered me. Eventually I wrote some code to calculate my available desktop space, not counting my abnormally-large right-side-anchored taskbar, and use that information to size and position my JFrames. Then I got sick of copy/pasting the same frame size calculations into every Main.java of every pet project I make. So, I spent the better part of today writing a one-class library to centralize the functionality. I'm just doing this for practice and my own twisted sense of hard-working laziness, but it is complete and functioning code.

What I would like feedback on: the usual best practices for Java and Swing, naming of the class/methods/parameters of the public interface, and if it's not too much trouble I would also like feedback on the Javadoc I've written, since I've never written this level of documentation for professional projects.

I published a PDF of the javadoc and uploaded it so it's not necessary to read through the markup for that. It will be available here for one week.

The first thing I expect to be criticized is the name—I'm unhappy with TlcSizer, which was based on the initialism for top-level container, but FrameSizer was too specific and I'm not sure what would be both clear and concise.

TlcSizer.java:

package tlcsizer;

import java.awt.Dimension;
import java.awt.GraphicsEnvironment;
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.Window;
import javax.swing.LookAndFeel;
import javax.swing.UIManager;
import javax.swing.UnsupportedLookAndFeelException;

/**
 * {@code TlcSizer} is a utility class for Swing user interfaces to determine
 * top-level component bounds based on screen size and available screen space.
 * <p>This class is designed so that you can acquire an instance prior to
 * initializing your interface, which minimizes the impact of changing the
 * {@link LookAndFeel}. By aquiring a sizer instance and using only
 * instance methods, you can avoid the implementation details of
 * look&nbsp;and&nbsp;feel changes.
 * <p>The static methods are made available for finer control over this
 * process, though it is usually unnecessary to use them.
 * <p><b>Note:</b> this class does not implement support for multi-screen
 * environments.
 */
public final class TlcSizer {
    private final Rectangle mwb;

    /**
     * Creates a new FrameSizer. The current {@link LookAndFeel} will be
     * altered to account for objects in the native windowing system such as
     * task bars and menu bars in calculating the available screen space. After
     * performing this calculation, the look&nbsp;and&nbsp;feel will be reset to
     * the previous look&nbsp;and&nbsp;feel.
     * <p>The {@code FrameSizer} class is designed so that you can acquire an
     * instance prior to initializing your interface, which minimizes the impact
     * of changing the look&nbsp;and&nbsp;feel. This constructor is best used
     * before client code sets the look&nbsp;and&nbsp;feel.
     * <p><b>Warning:</b> this constructor alters the current
     * look&nbsp;and&nbsp;feel. This will cause strange behaviour in your
     * application if your interface has already been initialized. If this is
     * the case, you must call {@link
     * javax.swing.SwingUtilities#updateComponentTreeUI(java.awt.Component)}
     * once for each top-level container. It is recommended that you
     * re-{@link Window#pack() pack} your top-level container after this
     * operation.
     * @throws tlcsizer.TlcSizer.TlcSizerException if there was a
     * problem setting the look&nbsp;and&nbsp;feel
     */
    public TlcSizer() throws TlcSizerException {
        this.mwb = getMaxBounds(true);
    }
    /**
     * Creates a new {@code FrameSizer}, setting the look&nbsp;and&nbsp;feel as
     * specified. This constructor needs to alter the current
     * look&nbsp;and&nbsp;feel to account for objects in the native windowing
     * system such as task bars and menu bars in calculating the available
     * screen space. After performing this calculation, the specified
     * look&nbsp;and&nbsp;feel will be set.
     * <p>The {@code FrameSizer} class is designed so that you can acquire an
     * instance prior to initializing your interface, which minimizes the impact
     * of changing the look&nbsp;and&nbsp;feel. This constructor is best used
     * as a replacement for setting the look&nbsp;and&nbsp;feel in client code.
     * <p><b>Warning:</b> this constructor alters the current
     * look&nbsp;and&nbsp;feel. This will cause strange behaviour in your
     * application if your interface has already been initialized. If this is
     * the case, you must call {@link
     * javax.swing.SwingUtilities#updateComponentTreeUI(java.awt.Component)}
     * once for each top-level container. It is recommended that you
     * re-{@link Window#pack() pack} your top-level container after this
     * operation.
     * @param lookAndFeel the name of the {@link LookAndFeel} to set
     * @throws tlcsizer.TlcSizer.TlcSizerException if there was a
     * problem setting the look&nbsp;and&nbsp;feel
     */
    public TlcSizer(String lookAndFeel) throws TlcSizerException {
        this.mwb = getMaxBounds(false);
        try {
            UIManager.setLookAndFeel(lookAndFeel);
        } catch (ClassNotFoundException | InstantiationException |
                IllegalAccessException | UnsupportedLookAndFeelException ex) {
            throw new TlcSizerException(ex);
        }
    }
    /**
     * Creates a FrameSizer which relies upon a given maximum window bounds,
     * rather than calculating it internally. This eliminates the usual need for
     * a FrameSizer to change the current look&nbsp;and&nbsp;feel to the
     * system's native look&nbsp;and&nbsp;feel to calculate the bounds itself.
     * @param maximumWindowBounds the externally-provided maximum window bounds
     */
    public TlcSizer(Rectangle maximumWindowBounds) {
        this.mwb = maximumWindowBounds;
    }

    /**
     * Calculates the bounds for a top-level container of a given size such that
     * the top-level container is centred <i>on the screen</i> as best as
     * possible. The size and position are constrained to fit within available
     * space, which excludes objects in the native windowing system such as
     * task bars and menu bars.
     * @param preferredWindowSize The preferred size of the top-level container.
     * Often, this is best obtained by {@link java.awt.Window#pack() packing}
     * it and then calling {@link java.awt.Window#getPreferredSize()}.
     * @return the bounds calculated for the given top-level container size
     * @see #getSpaceCentredBounds(java.awt.Dimension) 
     * @see #getScreenCentredBounds(java.awt.Dimension, java.awt.Rectangle) 
     * @see GraphicsEnvironment#getMaximumWindowBounds() 
     */
    public Rectangle getScreenCentredBounds(Dimension preferredWindowSize) {
        return getCentredBounds(preferredWindowSize, mwb,
                ReferenceRegion.SCREEN);
    }

    /**
     * Convenience method for setting the bounds of a window after its
     * preferred, centred bounds have been determined. The window will be
     * packed, which comes with the side-effect of setting it displayable.
     * @param window the window whose bounds are to be set
     * @return {@code true} if the window's bounds were changed as a result of
     * this operation
     * @see #getScreenCentredBounds(java.awt.Dimension) 
     * @see Window#pack() 
     */
    public boolean setScreenCentredBounds(Window window) {
        return setBounds(window, mwb, ReferenceRegion.SCREEN);
    }

    /**
     * Calculates the bounds for a top-level container of a given size such that
     * the top-level container is centred <i>on the screen</i> as best as
     * possible. The size and position are constrained to fit within available
     * space, which excludes objects in the native windowing system such as
     * task bars and menu bars.
     * @param preferredWindowSize The preferred size of the top-level container.
     * Often, this is best obtained by {@link java.awt.Window#pack() packing}
     * it and then calling {@link java.awt.Window#getPreferredSize()}.
     * @param maximumWindowBounds The maximum window bounds for the screen,
     * usually obtained from {@link #getMaxBounds(boolean)}.
     * @return the bounds calculated for the given top-level container size
     * @see #getSpaceCentredBounds(java.awt.Dimension, java.awt.Rectangle) 
     * @see #getScreenCentredBounds(java.awt.Dimension)
     * @see GraphicsEnvironment#getMaximumWindowBounds() 
     */
    public static Rectangle getScreenCentredBounds(
            Dimension preferredWindowSize, Rectangle maximumWindowBounds) {
        return getCentredBounds(preferredWindowSize, maximumWindowBounds,
                ReferenceRegion.SCREEN);
    }

    /**
     * Convenience method for setting the bounds of a window after its
     * preferred, centred bounds have been determined. The window will be
     * packed, which comes with the side-effect of setting it displayable.
     * @param window the window whose bounds are to be set
     * @param maximumWindowBounds The maximum window bounds for the screen,
     * usually obtained from {@link #getMaxBounds(boolean)}.
     * @return {@code true} if the window's bounds were changed as a result of
     * this operation
     * @see #getScreenCentredBounds(java.awt.Dimension) 
     * @see Window#pack() 
     */
    public static boolean setScreenCentredBounds(Window window,
            Rectangle maximumWindowBounds) {
        return setBounds(window, maximumWindowBounds, ReferenceRegion.SCREEN);
    }

    /**
     * Calculates the bounds for a top-level container of a given size such that
     * the top-level container is centred <i>within the available space</i> as
     * best as possible. These bounds account for objects in the native
     * windowing system such as task bars and menu bars.
     * @param preferredWindowSize The preferred size of the top-level container.
     * Often, this is best obtained by {@link java.awt.Window#pack() packing}
     * it and then calling {@link java.awt.Window#getPreferredSize()}.
     * @return the bounds calculated for the given top-level container size
     * @see #getScreenCentredBounds(java.awt.Dimension) 
     * @see #getSpaceCentredBounds(java.awt.Dimension, java.awt.Rectangle) 
     * @see GraphicsEnvironment#getMaximumWindowBounds() 
     */
    public Rectangle getSpaceCentredBounds(Dimension preferredWindowSize) {
        return getCentredBounds(preferredWindowSize, mwb,
                ReferenceRegion.AVAILABLE_SPACE);
    }

    /**
     * Convenience method for setting the bounds of a window after its
     * preferred, centred bounds have been determined. The window will be
     * packed, which comes with the side-effect of setting it displayable.
     * @param window the window whose bounds are to be set
     * @return {@code true} if the window's bounds were changed as a result of
     * this operation
     * @see #getSpaceCentredBounds(java.awt.Dimension) 
     * @see Window#pack() 
     */
    public boolean setSpaceCentredBounds(Window window) {
        return setBounds(window, mwb, ReferenceRegion.AVAILABLE_SPACE);
    }

    /**
     * Calculates the bounds for a top-level container of a given size such that
     * the top-level container is centred <i>within the available space</i> as
     * best as possible. These bounds account for objects in the native
     * windowing system such as task bars and menu bars.
     * @param preferredWindowSize The preferred size of the top-level container.
     * Often, this is best obtained by {@link java.awt.Window#pack() packing}
     * it and then calling {@link java.awt.Window#getPreferredSize()}.
     * @param maximumWindowBounds The maximum window bounds for the screen,
     * usually obtained from {@link #getMaxBounds(boolean)}.
     * @return the bounds calculated for the given top-level container size
     * @see #getScreenCentredBounds(java.awt.Dimension, java.awt.Rectangle) 
     * @see #getSpaceCentredBounds(java.awt.Dimension) 
     */
    public static Rectangle getSpaceCentredBounds(
            Dimension preferredWindowSize, Rectangle maximumWindowBounds) {
        return getCentredBounds(preferredWindowSize, maximumWindowBounds,
                ReferenceRegion.AVAILABLE_SPACE);
    }

    /**
     * Convenience method for setting the bounds of a window after its
     * preferred, centred bounds have been determined. The window will be
     * packed, which comes with the side-effect of setting it displayable.
     * @param window the window whose bounds are to be set
     * @param maximumWindowBounds The maximum window bounds for the screen,
     * usually obtained from {@link #getMaxBounds(boolean)}.
     * @return {@code true} if the window's bounds were changed as a result of
     * this operation
     * @see #getSpaceCentredBounds(java.awt.Dimension) 
     * @see Window#pack() 
     */
    public static boolean setSpaceCentredBounds(Window window,
            Rectangle maximumWindowBounds) {
        return setBounds(window, maximumWindowBounds,
                ReferenceRegion.AVAILABLE_SPACE);
    }

    private static boolean setBounds(Window window, Rectangle mwb,
            ReferenceRegion region) {
        window.pack();
        final Rectangle newBounds = getCentredBounds(window.getPreferredSize(),
                mwb, region);
        final boolean wasTlcModified = newBounds.equals(window.getBounds());
        if (wasTlcModified) {
            window.setBounds(newBounds);
        }
        return wasTlcModified;
    }

    private static Rectangle getCentredBounds(Dimension preferredSize,
            Rectangle mwb, ReferenceRegion region) {
        final Rectangle newBounds;
        final Dimension dFrame = new Dimension(
                Math.min(mwb.width, preferredSize.width),
                Math.min(mwb.height, preferredSize.height));
        final Point pFrame = new Point();
        final int x;
        final int y;
        switch (region) {
            case SCREEN:
                final Dimension screen =
                        java.awt.Toolkit.getDefaultToolkit().getScreenSize();
                // calculate ideal position
                x = (screen.width - dFrame.width) / 2;
                y = (screen.height - dFrame.height) / 2;
                // constrain position to avoid clipping OS desktop decorations
                pFrame.x = constrain(x, mwb.x, mwb.x + mwb.width);
                pFrame.y = constrain(y, mwb.y, mwb.y + mwb.height);
                break;
            case AVAILABLE_SPACE:
                x = mwb.x + (mwb.width - dFrame.width) / 2;
                y = mwb.y + (mwb.height - dFrame.height) / 2;
                // unnecessary to constrain since we used mwb for ideal position
                assert x == constrain(x, mwb.x, mwb.x + mwb.width);
                assert y == constrain(y, mwb.y, mwb.y + mwb.height);
                break;
            default:
                throw new RuntimeException("switch enum hit default case");
        }
        return new Rectangle(pFrame, dFrame);
    }

    private static int constrain(int value, int min, int max) {
        if (max < min) throw new ArithmeticException();
        value = Math.min(value, max);
        value = Math.max(value, min);
        return value;
    }

    /**
     * This method obtains the maximum window bounds from the local
     * {@link GraphicsEnvironment} via
     * {@link GraphicsEnvironment#getMaximumWindowBounds()
     * getMaximumWindowBounds()}. This method changes the
     * look&nbsp;and&nbsp;feel to the native system look&nbsp;and&nbsp;feel,
     * which is necessary for an accurate result.
     * @param changeLnfBackAfterwards whether to change the
     * look&nbsp;and&nbsp;feel back to the previously set
     * look&nbsp;and&nbsp;feel after the maximum bounds have been obtained.
     * @return the maximum bounds
     * @throws tlcsizer.TlcSizer.TlcSizerException if there was a
     * problem setting the look&nbsp;and&nbsp;feel
     */
    public static Rectangle getMaxBounds(boolean changeLnfBackAfterwards)
            throws TlcSizerException {
        LookAndFeel originalLnf = UIManager.getLookAndFeel();
        try {
            UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
        } catch (ClassNotFoundException | InstantiationException |
                IllegalAccessException | UnsupportedLookAndFeelException ex) {
            throw new TlcSizerException(ex);
        }
        // Needs to be done with the platform-dependent look and feel active
        Rectangle mwb = GraphicsEnvironment.getLocalGraphicsEnvironment()
                .getMaximumWindowBounds();
        if (changeLnfBackAfterwards) {
            try {
                UIManager.setLookAndFeel(originalLnf);
            } catch (UnsupportedLookAndFeelException ex) {
                throw new TlcSizerException(ex);
            }
        }
        return mwb;
    }

    private static enum ReferenceRegion { SCREEN, AVAILABLE_SPACE }

    /**
     * Thrown when a {@link TlcSizer} encounters an exception when attempting
     * to set the look&nbsp;and&nbsp;feel. The cause is set to the original
     * exception encountered, which may be any of those thrown by
     * {@link UIManager#setLookAndFeel(java.lang.String)}.
     * @see Exception#getCause()
     */
    public final static class TlcSizerException extends Exception {
        private TlcSizerException(Throwable cause) { super(cause); }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Wells, +1 for comments on public methods, it will be nicer for developers/maintainers if you do the same for your private ones too. :) \$\endgroup\$ – h.j.k. Aug 27 '14 at 6:03
  • 1
    \$\begingroup\$ @h.j.k. That's a good point. I have a tendency to get lazy with private and/or internal names; rolfl points out a similar sentiment in the opening paragraph of his answer. I will work on breaking this habit. \$\endgroup\$ – muffin Aug 27 '14 at 23:22
5
\$\begingroup\$

General

Your variable names are not helpful enough. I understand that mwb is maximum window bounds, but it is still too short.

In general, I worry that you have too many different ways to access the core logic. Too many public methods.

Further, some of your methods require a TlcSizer instance, but others are just public static methods. This inconsistency is confusing.

On the other hand, the JavaDoc is neat and consistent. I like the detail you have (but I am a lots-of-detail-person, I know other people who will think you have overdone it). I can tell you have paid attention to paragraphs, and other formatting in the JavaDoc.

Out of interest, when you have a <p> in the JavaDoc, I suggest also adding an empty line to make it a paragraph in the code format too, and also have an empty line before the @param sections. For example, your JavaDoc:

/**
 * Creates a new {@code FrameSizer}, setting the look&nbsp;and&nbsp;feel as
 * specified. This constructor needs to alter the current
 * look&nbsp;and&nbsp;feel to account for objects in the native windowing
 * system such as task bars and menu bars in calculating the available
 * screen space. After performing this calculation, the specified
 * look&nbsp;and&nbsp;feel will be set.
 * <p>The {@code FrameSizer} class is designed so that you can acquire an
 * instance prior to initializing your interface, which minimizes the impact
 * of changing the look&nbsp;and&nbsp;feel. This constructor is best used
 * as a replacement for setting the look&nbsp;and&nbsp;feel in client code.
 * <p><b>Warning:</b> this constructor alters the current
 * look&nbsp;and&nbsp;feel. This will cause strange behaviour in your
 * application if your interface has already been initialized. If this is
 * the case, you must call {@link
 * javax.swing.SwingUtilities#updateComponentTreeUI(java.awt.Component)}
 * once for each top-level container. It is recommended that you
 * re-{@link Window#pack() pack} your top-level container after this
 * operation.
 * @param lookAndFeel the name of the {@link LookAndFeel} to set
 * @throws tlcsizer.TlcSizer.TlcSizerException if there was a
 * problem setting the look&nbsp;and&nbsp;feel
 */

would be better if written:

/**
 * Creates a new {@code FrameSizer}, setting the look&nbsp;and&nbsp;feel as
 * specified. This constructor needs to alter the current
 * look&nbsp;and&nbsp;feel to account for objects in the native windowing
 * system such as task bars and menu bars in calculating the available
 * screen space. After performing this calculation, the specified
 * look&nbsp;and&nbsp;feel will be set.
 * <p>
 * The {@code FrameSizer} class is designed so that you can acquire an
 * instance prior to initializing your interface, which minimizes the impact
 * of changing the look&nbsp;and&nbsp;feel. This constructor is best used
 * as a replacement for setting the look&nbsp;and&nbsp;feel in client code.
 * <p>
 * <b>Warning:</b> this constructor alters the current
 * look&nbsp;and&nbsp;feel. This will cause strange behaviour in your
 * application if your interface has already been initialized. If this is
 * the case, you must call {@link
 * javax.swing.SwingUtilities#updateComponentTreeUI(java.awt.Component)}
 * once for each top-level container. It is recommended that you
 * re-{@link Window#pack() pack} your top-level container after this
 * operation.
 * 
 * @param lookAndFeel the name of the {@link LookAndFeel} to set
 * @throws tlcsizer.TlcSizer.TlcSizerException if there was a
 * problem setting the look&nbsp;and&nbsp;feel
 */

Bugs

In this method I believe you have a bug:

private static boolean setBounds(Window window, Rectangle mwb,
        ReferenceRegion region) {
    window.pack();
    final Rectangle newBounds = getCentredBounds(window.getPreferredSize(),
            mwb, region);
    final boolean wasTlcModified = newBounds.equals(window.getBounds());
    if (wasTlcModified) {
        window.setBounds(newBounds);
    }
    return wasTlcModified;
}

The boolean wasTlcModified should be the negated value of newBounds.equals(window.getBounds()).

A second bug, in private static Rectangle getCentredBounds(Dimension, Rectangle, ReferenceRegion), you have the option of AVAILABLE_SPACE as the region. From what I can see, that option does nothing. It sets the values of the x and y variables, but then does nothing with those values. What is the point?

Style

1-liners (even simple ones) should have {} braces:

if (max < min) throw new ArithmeticException();

should be:

if (max < min) {
    throw new ArithmeticException();
}

Additionally, adding a useful message would be good:

if (max < min) {
    throw new ArithmeticException(String.format(
        "Specified min value %d is larger than the specified max value %d", min, max));
}

Actually, if it was me, I would just remove the check since I have full control over the code, and I can't see how the condition is violated. But, you have assert statements in other places so I am not sure why you don't just assert this condition too. Consistency is always important. Hmm, while looking at the method, it may as well just be:

private static int constrain(int value, int min, int max) {
    return Math.max(Math.min(value, max), min);
}

Your TlcException is.... unnecessary. You may as well just declare that your method throws a plain Exception. If it was me, I would at least make the TlcException a RuntimeException so there are not checked exceptions coming out.

Further, the constructor does not take a message, and is all on one line... ouch:

private TlcSizerException(Throwable cause) { super(cause); }
\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to review my code. I'll do my best to learn from it. Though the StackExchange format doesn't really encourage it, I hope I'm not being too unconventional by responding in depth: \$\endgroup\$ – muffin Aug 27 '14 at 23:23
  • \$\begingroup\$ General #2/3: "In general, I worry that you have too many different ways to access the core logic... Further, some of your methods require a TlcSizer instance, but others are just public static methods. This inconsistency is confusing." This was something I felt somewhat wishywashy about. In the spirit of your comment, I will use the static getters, chop out the static setters, and eliminate the instantiables altogether. This makes #1 (member variable name unhelpful) a moot point, but a point taken nonetheless. \$\endgroup\$ – muffin Aug 27 '14 at 23:23
  • \$\begingroup\$ General #4/5: Thanks for the positive feedback on my Javadoc. It feels dirty to ask for feedback on something I am proud of, but it's easy to start second-guessing oneself when nobody else is seeing one's work. I also hear what you've said about a preference for details, and absences thereof. I like your newline tips, too, and I think I will incorporate them into my style. \$\endgroup\$ – muffin Aug 27 '14 at 23:23
  • \$\begingroup\$ Bugs: Yes, you are correct on both counts. The former is as simple as you point out. The latter error is my forgetting to assign those values to pFrame because of a sloppy copy/paste/modify. My only tested use-case never explored the AVAILABLE_SPACE case, or I would have seen that the method was returning a Rectangle originating at (0,0) regardless of input. \$\endgroup\$ – muffin Aug 27 '14 at 23:24
  • \$\begingroup\$ Style: On one-liners, I agree. Further laziness on my part, though I believe in the reasons for avoiding such blockless if statements, per Steve McConnell. As far as throwing exceptions and using assertions goes, this is something I've never really been comfortable with. Where is the line between too much error-checking and too little? I have a vague idea, since I cross it both ways frequently... \$\endgroup\$ – muffin Aug 27 '14 at 23:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.