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);
}
}
1 Answer 1
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 WindowSimulator
s 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.
for
each 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.
-
\$\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– Sirac2015年01月04日 18:46:46 +00:00Commented Jan 4, 2015 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 ofFrame
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– Sirac2015年01月04日 18:47:06 +00:00Commented Jan 4, 2015 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 themouseRelease()
-code in order. Does thefinal
of the method parameters do anything? Performance or something like that? \$\endgroup\$Sirac– Sirac2015年01月04日 18:48:14 +00:00Commented Jan 4, 2015 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 declarefinal
in Java (like me) and some that think them to befinal
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– Christian Hujer2015年01月04日 18:50:40 +00:00Commented Jan 4, 2015 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 likeActionListener
which have only a single method. \$\endgroup\$Christian Hujer– Christian Hujer2015年01月04日 18:52:27 +00:00Commented Jan 4, 2015 at 18:52