2
\$\begingroup\$

I am trying to simulate a window system with java. I don't know much about Java graphic-APIs, so i began with AWT (I am using Graphics2D here, so far I don't need it, but I think it will come in handy at some point). So far I just have two functions: Each window can draw its own individual content and windows are put on top if you click on them.

But I think I am reinventing the wheel with this code, so I want to know if this code uses the AWT-library correctly and how I can improve it. It seems that using repaint() doesn't look that good when putting a window on top. In the end there should be several windows with elements like text, images, animations etc., so I am not happy about redrawing everything every time. Is the AWT the correct API for this or should I look for something else?

We can also argue about window-style and names, but I know that I did no good job with that, this is only a first draft.

SimulatorWindow.java

import java.awt.Color;
import java.awt.Graphics2D;

public abstract class SimulatorWindow {
    private int x;
    private int y;
    private int width;
    private int height;

    public SimulatorWindow(int x, int y, int width, int height){
        this.setX(x);
        this.setY(y);
        this.setWidth(width);
        this.setHeight(height);
    }

    public final int getX() {
        return x;
    }

    public final void setX(int x) {
        this.x = x;
    }

    public final int getY() {
        return y;
    }

    public final void setY(int y) {
        this.y = y;
    }

    public final int getWidth() {
        return width;
    }

    public final void setWidth(int width) {
        this.width = width;
    }

    public final int getHeight() {
        return height;
    }

    public final void setHeight(int height) {
        this.height = height;
    }

    public final boolean contains(int pointX, int pointY){
        return this.x <= pointX && pointX <= this.x + this.width &&
                this.y <= pointY && pointY <= this.y + this.height;
    }

    public void paint2D(Graphics2D graphics){
        graphics.setColor(new Color(0, 0, 240, 32));
        graphics.fillRect(0, 0, getWidth(), getHeight());

        paintContent(graphics);
    }

    public abstract void paintContent(Graphics2D graphics);
}

BaseSimulatorWindow.java

import java.awt.Graphics2D;

public class BaseSimulatorWindow extends SimulatorWindow {
    public BaseSimulatorWindow(int x, int y, int width, int height){
        super(x, y, width, height);
    }

    @Override
    public void paintContent(Graphics2D graphics){
        // do nothing
    }
}

WindowSimulator.java

import java.awt.Color;
import java.awt.Frame;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import java.awt.event.WindowAdapter;
import java.awt.event.WindowEvent;
import java.util.Iterator;
import java.util.LinkedList;

public class WindowSimulator extends Frame implements MouseListener {
    private static final long serialVersionUID = -5278162214462770377L;
    private static final int MENUBAR_HEIGHT = 20;

    private class SimulatorAdapter extends WindowAdapter {
        @Override
        public void windowClosing(WindowEvent event){
            event.getWindow().dispose();
        }
    }

    private LinkedList<SimulatorWindow> simulatedWindows;

    public WindowSimulator(){
        super();
        this.addWindowListener(new SimulatorAdapter());
        this.addMouseListener(this);

        this.simulatedWindows = new LinkedList<SimulatorWindow>();
    }

    @Override
    public void paint(Graphics graphics){
        // This should never happen
        assert !(graphics instanceof Graphics2D): "WindowSimulator.paint(Graphics): Could not cast to Graphics2D";

        paint2D((Graphics2D) graphics);
    }

    public void paint2D(Graphics2D graphics){
        graphics.setColor(Color.BLACK);
        graphics.fillRect(0, 0, getWidth(), getHeight());

        synchronized (simulatedWindows){
            for (SimulatorWindow window: simulatedWindows){
                drawWindowMenu(graphics, window);

                Graphics windowGraphics =
                        graphics.create(window.getX(), window.getY(),
                                window.getWidth(), window.getHeight());

                // This should never happen
                assert !(windowGraphics instanceof Graphics2D): "WindowSimulator.paint2D(Graphics2D): Could not cast to Graphics2D";
                window.paint2D((Graphics2D) windowGraphics);
                windowGraphics.dispose();
            }
        }
    }

    protected void drawWindowMenu(Graphics2D graphics, SimulatorWindow window){
        graphics.setColor(Color.GREEN);
        graphics.drawRect(window.getX() - 1, window.getY() - MENUBAR_HEIGHT - 1,
                        window.getWidth() + 2, window.getHeight() + MENUBAR_HEIGHT + 2);
        graphics.drawLine(window.getX(), window.getY() - 1,
                        window.getX() + window.getWidth(), window.getY() - 1);
    }

    public void addSimulatorWindow(SimulatorWindow window){
        synchronized (simulatedWindows) {
            if (!simulatedWindows.contains(window)){
                simulatedWindows.add(window);
            }
        }
    }

    @Override
    public void mouseClicked(MouseEvent arg0) {}

    @Override
    public void mouseEntered(MouseEvent arg0) {}

    @Override
    public void mouseExited(MouseEvent arg0) {}

    @Override
    public void mousePressed(MouseEvent arg0) {}

    @Override
    public void mouseReleased(MouseEvent event) {
        int x = event.getX();
        int y = event.getY();

        synchronized (simulatedWindows){
            Iterator<SimulatorWindow> it = simulatedWindows.descendingIterator();

            while (it.hasNext()){
                SimulatorWindow window = it.next();

                if (window.contains(x, y)){
                    if (simulatedWindows.remove(window)) simulatedWindows.add(window);
                    repaint();
                    break;
                }
            }
        }
    }
}

Example.java (contains main-method)

import java.util.Random;


public class Example {
    private static Random random = new Random();

    public static void main(String[] args){
        WindowSimulator sim = new WindowSimulator();

        // Just put some windows at random positions on the simulator
        for (int i = 0; i < 5; i++){
            sim.addSimulatorWindow(
                new BaseSimulatorWindow(
                        randrange(0, 800, 50) + i * 10,
                        randrange(0, 600, 50) + i * 10,
                        randrange(300, 600, 50),
                        randrange(200, 400, 50)));
        }

        sim.setVisible(true);
    }

    private static int randrange(int start, int stop, int step){
        return start + step * random.nextInt((stop - start) / step);
    }
}
\$\endgroup\$
3
\$\begingroup\$

AWT vs Swing

Just in case you haven't heard of this - basically nobody is using plain AWT these days. People use Swing, with many extensions, or make use of alternative toolkits like SWT. If I were in your situation and want to code this stuff myself, I'd probably go for Swing, not AWT. If you're using AWT intentionally, then simply ignore this comment.

JDesktopPane, JInternalFrame

In case you want this in production code for the purpose of having "internal windows", you might want to have a look into JDesktopPane and JInternalFrame. However, if you're reprogramming this stuff yourself for joy, amusement, learning, fun etc., ignore this comment.

Assertions

It seems that you've never tested your assertion (run with java -ea). Assertions assert that a condition is met.

With your code

assert !(windowGraphics instanceof Graphics2D): "WindowSimulator.paint2D(Graphics2D): Could not cast to Graphics2D";

you actually assert that windowGraphics is not an instanceof Graphics2D.

What you really want is:

assert windowGraphics instanceof Graphics2D : "WindowSimulator.paint2D(Graphics2D): Could not cast to Graphics2D";

Usage by extension?

Usage by extension is a bit disputed. I'd even go so far and call it an anti-pattern, just like Singleton.

All too often, usage by extension breaks the LSP - Liskov substitution principle.

Instead of extending Frame, I'd just instantiate Frame.

In your case, extension could still make sense - but in that case, you might want to extend Component instead. If WindowSimulator were a subclass of Component, you could still do the same thing - but you could also reuse it and create i.e. two WindowSimulators in one frame.

Interface exposure

The users of WindowSimulator do not need to know that it's a MouseListener. Actually, it is not necessary that WindowSimulator is a MouseListener. In that case, WindowSimulator should rather not implements MouseListener. All you need is a MouseListener that you can use, not that "the whole world knows that "WindowSimulator is a MouseListener".

By using a MouseAdapter instead of a MouseListener, you could even shorten your code maybe.

foreach loops for reverse traversal of LinkedList

The code in mouseReleased()

    synchronized (simulatedWindows) {
        Iterator<SimulatorWindow> it = simulatedWindows.descendingIterator();

        while (it.hasNext()){
            SimulatorWindow window = it.next();

            if (window.contains(x, y)){
                if (simulatedWindows.remove(window)) simulatedWindows.add(window);
                repaint();
                break;
            }
        }
    }

can be simplified like this:

    synchronized (simulatedWindows) {
        for (SimulatorWindow window : (Iterable<SimulatorWindow>) list::descendingIterator) {
            if (window.contains(x, y)) {
                if (simulatedWindows.remove(window))
                    simulatedWindows.add(window);
                repaint();
                break;
            }
        }
    }

(That the cast is necessary is IMO a bug in the compiler or, if the compiler implements the language spec correctly, a bug in the language spec.)

synchronized on the Event Thread

I'm a bit skeptical about using synchronized on the Event Thread. It's all too easy to get the Event Thread locked up or sluggish.

In your situation it's still simple. But in general, I'd rather try to avoid synchronized on the Event Thread and do things which need synchronization on another Thread.

Names of parameters in overriding methods

If names of parameters in overriding methods match the names of the overridden methods, it's easier for users and tools like JavaDoc.

I'm refering to the names of the parameters of the MouseListener methods.

Documentation of invariants and similar constraints

The variable simulatedWindows has a very important constraint. The order of the simulatedWindows matters.

That's not self-explanatory, because quite often List is used simply because duplicates are possible, and then sometimes LinkedList is used because add, insert and remove might be more frequent than random access.

In your case, simulatedWindows is the stack of windows. The first item is the bottom most window, the last item is the top most window. It might be worth mentioning that in a comment with simulatedWindows and maybe rename that variable to stackOfSimulatedWindows.

Maintainability of mouseRelease()

When reading mouseRelease(), one has to read and understand the whole mouseRelease() method in order to understand what it does. Here's what it does:

  • It gets the coordinates from the Mouse Events.
  • It finds the topmost simulated window that contains these coordinates.
  • It makes that window the real topmost window by moving it on the top of the window stack.
  • It repaints to display the modified stack.

How about implementing mouseRelease() like this:

@Override
public void mouseReleased(final MouseEvent event) {
    synchronized (simulatedWindows) {
        moveWindowToTop(event);
    }
}

private void moveWindowToTop(final MouseEvent event) {
    moveWindowToTop(event.getX(), event.getY());
}

private void moveWindowToTop(final int x, final int y) {
    moveWindowToTop(getTopMostWindowAt(x, y));
}

private SimulatedWindow getTopMostWindowAt(final int x, final int y) {
    for (SimulatorWindow window : (Iterable<SimulatorWindow>) list::descendingIterator)
        if (window.contains(x, y))
            return window;
    return null;
}

private void moveWindowToTop(final SimulatorWindow window) {
     if (window != null && simulatedWindows.remove(window))
         simulatedWindows.add(window);
     repaint();
}

Now it's perfectly clear what mouseRelease() does.

The behavior is still the same as that of the original code. In case no window was found, or in case the window already is on top, you might want to skip repaint(), if you want to optimize for performance.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ First of all, thanks for your detailed answer, you gave me a lot to thing about. Your first hint was the most important: using Swing. I never really did anything with Swing, so I first used AWT. But my biggest concern was the ugly repaint, which existed also here: codereview.stackexchange.com/questions/74157/…, in a Swing example (codereview.stackexchange.com/questions/29630/…) this didn't happen. I really want to reprogram the whole thing (I'm focused on the window design), so using JDesktopPane etc. is not for me. \$\endgroup\$ – Sirac Jan 4 '15 at 18:46
  • 1
    \$\begingroup\$ When talking about assertions, I never really worked with them before (I mainly used jUnit) and so I tested for the wrong case. Your idea with extending Component instead of Frame is good, this may have saved me some time in the future. When it comes to Listeners, I have to think about using an inner class or implementing the interface itself. By using an inner class I have to implement several new methods like getters or setters or I use direct access, which could bring messy code. By implementing the class will look a bit ugly and I have to implement several empty methods. \$\endgroup\$ – Sirac Jan 4 '15 at 18:47
  • \$\begingroup\$ So I have to look for the lesser evil. The synchronized-block never was a problem for me, but you're right that it could cause some "unexpected behaviour". But it is neccessary here and I will keep a sharp eye on that. Also, thanks for giving ideas when it comes to names and putting the mouseRelease()-code in order. Does the final of the method parameters do anything? Performance or something like that? \$\endgroup\$ – Sirac Jan 4 '15 at 18:48
  • \$\begingroup\$ The final is just a habit of people like me who are big fans of functional programming. Whereas most people see variables as variables, functional programmers see variables as constants - not compile-time constants but once assigned they would never get re-assigned - except in rare cases. Among these people there are two subgroups - some that declare final in Java (like me) and some that think them to be final but don't declare (like Robert C. Martin). So, final simply tells the reader "I'm not gonna re-assign this, you can rely on its initial value." \$\endgroup\$ – Christian Hujer Jan 4 '15 at 18:50
  • \$\begingroup\$ Actually it is not thought to be messy when an inner class accesses members from its outer class directly. It's quite the opposite - that's what they're meant for. Otherwise you could have a separate class. You might also consider using functional programming and method references, the new features of Java 8. It doesn't help you with MouseListener because it has multiple methods, but it helps you with all those interfaces like ActionListener which have only a single method. \$\endgroup\$ – Christian Hujer Jan 4 '15 at 18:52

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.