modular structure for JOSM core

[bastiK]

In the spirit of project Jigsaw, we could try to create a more modular structure for the JOSM source repository. Not only is it easier to maintain a modular project and more accessible for new developers, there are practical goals as well, namely to create standalone libraries and tools that will be valuable for other projects.

Replying to Don-vip:

I'd like to see JOSM core classes become the standard OSM Java library :) There would be no need for other people to create their own (​osm-common or ​osmapi for example)

Other goals would be:

• standalone validator (#15182)
• MapCSS library and renderer
• projection library and conversion tool
• IO for GPX, nmea, ...
• imagery (move WMS and WMTS support into JMapViewer)
• ...

I've started experimenting on GitHub: ​bastik:modules.

[bastiK]

In 12713/josm:

see #15229 - remove dependencies of CheckParameterUtil on various data classes

[Don-vip]

I see you're listing individual files, I assume it's because you're experimenting in an iterative manner?
One important thing: Java 9 does now allow split packages (same package defined in two different modules); that's why in #15182 I moved a few classes around and created new packages.

[bastiK]

Replying to Don-vip:

I see you're listing individual files, I assume it's because you're experimenting in an iterative manner?

Yes, I try to keep it small and get the modules to compile in a bottom-up kind of way.

One important thing: Java 9 does now allow split packages (same package defined in two different modules); that's why in #15182 I moved a few classes around and created new packages.

Okay, I'll keep that in mind, but it can always be fixed later on.

[Don-vip]

See also the patch in #13036. Maybe you have an idea about Michael's comments?

[Don-vip]

In 12718/josm:

see #13036 - see #15229 - see #15182 - make Commands depends only on a DataSet, not a Layer. This removes a lot of GUI dependencies

[bastiK]

I'd like ILatLon, LatLon, EastNorth, Bounds, ProjectionBounds, ... to be pure data classes without dependency on global singletons like Main.proj and Main.pref.

Attached patch deprecates ILatLon#getEastNorth() and makes a few more classes accept ILatLon instead of LatLon.

[Don-vip]

nice :)

[bastiK]

In 12725/josm:

see #15229 - deprecate ILatLon#getEastNorth() so ILatLon has no dependency on Main.proj

[Don-vip]

In 12729/josm:

see #15229 - fix typos, warnings

[Don-vip]

In 12734/josm:

see #15182 - see #15229 - remove GUI dependence in NoteData

[bastiK]

In 12735/josm:

see #15229 - move CoordinateFormat code out of LatLon class

[bastiK]

In 12738/josm:

see #15229 - checkstyle warnings

[bastiK]

In 12739/josm:

see #15229 - PMD warnings

[Don-vip]

In 12741/josm:

see #15229 - fix javadoc warnings

[Don-vip]

In 12742/josm:

see #15229 - see #15182 - remove useless dependence of Geometry on GUI now that MultipolygonCache does not need anymore a MapView

[Don-vip]

In 12743/josm:

see #15229 - see #15182 - deprecate gui.JosmUserIdentityManager - replaced by data.UserIdentityManager

[Don-vip]

In 12744/josm:

see #15229 - see #15182 - remove GUI dependence of PlatformHookWindows - move workaround to MainApplication

[bastiK]

In 12745/josm:

see #15229 - deprecate LatLon#toStringCSV

removes last references of LatLon to CoordinateFormat classes

[bastiK]

In 12746/josm:

see #15229 - fix deprecations in tests

[Don-vip]

In 12747/josm:

see #15229 - see #15182 - fix unit tests

[Don-vip]

In 12748/josm:

see #15229 - see #15182 - deprecate GuiHelper.getMenuShortcutKeyMaskEx() - replaced by PlatformHook.getMenuShortcutKeyMaskEx()

[Don-vip]

In 12749/josm:

see #15229 - see #15182 - see #13036 - remove GUI stuff from Command and DeleteCommand

[Don-vip]

In 12750/josm:

see #15229 - see #15182 - fix unit tests

[Don-vip]

@Wiktor, Michael: the use of org.openstreetmap.gui.jmapviewer.FeatureAdapter.getLogger in JCSCachedTileLoaderJob and JCSCacheManager is problematic. Could you please see if it can be replaced by an API solely based on josm.tools.Logging?

[Don-vip]

In 12751/josm:

see #15229 - see #15182 - see #13036 - update unit tests and groovy script

[Don-vip]

In 12752/josm:

see #15229 - see #15182 - see #13036 - fix groovy script

[wiktorn]

Replying to Don-vip:

@Wiktor, Michael: the use of org.openstreetmap.gui.jmapviewer.FeatureAdapter.getLogger in JCSCachedTileLoaderJob and JCSCacheManager is problematic. Could you please see if it can be replaced by an API solely based on josm.tools.Logging?

This shouldn't be a problem. I'll take a look in the evening.

Do you plan to move WMS/WMTS support together with caching to JMapViewer or you'd like to keep this separate?

[Don-vip]

In 12754/josm:

see #15229 - see #15182 - see #13036 - update unit tests

[Don-vip]

Replying to wiktorn:

This shouldn't be a problem. I'll take a look in the evening.
Do you plan to move WMS/WMTS support together with caching to JMapViewer or you'd like to keep this separate?

Ah, maybe simply do nothing. I thought cache was needed for my standalone validator project, but in fact I can skip these classes. Paul has plans to move wms/wmts support to JMapViewer, I let this to him :)

[Don-vip]

In 12755/josm:

see #15229 - see #15182 - make Main to not depend on JCS

[Don-vip]

In 12756/josm:

see #15229 - see #15182 - move RotationAngle from gui.util to tools

[bastiK]

Replying to wiktorn:

Do you plan to move WMS/WMTS support together with caching to JMapViewer or you'd like to keep this separate?

Moving WMS/WMTS support to JMapViewer is more like a brainstorming idea. At the moment, I have no concrete plans for this project.

As for the cache, it makes sense to define an abstract caching interface and offer a way to select different caching implementations.

[Don-vip]

In 12757/josm:

see #15229 - see #15182 - see #13036 - update fixture for performance tests

[Don-vip]

In 12758/josm:

see #15229 - see #15182 - deprecate DataSet.getAutoCompletionManager() - replacement: AutoCompletionManager.of(DataSet)

[Don-vip]

In 12760/josm:

see #15229 - see #15182 - see #13036 - remove more GUI stuff from DeleteCommand

[Don-vip]

In 12761/josm:

see #15229 - see #15182 - see #13036 - fix stupid mistake

[Don-vip]

In 12763/josm:

see #15229 - see #15182 - see #13036 - remove last GUI stuff from DeleteCommand

[Don-vip]

I have several problematic calls to GuiHelper.runInEDT* outside of GUI.
Here is a proposition, what do you think? I don't know if it's the best one.

src/org/openstreetmap/josm/gui/util/GuiHelper.java

@@ -24 +24 @@
 import java.awt.event.MouseAdapter;
 import java.awt.event.MouseEvent;
 import java.awt.image.FilteredImageSource;
-import java.lang.reflect.InvocationTargetException;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Enumeration;
 import java.util.EventObject;
 import java.util.Locale;
 import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.FutureTask;
 
 import javax.swing.GrayFilter;
 import javax.swing.ImageIcon;
@@ -59 +56 @@
 import org.openstreetmap.josm.gui.widgets.HtmlPanel;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.ColorHelper;
+import org.openstreetmap.josm.tools.EdtHelper;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.ImageOverlay;
 import org.openstreetmap.josm.tools.ImageProvider;
@@ -66 +64 @@
 import org.openstreetmap.josm.tools.LanguageInfo;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.PlatformHook;
-import org.openstreetmap.josm.tools.bugreport.BugReport;
 import org.openstreetmap.josm.tools.bugreport.ReportedException;
 
 /**
@@ -194 +191 @@
      * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
      * @param task The runnable to execute
      * @see SwingUtilities#invokeLater
+     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDT(Runnable)} instead
      */
+    @Deprecated
     public static void runInEDT(Runnable task) {
-        if (SwingUtilities.isEventDispatchThread()) {
-            task.run();
-        } else {
-            SwingUtilities.invokeLater(task);
-        }
+        EdtHelper.runInEDT(task);
     }
 
     /**
@@ -208 +203 @@
      * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
      * @param task The runnable to execute
      * @see SwingUtilities#invokeAndWait
+     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWait(Runnable)} instead
      */
+    @Deprecated
     public static void runInEDTAndWait(Runnable task) {
-        if (SwingUtilities.isEventDispatchThread()) {
-            task.run();
-        } else {
-            try {
-                SwingUtilities.invokeAndWait(task);
-            } catch (InterruptedException | InvocationTargetException e) {
-                Logging.error(e);
-            }
-        }
+        EdtHelper.runInEDTAndWait(task);
     }
 
     /**
@@ -230 +219 @@
      * @param task The runnable to execute
      * @see SwingUtilities#invokeAndWait
      * @since 10271
+     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWaitWithException(Runnable)} instead
      */
+    @Deprecated
     public static void runInEDTAndWaitWithException(Runnable task) {
-        if (SwingUtilities.isEventDispatchThread()) {
-            task.run();
-        } else {
-            try {
-                SwingUtilities.invokeAndWait(task);
-            } catch (InterruptedException | InvocationTargetException e) {
-                throw BugReport.intercept(e).put("task", task);
-            }
-        }
+        EdtHelper.runInEDTAndWaitWithException(task);
     }
 
     /**
@@ -251 +234 @@
      * @param callable The callable to execute
      * @return The computed result
      * @since 7204
+     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWaitAndReturn(Callable)} instead
      */
+    @Deprecated
     public static <V> V runInEDTAndWaitAndReturn(Callable<V> callable) {
-        if (SwingUtilities.isEventDispatchThread()) {
-            try {
-                return callable.call();
-            } catch (Exception e) { // NOPMD
-                Logging.error(e);
-                return null;
-            }
-        } else {
-            FutureTask<V> task = new FutureTask<>(callable);
-            SwingUtilities.invokeLater(task);
-            try {
-                return task.get();
-            } catch (InterruptedException | ExecutionException e) {
-                Logging.error(e);
-                return null;
-            }
-        }
+        return EdtHelper.runInEDTAndWaitAndReturn(callable);
     }
 
     /**
      * This function fails if it was not called from the EDT thread.
      * @throws IllegalStateException if called from wrong thread.
      * @since 10271
+     * @deprecated to be removed end of 2017. Use {@link EdtHelper#assertCallFromEdt()} instead
      */
+    @Deprecated
     public static void assertCallFromEdt() {
-        if (!SwingUtilities.isEventDispatchThread()) {
-            throw new IllegalStateException(
-                    "Needs to be called from the EDT thread, not from " + Thread.currentThread().getName());
-        }
+        EdtHelper.assertCallFromEdt();
     }
 
     /**

src/org/openstreetmap/josm/tools/EdtHelper.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.tools;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
+
+import javax.swing.SwingUtilities;
+
+import org.openstreetmap.josm.tools.bugreport.BugReport;
+import org.openstreetmap.josm.tools.bugreport.ReportedException;
+
+/**
+ * Utilities to make life with Event Dispatch Thread (EDT) easier.
+ * @since xxx
+ */
+public final class EdtHelper {
+
+    private EdtHelper() {
+        // Hide default constructor for utils classes
+    }
+
+    /**
+     * Executes asynchronously a runnable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
+     * @param task The runnable to execute
+     * @see SwingUtilities#invokeLater
+     */
+    public static void runInEDT(Runnable task) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            task.run();
+        } else {
+            SwingUtilities.invokeLater(task);
+        }
+    }
+
+    /**
+     * Executes synchronously a runnable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
+     * @param task The runnable to execute
+     * @see SwingUtilities#invokeAndWait
+     */
+    public static void runInEDTAndWait(Runnable task) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            task.run();
+        } else {
+            try {
+                SwingUtilities.invokeAndWait(task);
+            } catch (InterruptedException | InvocationTargetException e) {
+                Logging.error(e);
+            }
+        }
+    }
+
+    /**
+     * Executes synchronously a runnable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
+     * <p>
+     * Passes on the exception that was thrown to the thread calling this.
+     * The exception is wrapped using a {@link ReportedException}.
+     * @param task The runnable to execute
+     * @see SwingUtilities#invokeAndWait
+     */
+    public static void runInEDTAndWaitWithException(Runnable task) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            task.run();
+        } else {
+            try {
+                SwingUtilities.invokeAndWait(task);
+            } catch (InterruptedException | InvocationTargetException e) {
+                throw BugReport.intercept(e).put("task", task);
+            }
+        }
+    }
+
+    /**
+     * Executes synchronously a callable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>
+     * and return a value.
+     * @param <V> the result type of method <tt>call</tt>
+     * @param callable The callable to execute
+     * @return The computed result
+     */
+    public static <V> V runInEDTAndWaitAndReturn(Callable<V> callable) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            try {
+                return callable.call();
+            } catch (Exception e) { // NOPMD
+                Logging.error(e);
+                return null;
+            }
+        } else {
+            FutureTask<V> task = new FutureTask<>(callable);
+            SwingUtilities.invokeLater(task);
+            try {
+                return task.get();
+            } catch (InterruptedException | ExecutionException e) {
+                Logging.error(e);
+                return null;
+            }
+        }
+    }
+
+    /**
+     * This function fails if it was not called from the EDT thread.
+     * @throws IllegalStateException if called from wrong thread.
+     */
+    public static void assertCallFromEdt() {
+        if (!SwingUtilities.isEventDispatchThread()) {
+            throw new IllegalStateException(
+                    "Needs to be called from the EDT thread, not from " + Thread.currentThread().getName());
+        }
+    }
+}

[bastiK]

Replying to Don-vip:

I have several problematic calls to GuiHelper.runInEDT* outside of GUI.

For example?

Here is a proposition, what do you think? I don't know if it's the best one.

Why not, as long as javax.swing.SwingUtilities import is fine.

[bastiK]

In the github ​branch, I can now compile the projection classes with just the following minimal dependencies:

org.openstreetmap.josm.data.Bounds
org.openstreetmap.josm.data.coor.Coordinate
org.openstreetmap.josm.data.coor.EastNorth
org.openstreetmap.josm.data.coor.ILatLon
org.openstreetmap.josm.data.coor.LatLon
org.openstreetmap.josm.data.coor.LatLonToEastNorthConverter
org.openstreetmap.josm.data.osm.BBox
org.openstreetmap.josm.data.osm.BBoxProvider
org.openstreetmap.josm.data.ProjectionBounds
org.openstreetmap.josm.tools.bugreport.BugReport
org.openstreetmap.josm.tools.bugreport.BugReportQueue
org.openstreetmap.josm.tools.bugreport.ReportedException
org.openstreetmap.josm.tools.CheckParameterUtil
org.openstreetmap.josm.tools.I18n
org.openstreetmap.josm.tools.JosmRuntimeException
org.openstreetmap.josm.tools.LanguageInfo
org.openstreetmap.josm.tools.Logging
org.openstreetmap.josm.tools.MultiMap
org.openstreetmap.josm.tools.Platform
org.openstreetmap.josm.tools.PlatformVisitor
org.openstreetmap.josm.tools.StreamUtils
org.openstreetmap.josm.tools.SubclassFilteredCollection
org.openstreetmap.josm.tools.Utils

I will commit those changes (except methods will be deprecated and not deleted + javadoc). You are welcome to review the patches!

Now, the MapCSS code looks a bit more difficult to disentangle... :)

[Don-vip]

I have 5 occurences:

data.osm.ChangesetCache.java
82: GuiHelper.runInEDT(() -> {  

gui.mappaint.ElemStyles.java
84: GuiHelper.runInEDT(() -> {  

gui.tagging.presets.TaggingPreset.java
220: GuiHelper.runInEDT(() -> result.attachImageIcon(this));  

io.auth.AbstractCredentialsAgent.java
54: GuiHelper.runInEDTAndWait(() -> {  

io.MessageNotifier.java
73: GuiHelper.runInEDT(() -> {  

Ideally I'd like to not depend on AWT and Swing but it is more and more difficult!

[Don-vip]

Replying to bastiK:

You are welcome to review the patches!

You can move Utils.getJavaExpirationDate() alongside Utils.getJavaLatestVersion(). These two methods are meant to be together :)

Now, the MapCSS code looks a bit more difficult to disentangle... :)

Agreed! Didn't start to look at it yet. I think I'll go with the presets first.

[Don-vip]

In 12764/josm:

see #15229 - see #15182 - fix unit test

[bastiK]

Replying to Don-vip:

I have 5 occurences:

data.osm.ChangesetCache.java
82: GuiHelper.runInEDT(() -> {

Convenience code, can be moved to ChangesetCacheListener.changesetCacheUpdated methods.

gui.mappaint.ElemStyles.java
84: GuiHelper.runInEDT(() -> {  

Tricky, not sure how to deal with that.

gui.tagging.presets.TaggingPreset.java
220: GuiHelper.runInEDT(() -> result.attachImageIcon(this));  

io.auth.AbstractCredentialsAgent.java
54: GuiHelper.runInEDTAndWait(() -> {  

io.MessageNotifier.java
73: GuiHelper.runInEDT(() -> {  

These 3 seem closely tied to GUI code. If that GUI code is extracted, the runInEDT* calls can be moved as well.

[wiktorn]

In 12765/josm:

Use tools.Logging instead of FeatureAdapter.getLogger

See: #15229

[Don-vip]

In 12766/josm:

see #15229 - see #15182 - remove GUI stuff from MessageNotifier

[Don-vip]

In 12767/josm:

see #15229 - see #15182 - do not longer fire ChangesetCacheEvent in EDT - now up to listeners to run their code in it if required

[bastiK]

In 12770/josm:

see #15229 - remove GUI references from BugReport and BugReportQueue

signature of BugReport#getReportText changed without deprecation
(probably not used by any external plugin)

[bastiK]

In 12772/josm:

see #15229 - move compression methods from Utils to Compression

[bastiK]

In 12776/josm:

see #15229 - remove dependency of NTV2GridShiftFileWrapper on Main.platform

• PlatformHook bundles all application-wide platform dependent code, which is

convenient, but problematic for separating modules

• introduces lightweight tools.Platform for adding platform dependent code more locally

[bastiK]

In 12777/josm:

see #15229 - add registry for NTV2 grid file sources

removes dependency of NTV2GridShiftFileWrapper on Main

[bastiK]

In 12778/josm:

see #15229 - deprecate Projections#project and Projections#inverseProject

replacement is a bit more verbose, but the fact that Main.proj is
involved need not be hidden

[Don-vip]

In 12779/josm:

see #15229 - fix unit test

[Don-vip]

In 12780/josm:

see #15229 - fix PMD warning

[Don-vip]

In 12781/josm:

see #15229 - add javadoc

[Don-vip]

In 12782/josm:

see #15229 - see #15182 - move ImageProcessor from gui.layer to tools

[Don-vip]

In 12783/josm:

see #15229 - fix warnings

[bastiK]

In 12784/josm:

see #15229 - minor cleanup

[bastiK]

In 12788/josm:

see #15229 - PMD warnings

[Don-vip]

In 12790/josm:

see #15229 - see #15182 - remove GUI references from BugReportSender

[Don-vip]

Jenkins is stuck, maybe because of projection changes, see jenkins.txt.

[Don-vip]

In 12791/josm:

see #15229 - see #15182 - javadoc fixes

[bastiK]

In [12786]:

see #15182 - remove GUI references from Projections class

[bastiK]

Replying to Don-vip:

Jenkins is stuck, maybe because of projection changes, see jenkins.txt.

This thread gets a lock on Main, then tries to do DataSet.beginUpdate, but somehow gets stuck. Not sure what is blocking the DataSet lock.

[bastiK]

In 12792/josm:

closes #15273, see #15229, see #15182 - add command line interface module for projections

• run josm project --help to see the options
• extracts parser from LatLon and CustomProjection into LatLonParser

[bastiK]

In 12793/josm:

see #15273, see #15229, see #15182 - add missing class

[bastiK]

In 12795/josm:

see #15273, see #15229 - fix unit tests, PMD, etc.

[Don-vip]

In 12796/josm:

see #15229 - see #15182 - remove GUI references from OsmUrlToBounds

[Don-vip]

In 12799/josm:

see #15229 - see #15182 - see #14794 - move Multi* GUI classes from tools to gui.util

[Don-vip]

In 12800/josm:

see #15229 - see #15182 - deprecate OsmWriter.writeLayer(OsmDataLayer) - replacement: OsmWriter.write(DataSet)

[Don-vip]

In 12803/josm:

see #15229 - see #15182 - remove GUI references from OsmConnection

[Don-vip]

In 12804/josm:

see #15229 - see #15182 - remove GUI references from OsmApi

[Don-vip]

In 12805/josm:

see #15229 - see #15182 - remove GUI references from DefaultProxySelector

[Don-vip]

In 12806/josm:

see #15229 - see #15182 - remove GUI references from GeoJSONWriter

[Don-vip]

In 12807/josm:

see #15229 - see #15182 - see #14794 - checkstyle, unit tests

[Don-vip]

In 12816/josm:

see #15229 - see #15182 - remove GUI references from I/O subsystem

[bastiK]

In 12817/josm:

see #15229 - deprecate unused method

[bastiK]

In 12818/josm:

see #15229 - move Bounds#visitEdge to Projection#visitOutline

• joins 2 implementations of the same algorithm
• removes dependency of Bounds on Projection

[bastiK]

In 12819/josm:

see #15229 - trace world bounds in finer steps

no efficiency problem as result is cached

[bastiK]

In 12820/josm:

see #15229 - wrap around +180 degrees, for consistency

[Don-vip]

In 12821/josm:

see #15229 - see #15182 - remove GUI references from I/O subsystem

[Don-vip]

In 12823/josm:

see #15229 - see #15182 - move PaintVisitor from data.validation to gui.layer.validation

[Don-vip]

how do you think we should proceed for mappaint and tagging presets?

1. move data/model classes that do not depend on GUI components to data.mappaint / data.tagging.presets (major impact, many classes to move, many plugins will break)

or:

2. keep as many classes in the current packages, but move pure GUI classes to another package. But how should we name it?

[Don-vip]

In 12825/josm:

see #15229 - see #15182 - make FileWatcher generic so it has no more dependence on MapCSS

[Don-vip]

In 12826/josm:

see #15229 - see #15182 - remove GUI references from PreferencesUtils

[bastiK]

Replying to Don-vip:

how do you think we should proceed for mappaint and tagging presets?

1. move data/model classes that do not depend on GUI components to data.mappaint / data.tagging.presets

I haven't really looked into it, but when we have a command line renderer without dependency on swing, then it would make sense to move the packages org.openstreetmap.josm.gui.mappaint.* to org.openstreetmap.josm.rendering.*.

(major impact, many classes to move, many plugins will break)

If it is just a change of package/class name, we could create a TODO list and then make all the plugin-breaking changes in one go.

or:

2. keep as many classes in the current packages, but move pure GUI classes to another package. But how should we name it?

gui.mappaint.gui ;)

[Don-vip]

Replying to bastiK:

gui.mappaint.gui ;)

... that's what I was afraid of :D It's ugly :D

[bastiK]

One thing I'm still struggling with are the service interfaces. We have a number "client" modules:

• validation, rendering, imagery/jmapviewer, osmapi, ...

and a number of service modules:

• projection, i18n, preferences, logging, ...

(The service implementations like logging and i18n don't actually need to be separate modules, but for the sake of argument, lets say they are.)

Each client depends on a number of services, but it would be advantageous to make that a loose coupling:

• For example, the renderer should be capable to draw in any given projection. In the majority of cases, all you want is to draw with standard Mercator projection, so no need to always include the full-blown projection module. Likewise for an imagery module: if all you want is TMS, then there is no need for all those projection classes and data files.
• Another example would be the preference handling: If one of the client modules is included in another application as a library, then this application should be able to inject a custom implementation of the preferences handling.

What this calls for, is an intermediate service API, i.e. something like the FeatureAdapter class in jmapviewer. Only that the service API will be required by several different clients. This means the service interface cannot be placed in the client module (unless you accept massive code duplication). It also cannot be placed inside the service module, or else the JOSM service implementation becomes a mandatory dependency and there is no loose coupling.

For now, the only solution I can think of, is to make each service API a Java 9 module of its own. Those would be very small modules, with just an interface and possibly a fallback-/default implementation of the service. Not sure if this is ugly, or how it is supposed to be. :)

[Don-vip]

In 12828/josm:

see #15229 - see #15182 - see #13036 - convert SplitWayAction to SplitWayCommand to remove dependence of DeleteCommand on actions package

[bastiK]

In 12831/josm:

see #15229 - extract GUI from StyleSetting class

[bastiK]

Replying to Don-vip:

Replying to bastiK:

gui.mappaint.gui ;)

... that's what I was afraid of :D It's ugly :D

For now ([12831]), I've just put the GUI classes in the same package as the data classes (anticipating a move of the data classes to data.mappaint at some point).

[bastiK]

In 12832/josm:

see #15229 - fix checkstyle warning

[Don-vip]

In 12834/josm:

see #15229 - see #15182 - move TaggingPresetSearchAction to actions package

[Don-vip]

In 12835/josm:

see #15229 - see #15182 - remove GUI references from TaggingPresetItem

[bastiK]

In 12840/josm:

see #15229 - extract interface for Preferences class

some changes to the method names and signatures

• to be more in line with java.util.prefs.Preferences:

  put(String, boolean) -> putBoolean(String, boolean)
  getInteger(String) -> getInt(String)
  putInteger(String, Integer) -> putInt(String, int)
  putDouble(String, Double) -> putDouble(String, double)
  

• for internal consistency with Setting classes:

  getCollection -> getList
  putCollection -> putList
  getArray -> getListOfLists
  putArray -> putListOfLists
  getListOfStructs -> getListOfMaps
  putListOfStructs -> putListOfMaps
  

[bastiK]

In 12841/josm:

see #15229 - fix deprecations caused by [12840]

[bastiK]

In 12842/josm:

see #15229 - fix test deprecations

[bastiK]

In 12843/josm:

see #15229 - minor fixes (Jenkins)

[bastiK]

In 12844/josm:

see #15229 - one more minor style fix

[bastiK]

Next step is to replace calls to Main.pref.get(key) by something like Config.getPref().get(key).

This removes dependencies on the JOSM-specific class org.openstreetmap.josm.data.Preferences. Instead, classes only need to know about the interface IPreferences in order to get or set preferences. Then a module, which runs standalone or as part of some other software, can still have preferences, but a custom handler may be installed:

src/org/openstreetmap/josm/gui/MainApplication.java

@@ -144 +144 @@
 import org.openstreetmap.josm.io.remotecontrol.RemoteControl;
 import org.openstreetmap.josm.plugins.PluginHandler;
 import org.openstreetmap.josm.plugins.PluginInformation;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.FontsManager;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.HttpClient;
@@ -920 +921 @@
             I18n.set(Main.pref.get("language", null));
         }
         Main.pref.updateSystemProperties();
+        Config.setPreferencesInstance(Main.pref);
 
         checkIPv6();
 

src/org/openstreetmap/josm/gui/util/WindowGeometry.java

@@ -19 +19 @@
 import javax.swing.JComponent;
 
 import org.openstreetmap.josm.Main;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Logging;
@@ -205 +206 @@
     }
 
     protected final void initFromPreferences(String preferenceKey) throws WindowGeometryException {
-        String value = Main.pref.get(preferenceKey);
+        String value = Config.getPref().get(preferenceKey);
         if (value.isEmpty())
             throw new WindowGeometryException(
                     tr("Preference with key ''{0}'' does not exist. Cannot restore window geometry from preferences.", preferenceKey));
@@ -274 +275 @@
         StringBuilder value = new StringBuilder(32);
         value.append("x=").append(topLeft.x).append(",y=").append(topLeft.y)
              .append(",width=").append(extent.width).append(",height=").append(extent.height);
-        Main.pref.put(preferenceKey, value.toString());
+        Config.getPref().put(preferenceKey, value.toString());
     }
 
     /**

src/org/openstreetmap/josm/spi/preferences/Config.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.spi.preferences;
+
+import org.openstreetmap.josm.data.preferences.IPreferences;
+
+/**
+ * Class to hold the global preferences object.
+ */
+public class Config {
+
+    private static IPreferences preferences;
+
+    /**
+     * Get the preferences.
+     * @return the preferences
+     */
+    public static IPreferences getPref() {
+        return preferences;
+    }
+
+    /**
+     * Install the global preference instance.
+     * @param preferences the global preference instance to set
+     */
+    public static void setPreferencesInstance(IPreferences preferences) {
+        Config.preferences = preferences;
+    }
+}

[Don-vip]

Nice. For the global setters, I like to add null-check as follows:

public static void setPreferencesInstance(IPreferences preferences) {
    Config.preferences = Objects.requireNonNull(preferences, "preferences");
}

This allows to detect bad configuration earlier, before a real problem happens :)

[bastiK]

In [12845]:

new class Config to hold a global IPreferences singleton, replacing calls to Main.pref as far as the interface supports it

makes it possible to easily replace org.openstreetmap.josm.data.Preferences by
another preferences handler (implementation of IPreferences)

[bastiK]

In 12846/josm:

see #15229 - use Config.getPref() wherever possible

[bastiK]

In 12847/josm:

see #15229 - move new classes to spi package

[Don-vip]

Replying to bastiK:

In 12846/josm:

see #15229 - use Config.getPref() wherever possible

I wonder if you didn't just beat the JOSM world record for the highest number of files modified in a single commit :D

[bastiK]

Yes, and I had to install Eclipse for refactoring as Netbeans wasn't up to the task. :)

[bastiK]

In 12848/josm:

see #15229 - set up Config for tests

[floscher]

@Don-Vip I think you're still in the lead with:

• https://josm.openstreetmap.de/changeset/7491/josm (images, which you optimized, so maybe not really countable)
• https://josm.openstreetmap.de/changeset/8510/josm (mostly whitespace)
• https://josm.openstreetmap.de/changeset/12620/josm (mostly non-whitespace)

But still impressive @bastiK ;) 😉

[bastiK]

In 12849/josm:

see #15229 - use Config in tests

[bastiK]

In 12850/josm:

see #15229 - use Config in functional tests

[Don-vip]

Replying to floscher:

@Don-Vip I think you're still in the lead with:

• https://josm.openstreetmap.de/changeset/7491/josm (images, which you optimized, so maybe not really countable)
• https://josm.openstreetmap.de/changeset/8510/josm (mostly whitespace)
• https://josm.openstreetmap.de/changeset/12620/josm (mostly non-whitespace)

But still impressive @bastiK ;) 😉

haha nice :) Yes, r7491 was only running a script and does not count. I remember the two other ones now. I used some regex, but still there was a lot of manual work.

[bastiK]

In 12851/josm:

see #15229 - extract "struct" handling from Preference to StructUtils

[bastiK]

In 12852/josm:

see #15229 - fix taginfo script

[bastiK]

In 12853/josm:

see #15229 - include support for the long type in IPreferences

[bastiK]

In 12854/josm:

see #15229 - fix checkstyle warnings

[bastiK]

In 12855/josm:

see #15229 - add separate interface IBaseDirectories to look up pref, user data and cache dir

[bastiK]

In 12856/josm:

see #15229 - add parameter to base directory methods

[bastiK]

In 12857/josm:

see #15229 - fix bug where parameter createIfMissing would be ignored because of caching

[Don-vip]

In 12858/josm:

see #15229 - fix integration tests

[Don-vip]

Don't forget the package-info.java for new packages :)

[Don-vip]

In 12859/josm:

see #15229 - see #15182 - deprecate non-GUI AutoCompletion* classes from gui.tagging.ac. Offer better replacements in new data.tagging.ac package

[bastiK]

In 12860/josm:

see #15229 - add package-info.java

[bastiK]

Replying to Don-vip:

Don't forget the package-info.java for new packages :)

Okay, didn't really know those were a thing.

[Don-vip]

It displays the package description on doc.

[Don-vip]

In 12869/josm:

see #15229 - see #15182 - SonarQube - squid:S2444 - make static fields volatile

[Don-vip]

In 12870/josm:

see #15229 - see #15182 - remove dependence of ImageProvider on PluginHandler

[bastiK]

In [o33632], [o33633]: [indoorhelper] avoid usage of data.preferences.Setting class in anticipation of JOSM-core rework

[bastiK]

In 12881/josm:

see #15229 - move remaining classes to spi.preferences package, to make it self-contained

• extract event listener classes from Preferences (duplicated, for smooth transition)
• move *Setting classes

[bastiK]

In 12882/josm:

see #15229 - fix @since

[Don-vip]

Replying to bastiK:

In 12881/josm:

* move `*Setting` classes

Tests must be updated.

[bastiK]

In 12884/josm:

see #15229 - fix unit tests

[bastiK]

In 12885/josm:

see #15229 - fix checkstyle warnings

[simon04]

In 12886/josm:

see #15229 - see #15182 - fix typo

[bastiK]

In 12891/josm:

see #15229 - move non-essential helper methods from Preferences to PreferencesUtils

[bastiK]

In 12892/josm:

see #15229 - remove dependency of getopt library on JOSM classes

[bastiK]

In 12893/josm:

see #15229 - add getopt to ant target "compile-cots"; minor fix

[bastiK]

In 12894/josm:

see #15229 - update method name and signature for consistency

[bastiK]

In 12906/josm:

see #15273, see #15229 - add command line module for rendering

run josm render --help to see the options

[bastiK]

In 12907/josm:

see #15273, see #15229 - fix tests and Jenkins warnings

[bastiK]

In 12928/josm:

see #15229 - do not copy the entire preferences list, just to set a custom server API in OAuth wizard

[bastiK]

In 12999/josm:

see #15229 - use Config.getPref() in *Property classes

[bastiK]

In 13002/josm:

see #15229 - remove some uses of Main.pref

[bastiK]

In 13021/josm:

make Preferences more generic (see #15229, closes #15451)

• extract base directories
• move updateSystemProperties() as it is specific to JOSM-core as a "prefefences client"

[Don-vip]

In 13140/josm:

see #15229 - provide a replacement API to deprecated Coordinate.toBBox() (used by plugins)

[Don-vip]

In 14118/josm:

see #15229 - new package cli to avoid circular package dependencies

[Don-vip]

Current dependencies graph using ​CDA:

[Don-vip]

Same graph using ​jdeps:

jdeps --dot-output dots -f "java.*|org.apache.*|org.xml.*|org.w3c.*|org.openstreetmap.gui.*|oauth.*|gnu.*|
com.*|org.jdesktop.*|sun.*" *.jar
dot -Tpng -O dots/summary.dot

[Don-vip]

In 14119/josm:

see #15229 - deprecate all Main methods returning an URL

[Don-vip]

In 14120/josm:

see #15229 - deprecate all Main methods related to projections. New ProjectionRegistry class

[Don-vip]

In 14121/josm:

see #15229 - deprecate all Main methods related to network features. New NetworkManager class

[Don-vip]

In 14122/josm:

see #15229 - minor preferences refactoring

[Don-vip]

In 14125/josm:

see #15229 - extract lifecycle Main classes to a new lifecycle SPI

[Don-vip]

In 14128/josm:

see #15229 - deprecate Main.fileWatcher

[Klumbumbus]

Replying to Don-vip:

In 14125/josm:

see #15229 - extract lifecycle Main classes to a new lifecycle SPI

You forgot to replace @since xxx in the new files.

[Don-vip]

In 14131/josm:

see #16590 - more fixes (patch by ris)
see #15229 - javadoc fixes

[Don-vip]

In 14134/josm:

see #15229 - deprecate Main*.undoRedo - make UndoRedoHandler a singleton

[Don-vip]

In 14135/josm:

see #15229 - fix deprecation warnings

[Don-vip]

In 14136/josm:

see #15229 - add new ant target to generate jdeps dependencies graphs

[Don-vip]

We can now generate three dependencies graphs using ant jdeps:

JOSM classes only:

JOSM classes and direct dependencies:

JOSM classes and all dependencies:

[Don-vip]

In 14137/josm:

see #15229 - checkstyle

[Don-vip]

In 14138/josm:

see #15229 - deprecate Main.platform and related methods - new class PlatformManager

[Don-vip]

In 14139/josm:

see #15229 - move Main initialization methods to lifecycle SPI + new class MainInitialization

[Don-vip]

In 14140/josm:

see #15229 - move Main* termination methods to lifecycle SPI + new class MainTermination

[Don-vip]

In 14143/josm:

see #15229 - deprecate Main.main - new class OsmDataManager

[Don-vip]

In 14144/josm:

see #15229 - code refactoring - add PlatformHook.getPossiblePreferenceDirs()

[Don-vip]

In 14145/josm:

see #15229 - code refactoring - make Preferences.getAllPossiblePreferenceDirs() static

[Don-vip]

In 14146/josm:

see #15229 - don't import Main just for javadoc

[Don-vip]

In 14147/josm:

see #15229 - code refactoring - move getAllPrefix* methods from Preferences to AbstractPreferences

[Don-vip]

In 14148/josm:

see #15229 - code refactoring - make Preferences.getJOSMDirectoryBaseName() static

[Don-vip]

In 14149/josm:

see #15229 - deprecate Main.pref

[Don-vip]

In 14150/josm:

see #15229 - make sure Main.pref has the correct global value

[Don-vip]

In 14153/josm:

see #15229 - deprecate Main.parent and Main itself

[Don-vip]

In 14154/josm:

see #15229 - fix warnings

[Don-vip]

Plugins updated in [o34452:34575].

[Don-vip]

In 14253/josm:

see #15229 - kill Main. RIP

[Don-vip]

In 14254/josm:

see #15229 - update spotbugs filters

[Don-vip]

Updated graph:

[Don-vip]

In 14255/josm:

see #15229 - remove josm-main.jar from jdeps graph

[Don-vip]

In 14256/josm:

see #15229 - move corrector package to actions.corrector

[michael2402]

A graph of (possible) JOSM modules.

Parts of it do not correspond to the JOSM directory structure. We especially have problems with the tools / preferences, avoiding circular dependencies there requires some more work.

[michael2402]

Update some missing dependencies

[michael2402]

Mind that this is WIP and we will probably be changing this tomorrow.