Modify

Opened 7 years ago

Last modified 5 years ago

#15229 new enhancement

modular structure for JOSM core

Reported by: bastiK Owned by: team
Priority: major Milestone: Longterm
Component: Core Version:
Keywords: modularization hack-weekend-2018-10 Cc: wiktorn, michael2402

Description (last modified by Don-vip)

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.

Attachments (11)

deprecate_ILatLon#getEastNorth().patch (25.2 KB ) - added by bastiK 7 years ago.
jenkins.txt (78.7 KB ) - added by Don-vip 7 years ago.
graph-cda-20180811.png (47.7 KB ) - added by Don-vip 6 years ago.
graph-jdeps-20180811.png (300.4 KB ) - added by Don-vip 6 years ago.
josm-without-dependencies.png (327.6 KB ) - added by Don-vip 6 years ago.
josm-with-direct-dependencies.png (513.8 KB ) - added by Don-vip 6 years ago.
josm-with-all-dependencies.png (1.1 MB ) - added by Don-vip 6 years ago.
josm-without-dependencies-20180916.png (169.9 KB ) - added by Don-vip 6 years ago.
modules.png (71.7 KB ) - added by michael2402 5 years ago.
modules.2.png (90.0 KB ) - added by michael2402 5 years ago.
Update some missing dependencies
hack-weekend-2018-10-modules.png (40.7 KB ) - added by simon04 5 years ago.

Change History (198)

comment:1 by bastiK, 7 years ago

In 12713/josm:

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

comment:2 by Don-vip, 7 years ago

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.

comment:3 by Don-vip, 7 years ago

Keywords: modularization added
Type: defectenhancement

comment:4 by Don-vip, 7 years ago

Description: modified (diff)

in reply to:  2 comment:5 by bastiK, 7 years ago

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.

comment:6 by Don-vip, 7 years ago

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

comment:7 by Don-vip, 7 years ago

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

comment:8 by bastiK, 7 years ago

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.

comment:9 by Don-vip, 7 years ago

nice :)

comment:10 by bastiK, 7 years ago

In 12725/josm:

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

comment:11 by Don-vip, 7 years ago

In 12729/josm:

see #15229 - fix typos, warnings

comment:12 by Don-vip, 7 years ago

In 12734/josm:

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

comment:13 by bastiK, 7 years ago

In 12735/josm:

see #15229 - move CoordinateFormat code out of LatLon class

comment:14 by bastiK, 7 years ago

In 12738/josm:

see #15229 - checkstyle warnings

comment:15 by bastiK, 7 years ago

In 12739/josm:

see #15229 - PMD warnings

comment:16 by Don-vip, 7 years ago

In 12741/josm:

see #15229 - fix javadoc warnings

comment:17 by Don-vip, 7 years ago

In 12742/josm:

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

comment:18 by Don-vip, 7 years ago

In 12743/josm:

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

comment:19 by Don-vip, 7 years ago

In 12744/josm:

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

comment:20 by bastiK, 7 years ago

In 12745/josm:

see #15229 - deprecate LatLon#toStringCSV

removes last references of LatLon to CoordinateFormat classes

comment:21 by bastiK, 7 years ago

In 12746/josm:

see #15229 - fix deprecations in tests

comment:22 by Don-vip, 7 years ago

In 12747/josm:

see #15229 - see #15182 - fix unit tests

comment:23 by Don-vip, 7 years ago

In 12748/josm:

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

comment:24 by Don-vip, 7 years ago

In 12749/josm:

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

comment:25 by Don-vip, 7 years ago

In 12750/josm:

see #15229 - see #15182 - fix unit tests

comment:26 by Don-vip, 7 years ago

Cc: wiktorn michael2402 added

@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?

comment:27 by Don-vip, 7 years ago

In 12751/josm:

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

comment:28 by Don-vip, 7 years ago

In 12752/josm:

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

in reply to:  26 ; comment:29 by wiktorn, 7 years ago

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?

comment:30 by Don-vip, 7 years ago

In 12754/josm:

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

in reply to:  29 comment:31 by Don-vip, 7 years ago

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 :)

comment:32 by Don-vip, 7 years ago

In 12755/josm:

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

comment:33 by Don-vip, 7 years ago

In 12756/josm:

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

in reply to:  29 comment:34 by bastiK, 7 years ago

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.

comment:35 by Don-vip, 7 years ago

In 12757/josm:

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

comment:36 by Don-vip, 7 years ago

In 12758/josm:

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

comment:37 by Don-vip, 7 years ago

In 12760/josm:

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

comment:38 by Don-vip, 7 years ago

In 12761/josm:

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

comment:39 by Don-vip, 7 years ago

In 12763/josm:

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

comment:40 by Don-vip, 7 years ago

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

     
    2424import java.awt.event.MouseAdapter;
    2525import java.awt.event.MouseEvent;
    2626import java.awt.image.FilteredImageSource;
    27 import java.lang.reflect.InvocationTargetException;
    2827import java.util.Arrays;
    2928import java.util.Collection;
    3029import java.util.Enumeration;
    3130import java.util.EventObject;
    3231import java.util.Locale;
    3332import java.util.concurrent.Callable;
    34 import java.util.concurrent.ExecutionException;
    35 import java.util.concurrent.FutureTask;
    3633
    3734import javax.swing.GrayFilter;
    3835import javax.swing.ImageIcon;
     
    5956import org.openstreetmap.josm.gui.widgets.HtmlPanel;
    6057import org.openstreetmap.josm.tools.CheckParameterUtil;
    6158import org.openstreetmap.josm.tools.ColorHelper;
     59import org.openstreetmap.josm.tools.EdtHelper;
    6260import org.openstreetmap.josm.tools.GBC;
    6361import org.openstreetmap.josm.tools.ImageOverlay;
    6462import org.openstreetmap.josm.tools.ImageProvider;
     
    6664import org.openstreetmap.josm.tools.LanguageInfo;
    6765import org.openstreetmap.josm.tools.Logging;
    6866import org.openstreetmap.josm.tools.PlatformHook;
    69 import org.openstreetmap.josm.tools.bugreport.BugReport;
    7067import org.openstreetmap.josm.tools.bugreport.ReportedException;
    7168
    7269/**
     
    194191     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
    195192     * @param task The runnable to execute
    196193     * @see SwingUtilities#invokeLater
     194     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDT(Runnable)} instead
    197195     */
     196    @Deprecated
    198197    public static void runInEDT(Runnable task) {
    199         if (SwingUtilities.isEventDispatchThread()) {
    200             task.run();
    201         } else {
    202             SwingUtilities.invokeLater(task);
    203         }
     198        EdtHelper.runInEDT(task);
    204199    }
    205200
    206201    /**
     
    208203     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
    209204     * @param task The runnable to execute
    210205     * @see SwingUtilities#invokeAndWait
     206     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWait(Runnable)} instead
    211207     */
     208    @Deprecated
    212209    public static void runInEDTAndWait(Runnable task) {
    213         if (SwingUtilities.isEventDispatchThread()) {
    214             task.run();
    215         } else {
    216             try {
    217                 SwingUtilities.invokeAndWait(task);
    218             } catch (InterruptedException | InvocationTargetException e) {
    219                 Logging.error(e);
    220             }
    221         }
     210        EdtHelper.runInEDTAndWait(task);
    222211    }
    223212
    224213    /**
     
    230219     * @param task The runnable to execute
    231220     * @see SwingUtilities#invokeAndWait
    232221     * @since 10271
     222     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWaitWithException(Runnable)} instead
    233223     */
     224    @Deprecated
    234225    public static void runInEDTAndWaitWithException(Runnable task) {
    235         if (SwingUtilities.isEventDispatchThread()) {
    236             task.run();
    237         } else {
    238             try {
    239                 SwingUtilities.invokeAndWait(task);
    240             } catch (InterruptedException | InvocationTargetException e) {
    241                 throw BugReport.intercept(e).put("task", task);
    242             }
    243         }
     226        EdtHelper.runInEDTAndWaitWithException(task);
    244227    }
    245228
    246229    /**
     
    251234     * @param callable The callable to execute
    252235     * @return The computed result
    253236     * @since 7204
     237     * @deprecated to be removed end of 2017. Use {@link EdtHelper#runInEDTAndWaitAndReturn(Callable)} instead
    254238     */
     239    @Deprecated
    255240    public static <V> V runInEDTAndWaitAndReturn(Callable<V> callable) {
    256         if (SwingUtilities.isEventDispatchThread()) {
    257             try {
    258                 return callable.call();
    259             } catch (Exception e) { // NOPMD
    260                 Logging.error(e);
    261                 return null;
    262             }
    263         } else {
    264             FutureTask<V> task = new FutureTask<>(callable);
    265             SwingUtilities.invokeLater(task);
    266             try {
    267                 return task.get();
    268             } catch (InterruptedException | ExecutionException e) {
    269                 Logging.error(e);
    270                 return null;
    271             }
    272         }
     241        return EdtHelper.runInEDTAndWaitAndReturn(callable);
    273242    }
    274243
    275244    /**
    276245     * This function fails if it was not called from the EDT thread.
    277246     * @throws IllegalStateException if called from wrong thread.
    278247     * @since 10271
     248     * @deprecated to be removed end of 2017. Use {@link EdtHelper#assertCallFromEdt()} instead
    279249     */
     250    @Deprecated
    280251    public static void assertCallFromEdt() {
    281         if (!SwingUtilities.isEventDispatchThread()) {
    282             throw new IllegalStateException(
    283                     "Needs to be called from the EDT thread, not from " + Thread.currentThread().getName());
    284         }
     252        EdtHelper.assertCallFromEdt();
    285253    }
    286254
    287255    /**
  • src/org/openstreetmap/josm/tools/EdtHelper.java

     
     1// License: GPL. For details, see LICENSE file.
     2package org.openstreetmap.josm.tools;
     3
     4import java.lang.reflect.InvocationTargetException;
     5import java.util.concurrent.Callable;
     6import java.util.concurrent.ExecutionException;
     7import java.util.concurrent.FutureTask;
     8
     9import javax.swing.SwingUtilities;
     10
     11import org.openstreetmap.josm.tools.bugreport.BugReport;
     12import org.openstreetmap.josm.tools.bugreport.ReportedException;
     13
     14/**
     15 * Utilities to make life with Event Dispatch Thread (EDT) easier.
     16 * @since xxx
     17 */
     18public final class EdtHelper {
     19
     20    private EdtHelper() {
     21        // Hide default constructor for utils classes
     22    }
     23
     24    /**
     25     * Executes asynchronously a runnable in
     26     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
     27     * @param task The runnable to execute
     28     * @see SwingUtilities#invokeLater
     29     */
     30    public static void runInEDT(Runnable task) {
     31        if (SwingUtilities.isEventDispatchThread()) {
     32            task.run();
     33        } else {
     34            SwingUtilities.invokeLater(task);
     35        }
     36    }
     37
     38    /**
     39     * Executes synchronously a runnable in
     40     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
     41     * @param task The runnable to execute
     42     * @see SwingUtilities#invokeAndWait
     43     */
     44    public static void runInEDTAndWait(Runnable task) {
     45        if (SwingUtilities.isEventDispatchThread()) {
     46            task.run();
     47        } else {
     48            try {
     49                SwingUtilities.invokeAndWait(task);
     50            } catch (InterruptedException | InvocationTargetException e) {
     51                Logging.error(e);
     52            }
     53        }
     54    }
     55
     56    /**
     57     * Executes synchronously a runnable in
     58     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
     59     * <p>
     60     * Passes on the exception that was thrown to the thread calling this.
     61     * The exception is wrapped using a {@link ReportedException}.
     62     * @param task The runnable to execute
     63     * @see SwingUtilities#invokeAndWait
     64     */
     65    public static void runInEDTAndWaitWithException(Runnable task) {
     66        if (SwingUtilities.isEventDispatchThread()) {
     67            task.run();
     68        } else {
     69            try {
     70                SwingUtilities.invokeAndWait(task);
     71            } catch (InterruptedException | InvocationTargetException e) {
     72                throw BugReport.intercept(e).put("task", task);
     73            }
     74        }
     75    }
     76
     77    /**
     78     * Executes synchronously a callable in
     79     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>
     80     * and return a value.
     81     * @param <V> the result type of method <tt>call</tt>
     82     * @param callable The callable to execute
     83     * @return The computed result
     84     */
     85    public static <V> V runInEDTAndWaitAndReturn(Callable<V> callable) {
     86        if (SwingUtilities.isEventDispatchThread()) {
     87            try {
     88                return callable.call();
     89            } catch (Exception e) { // NOPMD
     90                Logging.error(e);
     91                return null;
     92            }
     93        } else {
     94            FutureTask<V> task = new FutureTask<>(callable);
     95            SwingUtilities.invokeLater(task);
     96            try {
     97                return task.get();
     98            } catch (InterruptedException | ExecutionException e) {
     99                Logging.error(e);
     100                return null;
     101            }
     102        }
     103    }
     104
     105    /**
     106     * This function fails if it was not called from the EDT thread.
     107     * @throws IllegalStateException if called from wrong thread.
     108     */
     109    public static void assertCallFromEdt() {
     110        if (!SwingUtilities.isEventDispatchThread()) {
     111            throw new IllegalStateException(
     112                    "Needs to be called from the EDT thread, not from " + Thread.currentThread().getName());
     113        }
     114    }
     115}

in reply to:  40 comment:41 by bastiK, 7 years ago

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.

comment:42 by bastiK, 7 years ago

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... :)

comment:43 by Don-vip, 7 years ago

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!

in reply to:  42 comment:44 by Don-vip, 7 years ago

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.

comment:45 by Don-vip, 7 years ago

In 12764/josm:

see #15229 - see #15182 - fix unit test

in reply to:  43 comment:46 by bastiK, 7 years ago

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.

comment:47 by wiktorn, 7 years ago

In 12765/josm:

Use tools.Logging instead of FeatureAdapter.getLogger

See: #15229

comment:48 by Don-vip, 7 years ago

In 12766/josm:

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

comment:49 by Don-vip, 7 years ago

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

comment:50 by bastiK, 7 years ago

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)

comment:51 by bastiK, 7 years ago

In 12772/josm:

see #15229 - move compression methods from Utils to Compression

comment:52 by bastiK, 7 years ago

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

comment:53 by bastiK, 7 years ago

In 12777/josm:

see #15229 - add registry for NTV2 grid file sources

removes dependency of NTV2GridShiftFileWrapper on Main

comment:54 by bastiK, 7 years ago

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

comment:55 by Don-vip, 7 years ago

In 12779/josm:

see #15229 - fix unit test

comment:56 by Don-vip, 7 years ago

In 12780/josm:

see #15229 - fix PMD warning

comment:57 by Don-vip, 7 years ago

In 12781/josm:

see #15229 - add javadoc

comment:58 by Don-vip, 7 years ago

In 12782/josm:

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

comment:59 by Don-vip, 7 years ago

In 12783/josm:

see #15229 - fix warnings

comment:60 by bastiK, 7 years ago

In 12784/josm:

see #15229 - minor cleanup

comment:61 by bastiK, 7 years ago

In 12788/josm:

see #15229 - PMD warnings

comment:62 by Don-vip, 7 years ago

In 12790/josm:

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

comment:63 by Don-vip, 7 years ago

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

Last edited 7 years ago by Don-vip (previous) (diff)

by Don-vip, 7 years ago

Attachment: jenkins.txt added

comment:64 by Don-vip, 7 years ago

In 12791/josm:

see #15229 - see #15182 - javadoc fixes

comment:65 by bastiK, 7 years ago

In [12786]:

see #15182 - remove GUI references from Projections class

in reply to:  63 comment:66 by bastiK, 7 years ago

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.

Last edited 7 years ago by Don-vip (previous) (diff)

comment:67 by bastiK, 7 years ago

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

comment:68 by bastiK, 7 years ago

In 12793/josm:

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

comment:69 by bastiK, 7 years ago

In 12795/josm:

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

comment:70 by Don-vip, 7 years ago

In 12796/josm:

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

comment:71 by Don-vip, 7 years ago

In 12799/josm:

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

comment:72 by Don-vip, 7 years ago

In 12800/josm:

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

comment:73 by Don-vip, 7 years ago

In 12803/josm:

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

comment:74 by Don-vip, 7 years ago

In 12804/josm:

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

comment:75 by Don-vip, 7 years ago

In 12805/josm:

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

comment:76 by Don-vip, 7 years ago

In 12806/josm:

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

comment:77 by Don-vip, 7 years ago

In 12807/josm:

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

comment:78 by Don-vip, 7 years ago

In 12816/josm:

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

comment:79 by bastiK, 7 years ago

In 12817/josm:

see #15229 - deprecate unused method

comment:80 by bastiK, 7 years ago

In 12818/josm:

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

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

comment:81 by bastiK, 7 years ago

In 12819/josm:

see #15229 - trace world bounds in finer steps

no efficiency problem as result is cached

comment:82 by bastiK, 7 years ago

In 12820/josm:

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

comment:83 by Don-vip, 7 years ago

In 12821/josm:

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

comment:84 by Don-vip, 7 years ago

In 12823/josm:

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

comment:85 by Don-vip, 7 years ago

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:

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

comment:86 by Don-vip, 7 years ago

In 12825/josm:

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

comment:87 by Don-vip, 7 years ago

In 12826/josm:

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

in reply to:  85 ; comment:88 by bastiK, 7 years ago

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:

  1. 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 ;)

in reply to:  88 ; comment:89 by Don-vip, 7 years ago

Replying to bastiK:

gui.mappaint.gui ;)

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

comment:90 by bastiK, 7 years ago

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. :)

comment:91 by Don-vip, 7 years ago

In 12828/josm:

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

comment:92 by bastiK, 7 years ago

In 12831/josm:

see #15229 - extract GUI from StyleSetting class

in reply to:  89 comment:93 by bastiK, 7 years ago

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).

comment:94 by bastiK, 7 years ago

In 12832/josm:

see #15229 - fix checkstyle warning

comment:95 by Don-vip, 7 years ago

In 12834/josm:

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

comment:96 by Don-vip, 7 years ago

In 12835/josm:

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

comment:97 by bastiK, 7 years ago

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
    

comment:98 by bastiK, 7 years ago

In 12841/josm:

see #15229 - fix deprecations caused by [12840]

comment:99 by bastiK, 7 years ago

In 12842/josm:

see #15229 - fix test deprecations

comment:100 by bastiK, 7 years ago

In 12843/josm:

see #15229 - minor fixes (Jenkins)

comment:101 by bastiK, 7 years ago

In 12844/josm:

see #15229 - one more minor style fix

comment:102 by bastiK, 7 years ago

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

     
    144144import org.openstreetmap.josm.io.remotecontrol.RemoteControl;
    145145import org.openstreetmap.josm.plugins.PluginHandler;
    146146import org.openstreetmap.josm.plugins.PluginInformation;
     147import org.openstreetmap.josm.spi.preferences.Config;
    147148import org.openstreetmap.josm.tools.FontsManager;
    148149import org.openstreetmap.josm.tools.GBC;
    149150import org.openstreetmap.josm.tools.HttpClient;
     
    920921            I18n.set(Main.pref.get("language", null));
    921922        }
    922923        Main.pref.updateSystemProperties();
     924        Config.setPreferencesInstance(Main.pref);
    923925
    924926        checkIPv6();
    925927
  • src/org/openstreetmap/josm/gui/util/WindowGeometry.java

     
    1919import javax.swing.JComponent;
    2020
    2121import org.openstreetmap.josm.Main;
     22import org.openstreetmap.josm.spi.preferences.Config;
    2223import org.openstreetmap.josm.tools.CheckParameterUtil;
    2324import org.openstreetmap.josm.tools.JosmRuntimeException;
    2425import org.openstreetmap.josm.tools.Logging;
     
    205206    }
    206207
    207208    protected final void initFromPreferences(String preferenceKey) throws WindowGeometryException {
    208         String value = Main.pref.get(preferenceKey);
     209        String value = Config.getPref().get(preferenceKey);
    209210        if (value.isEmpty())
    210211            throw new WindowGeometryException(
    211212                    tr("Preference with key ''{0}'' does not exist. Cannot restore window geometry from preferences.", preferenceKey));
     
    274275        StringBuilder value = new StringBuilder(32);
    275276        value.append("x=").append(topLeft.x).append(",y=").append(topLeft.y)
    276277             .append(",width=").append(extent.width).append(",height=").append(extent.height);
    277         Main.pref.put(preferenceKey, value.toString());
     278        Config.getPref().put(preferenceKey, value.toString());
    278279    }
    279280
    280281    /**
  • src/org/openstreetmap/josm/spi/preferences/Config.java

     
     1// License: GPL. For details, see LICENSE file.
     2package org.openstreetmap.josm.spi.preferences;
     3
     4import org.openstreetmap.josm.data.preferences.IPreferences;
     5
     6/**
     7 * Class to hold the global preferences object.
     8 */
     9public class Config {
     10
     11    private static IPreferences preferences;
     12
     13    /**
     14     * Get the preferences.
     15     * @return the preferences
     16     */
     17    public static IPreferences getPref() {
     18        return preferences;
     19    }
     20
     21    /**
     22     * Install the global preference instance.
     23     * @param preferences the global preference instance to set
     24     */
     25    public static void setPreferencesInstance(IPreferences preferences) {
     26        Config.preferences = preferences;
     27    }
     28}

comment:103 by Don-vip, 7 years ago

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 :)

comment:104 by bastiK, 7 years ago

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)

comment:105 by bastiK, 7 years ago

In 12846/josm:

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

comment:106 by bastiK, 7 years ago

In 12847/josm:

see #15229 - move new classes to spi package

in reply to:  105 comment:107 by Don-vip, 7 years ago

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

Last edited 7 years ago by Don-vip (previous) (diff)

comment:108 by bastiK, 7 years ago

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

comment:109 by bastiK, 7 years ago

In 12848/josm:

see #15229 - set up Config for tests

comment:110 by floscher, 7 years ago

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

But still impressive @bastiK ;) 😉

comment:111 by bastiK, 7 years ago

In 12849/josm:

see #15229 - use Config in tests

comment:112 by bastiK, 7 years ago

In 12850/josm:

see #15229 - use Config in functional tests

in reply to:  110 comment:113 by Don-vip, 7 years ago

Replying to floscher:

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

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.

comment:114 by bastiK, 7 years ago

In 12851/josm:

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

comment:115 by bastiK, 7 years ago

In 12852/josm:

see #15229 - fix taginfo script

comment:116 by bastiK, 7 years ago

In 12853/josm:

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

comment:117 by bastiK, 7 years ago

In 12854/josm:

see #15229 - fix checkstyle warnings

comment:118 by bastiK, 7 years ago

In 12855/josm:

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

comment:119 by bastiK, 7 years ago

In 12856/josm:

see #15229 - add parameter to base directory methods

comment:120 by bastiK, 7 years ago

In 12857/josm:

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

comment:121 by Don-vip, 7 years ago

In 12858/josm:

see #15229 - fix integration tests

comment:122 by Don-vip, 7 years ago

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

comment:123 by Don-vip, 7 years ago

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

comment:124 by bastiK, 7 years ago

In 12860/josm:

see #15229 - add package-info.java

in reply to:  122 comment:125 by bastiK, 7 years ago

Replying to Don-vip:

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

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

comment:126 by Don-vip, 7 years ago

It displays the package description on doc.

comment:127 by Don-vip, 7 years ago

In 12869/josm:

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

comment:128 by Don-vip, 7 years ago

In 12870/josm:

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

comment:129 by bastiK, 7 years ago

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

comment:130 by bastiK, 6 years ago

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

comment:131 by bastiK, 6 years ago

In 12882/josm:

see #15229 - fix @since

in reply to:  130 comment:132 by Don-vip, 6 years ago

Replying to bastiK:

In 12881/josm:

* move `*Setting` classes

Tests must be updated.

comment:133 by bastiK, 6 years ago

In 12884/josm:

see #15229 - fix unit tests

comment:134 by bastiK, 6 years ago

In 12885/josm:

see #15229 - fix checkstyle warnings

comment:135 by simon04, 6 years ago

In 12886/josm:

see #15229 - see #15182 - fix typo

comment:136 by bastiK, 6 years ago

In 12891/josm:

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

comment:137 by bastiK, 6 years ago

In 12892/josm:

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

comment:138 by bastiK, 6 years ago

In 12893/josm:

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

comment:139 by bastiK, 6 years ago

In 12894/josm:

see #15229 - update method name and signature for consistency

comment:140 by bastiK, 6 years ago

In 12906/josm:

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

run josm render --help to see the options

comment:141 by bastiK, 6 years ago

In 12907/josm:

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

comment:142 by bastiK, 6 years ago

In 12928/josm:

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

comment:143 by bastiK, 6 years ago

In 12999/josm:

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

comment:144 by bastiK, 6 years ago

In 13002/josm:

see #15229 - remove some uses of Main.pref

comment:145 by bastiK, 6 years ago

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"

comment:146 by Don-vip, 6 years ago

In 13140/josm:

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

comment:147 by Don-vip, 6 years ago

Priority: normalmajor

comment:148 by Don-vip, 6 years ago

In 14118/josm:

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

by Don-vip, 6 years ago

Attachment: graph-cda-20180811.png added

comment:149 by Don-vip, 6 years ago

Current dependencies graph using CDA:


Last edited 6 years ago by Don-vip (previous) (diff)

by Don-vip, 6 years ago

Attachment: graph-jdeps-20180811.png added

comment:150 by Don-vip, 6 years ago

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


Last edited 6 years ago by Don-vip (previous) (diff)

comment:151 by Don-vip, 6 years ago

In 14119/josm:

see #15229 - deprecate all Main methods returning an URL

comment:152 by Don-vip, 6 years ago

In 14120/josm:

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

comment:153 by Don-vip, 6 years ago

In 14121/josm:

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

comment:154 by Don-vip, 6 years ago

In 14122/josm:

see #15229 - minor preferences refactoring

comment:155 by Don-vip, 6 years ago

In 14125/josm:

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

comment:156 by Don-vip, 6 years ago

In 14128/josm:

see #15229 - deprecate Main.fileWatcher

in reply to:  155 comment:157 by Klumbumbus, 6 years ago

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.

comment:158 by Don-vip, 6 years ago

In 14131/josm:

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

comment:159 by Don-vip, 6 years ago

In 14134/josm:

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

comment:160 by Don-vip, 6 years ago

In 14135/josm:

see #15229 - fix deprecation warnings

comment:161 by Don-vip, 6 years ago

In 14136/josm:

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

by Don-vip, 6 years ago

by Don-vip, 6 years ago

by Don-vip, 6 years ago

comment:162 by Don-vip, 6 years ago

We can now generate three dependencies graphs using ant jdeps:

JOSM classes only:

JOSM classes and direct dependencies:

JOSM classes and all dependencies:

comment:163 by Don-vip, 6 years ago

In 14137/josm:

see #15229 - checkstyle

comment:164 by Don-vip, 6 years ago

In 14138/josm:

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

comment:165 by Don-vip, 6 years ago

In 14139/josm:

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

comment:166 by Don-vip, 6 years ago

In 14140/josm:

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

comment:167 by Don-vip, 6 years ago

In 14143/josm:

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

comment:168 by Don-vip, 6 years ago

In 14144/josm:

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

comment:169 by Don-vip, 6 years ago

In 14145/josm:

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

comment:170 by Don-vip, 6 years ago

In 14146/josm:

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

comment:171 by Don-vip, 6 years ago

In 14147/josm:

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

comment:172 by Don-vip, 6 years ago

In 14148/josm:

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

comment:173 by Don-vip, 6 years ago

In 14149/josm:

see #15229 - deprecate Main.pref

comment:174 by Don-vip, 6 years ago

In 14150/josm:

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

comment:175 by Don-vip, 6 years ago

In 14153/josm:

see #15229 - deprecate Main.parent and Main itself

comment:176 by Don-vip, 6 years ago

In 14154/josm:

see #15229 - fix warnings

comment:177 by Don-vip, 6 years ago

Plugins updated in [o34452:34575].

comment:178 by Don-vip, 6 years ago

In 14253/josm:

see #15229 - kill Main. RIP

comment:179 by Don-vip, 6 years ago

In 14254/josm:

see #15229 - update spotbugs filters

comment:180 by Don-vip, 6 years ago

Updated graph:

comment:181 by Don-vip, 6 years ago

In 14255/josm:

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

comment:182 by Don-vip, 6 years ago

In 14256/josm:

see #15229 - move corrector package to actions.corrector

by michael2402, 5 years ago

Attachment: modules.png added

comment:183 by michael2402, 5 years ago

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.


by michael2402, 5 years ago

Attachment: modules.2.png added

Update some missing dependencies

comment:184 by stoecker, 5 years ago

Update some missing dependencies

comment:185 by michael2402, 5 years ago

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

comment:186 by Don-vip, 5 years ago

Keywords: hack-weekend-2018-10 added

by simon04, 5 years ago

comment:187 by Don-vip, 5 years ago

Milestone: Longterm

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to bastiK.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.