Modify

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11838 closed enhancement (fixed)

Clean up MapView, put layer management to new class.

Reported by: michael2402 Owned by: michael2402
Priority: normal Milestone: 16.06
Component: Core Version:
Keywords: layers, gsoc-core Cc: Don-vip, bastiK, stoecker

Description (last modified by 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.

Attachments (1)

layer-manager.patch (60.2 KB) - added by michael2402 6 years ago.

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by michael2402

Attachment: layer-manager.patch added

comment:1 Changed 6 years ago by 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
Last edited 6 years ago by simon04 (previous) (diff)

comment:2 in reply to:  1 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by simon04

Milestone: 15.10

Let's postpone MapView adaptions to the 15.10 release …

comment:4 Changed 6 years ago by simon04

Owner: changed from team to michael2402
Status: newneedinfo

What is the status of this ticket/patch?

comment:5 Changed 6 years ago by simon04

Milestone: 15.10

Unschedule tickets without progress

comment:6 Changed 5 years ago by michael2402

Description: modified (diff)
Keywords: gsoc-core added
Status: needinfonew
Summary: [Patch] Clean up MapView, put layer management to new class.Clean up MapView, put layer management to new class.

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.

comment:7 Changed 5 years ago by michael2402

Cc: Don-vip bastiK stoecker added

comment:8 Changed 5 years ago by Don-vip

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

comment:9 Changed 5 years ago by michael2402

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

comment:10 Changed 5 years ago by michael2402

Description: modified (diff)

comment:11 Changed 5 years ago by Don-vip

Milestone: 16.05

comment:12 Changed 5 years ago by Don-vip

Milestone: 16.0516.06

comment:13 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by michael2402

Description: modified (diff)

comment:15 Changed 5 years ago by 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.

Last edited 5 years ago by stoecker (previous) (diff)

comment:16 Changed 5 years ago by 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

comment:17 Changed 5 years ago by 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.

comment:18 Changed 5 years ago by michael2402

Description: modified (diff)

comment:19 Changed 5 years ago by michael2402

Description: modified (diff)

comment:20 Changed 5 years ago by michael2402

Resolution: fixed
Status: newclosed

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

Last edited 5 years ago by michael2402 (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain michael2402.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.