Clean up MapView, put layer management to new class.

[michael2402]

Move the layer core to a new class. ¶

• Move all layer code to a new LayerManager
• Move the active layer code to LayerManagerWithActive
• Define a new listener interface. Events are now wrapped in objects for easy extensibility and all run in the UI thread
  • LayerAddEvent
  • LayerRemoveEvent
  • LayerOrderChangeEvent
  • ActiveLayerChangeEvent
• The LayerManager is completely synchronized
• Provide adapter layer methods in MapView to not break plugins.
• Make the layer list accessible using Main.getLayers() and Main.getTemporarayLayers()

This patch is still not finished, since we should replace all layer access in JOSM with this new manager. But JOSM already works using the compatibility methods.

Since this is mostly a rewrite of the layer code, I cannot provide smaller patches.

Implementing this will require several steps, yielding at least one patch each:

Add the LayerManager ¶

The main change. We replace the mapView layer code with the new code and add compatibility methods.
I'll be doing this between May 23 and May 27.
See #12863

Add temporary layers ¶

We can use the LayerManager to manage temporary layers as well.

Migration ¶

Simply replace Main.map.mapView.getActiveLayer() with Main.getLayers().getActiveLayer() and so on. Most accesses to this are static, so they should be easy to find.

For some modules, it might be possible to add the LayerManager as private field. That way, we get rid of the dependency on a static Main access and make testing easier.

Migration patches: #12955, #12943, #12983, #12953.

[simon04]

Hi Michael, thank you for your refactoring work!

Here are some (non exhaustive) comments on your patch:

• For all deprecated methods/classes, please specify the new version to help plugin authors etc. to adapt their code.
• For all deprecated methods/classes, please specify a removal date (in 4 or 6 months?), see r8315 for some examples.
• Why did you add the field MapView#layerManager instead of accessing Main#layerManager? Doing the latter eases the migration (helps to figure out how to replace the deprecated method as well as helps when using refactoring in IDEs).
• Please avoid using deprecated methods in non deprecated methods (e.g., MapView#getLayerPos, MapView#isActiveLayerDrawable)
• Main#layerManager should be a final field.
• In MapView#getLayerPos, the indentation seems incorrect (did you diff with --ignore-all-space?)
• LayerManager#getLayersOfType claims returns a modifiable list, contradicting the Javadoc.
• I think, we should reduce the synchronization for many get* methods of the LayerManager/LayerManagerWithActive. The layers field could be implemented as CopyOnWriteArrayList (since changes are far less frequent than reads). Or use a ReentrantReadWriteLock as in DataSet#lock …

[michael2402]

Replying to simon04:

• For all deprecated methods/classes, please specify the new version to help plugin authors etc. to adapt their code.
• For all deprecated methods/classes, please specify a removal date (in 4 or 6 months?), see r8315 for some examples.

Thanks for the hint. I'll do that. I would suggest at least 6 Months (don't know how fast plugin authors are at updating, but this will break almost all plugins)

• Why did you add the field MapView#layerManager instead of accessing Main#layerManager? Doing the latter eases the migration (helps to figure out how to replace the deprecated method as well as helps when using refactoring in IDEs).

First, I don't like global state. Main#layerManager replaces the global MapView instance. I want the MapView to be a normal swing Component that simply displays the layer of one manager. That way, we can re-use it e.g. for the minimap and display data layers there - but we are not forced to display the same layers in all map views.

• Please avoid using deprecated methods in non deprecated methods (e.g., MapView#getLayerPos, MapView#isActiveLayerDrawable)

OK, I'll re-check that.

• Main#layerManager should be a final field.

Yes ;-)

• In MapView#getLayerPos, the indentation seems incorrect (did you diff with --ignore-all-space?)

• LayerManager#getLayersOfType claims returns a modifiable list, contradicting the Javadoc.

Code and Javadoc is just copy+paste, but this is a good time to fix that.

• I think, we should reduce the synchronization for many get* methods of the LayerManager/LayerManagerWithActive. The layers field could be implemented as CopyOnWriteArrayList (since changes are far less frequent than reads).

I think that getLayers is assumed to return a non-changing list in many methods (especially those handling/responding to events, since many listeners only do an invoke on the EDT). We might cache that list if performance is an issue there, but I don't think it is called that often (once every frame for drawing, once per action for most other operations that are influenced by layers). The main Idea behind making that method synchronized is that you do not have to worry about calling it in the middle of a moveLayer().

One more thing: I would like to force all addLayer/removeLayer/...-calls on the EDT thread (using runAndWait if necessary). This makes writing listeners that respond to those events and update UI elements much easier.

[simon04]

Let's postpone MapView adaptions to the 15.10 release …

[simon04]

What is the status of this ticket/patch?

[simon04]

Unschedule tickets without progress

[michael2402]

For this years GSoC, I will have time to dig this out again and work on this for one of the first things I am doing.

I adjusted the ticket to reflect my current design goals.

[Don-vip]

great! Please also have a look to #11390: I plan to switch to Java 8 in the coming weeks :)

[michael2402]

Thanks for the note, I was just about to ask that question - it makes listeners much nicer to read.

[michael2402]

I added a new patch for the layer manager.

Some notes:

• I call all listeners in the EDT. Most listeners update buttons, so this makes the listeners much easier.
• I did some minor porting to use the new API (OsmValidator, Main). There are still many places that need updating. JOSM still works using the deprecated methods but I do not want to blow the patch more. I'll work on this during the next weeks.
• There are tests for the new classes but not for compatibility methods/adapters.

[stoecker]

The two added files seem broken.

Ah, you wanted to remove the old patches. Please don't do this. Leave them as they are.

[michael2402]

I wanted to remove the patches I added to the wrong ticket. I did not find any remove function. They are now in #12863

[stoecker]

Normal users are not allowed to delete attachments (although they can overwrite their own ones, which is actually similar). I removed the two wrong files.

[michael2402]

I think we can consider this to be fully implemented. There are no more uses of the old interfaces in JOSM.