2

I have been perusing the open source code of JMapViewer. If anyone else wishes to look at it, check the SVN.

In a nutshell, the main class is JMapViewer, which is an extension of a JPanel. There is another very important class called DefaultMapController which acts as a MouseListener for the main class.

The first weird thing I noticed is that the viewer has no references to the controller. The JMapViewer constructor instantiates an anonymous instance of the DefaultMapController, like this:

public JMapViewer() {
    // other stuff
    new DefaultMapController(this);
}

This seems to me to be a poor design choice, since the controller has tons of methods (options, toggles, etc - example shown below), which now can not be accessed at all, so what good are they?

public void setMovementMouseButton(int movementMouseButton) {
    // changes which mouse button is used to move the map
}

The controller does have a reference to the viewer as shown in the first snippet above, which is how it is able to exercise control.

However, then I thought of something even weirder! If this anonymous instance of the listener has no references, why is it allowed to even survive? Shouldn't the GC destroy it quickly? Or is GC smart enough to know that a listener class which references a live JComponent must also stay alive to work properly, even if it has no name for some strange reason?

So, two real questions:

  • why does GC not destroy object?
  • is this indeed a poor design choice, or is there some way I'm unaware of to access the controller from the class which instantiates the viewer?

I want to contribute to this open source library, and my first idea for a change is to change the JMapViewer class to have a field referencing its controller, and to change the constructor to assign the currently anonymous controller to this new field. But, I want to make sure I'm not ignorantly missing something. I have searched the entire codebase for the text DefaultMapController, and it only occurs in its own class definitions, and in the anonymous instantiations in the JMapViewer constructors.


EDIT:

It does indeed appear that there is a way to access the anonymous listeners, by using the java.awt.Component method getMouseListeners(). So technically in my application I could search this collection for instances of DefaultMapController, and use that to access the methods I need to use to change the controller options.

To play devil's advocate though, if I go with original idea and give the map a reference of its controller, now I have a sort of circular reference (map knows of controller and controller knows of map). Is this a bad idea?

The111
  • 5,757
  • 4
  • 39
  • 55

2 Answers2

6

The abstract parent, JMapController, holds a reference to the JMapViewer passed there by the DefaultMapController constructor:

public DefaultMapController(JMapViewer map) {
    super(map);
}

Addendum: The map reference held by the controller is used to (selectively) add up to three controller references to the map's EventListenerList, discussed here. Any one of these would preclude GC. At least one salutary design benefit is that a concrete JMapController need only implement available interfaces.

As suggested in this MVC outline, it would be unusual to give the view a reference to the controller. In contrast, there's nothing wrong with letting the controller register as a listener to the view, as suggested here.

Note that only the no-argument JMapViewer constructor installs a DefaultMapController. You can use the alternate constructor, as noted in comments at line 57-59 in revision 29113 of Demo.java. A complete example is examined here.

Community
  • 1
  • 1
trashgod
  • 203,806
  • 29
  • 246
  • 1,045
  • 1
    Leave it to trash to go dumpster diving into a chit-load of code. Well done and 1+. – Hovercraft Full Of Eels Dec 24 '12 at 02:30
  • Right, I said that in my post. It is the `DefaultMapController` which has no references to it. Thanks though for actually digging into the code, did not expect anyone to do that. :-) – The111 Dec 24 '12 at 02:30
  • @The111: I've elaborated above. – trashgod Dec 24 '12 at 03:37
  • @trashgod Much thanks for the elaboration, I think it does help. I'll look through all your links in detail at a later date to make sure I understand 100%, as I'm on my way out the door soon. I especially appreciate the MVC link. I think I know what you're talking about in `Demo.java`, but I did not find a rev 29113 for it, max I found was 27992 (I think lines 57-59 in this rev are what you refer to). Thanks again. – The111 Dec 24 '12 at 03:49
  • Hahaha plus 1 to HFOE... And plus 1 to @trashgod, very nice explanation – David Kroukamp Dec 24 '12 at 05:59
  • @trashgod I have one more question regarding MVC and `JMapViewer`... assuming you are still relatively familiar with the design of this library... what class would you say is the model? The view and controller classes are obvious, but I am not sure there is a model... it seems to me the controllers update the view directly, which sort of functions as the model as well. I'd be curious to hear what you think. Thanks. – The111 Dec 27 '12 at 10:29
  • @The111: `JMapViewer` models a map as tiles, and `AbstractTileSource` is the root of classes implementing `TileSource` in what appears to be a strategy pattern. It would be nice to see [_package level documentation_](http://stackoverflow.com/q/3644726/230513) on this. – trashgod Dec 27 '12 at 12:07
  • @trashgod Thanks, that's what I figured. I've read all the code in the whole package so I get how the tiles are loaded/painted, but I thought it seemed not quite 100% MVC (which I'm still coming to grasp) to have the model *and* viewer represented by the same class, so was wondering if I was missing something. I guess in some ways, you could semantically argue, the tile server is the real model, although the `markers`, `polygons`, and `rectangles` that can be added to the map are clearly both part of the combined view/model implementation of the main `JMapViewer` class. – The111 Dec 27 '12 at 19:06
1

1) Everything you know is that, if and when the VM deems it to be appropriate, it will collect some or all of the dead objects. The GC is not required to do anything.

2) The best thing is to ask to the maintainer of the library. Anyway, as a general rule, I would not bother to change anything unless there's a good reason to, e.g. if it sensibly improves readability, and would rather focus myself on real problems.

3) Not sure if that's the case, but, when you serialize a JComponent, you also serialize all of its fields. And you do not want to serialize a lot of unused stuff.

ignis
  • 8,692
  • 2
  • 23
  • 20
  • There is a good reason to change it. I am calling the `JMapViewer` class in my application and wish the controller to behave slightly differently. There are already controller methods in place to create the behavior I desire. However, they're inaccessible since the controller is anonymous. The library hasn't been very actively maintained, but I am indeed trying to find out the reason for this design from them. – The111 Dec 24 '12 at 02:11