Modify

Opened 3 weeks ago

Last modified 27 hours ago

#15229 new enhancement

modular structure for JOSM core

Reported by: bastiK Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: modularization 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 (2)

deprecate_ILatLon#getEastNorth().patch (25.2 KB) - added by bastiK 3 weeks ago.
jenkins.txt (78.7 KB) - added by Don-vip 2 weeks ago.

Download all attachments as: .zip

Change History (137)

comment:1 Changed 3 weeks ago by bastiK

In 12713/josm:

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

comment:2 Changed 3 weeks ago by 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.

comment:3 Changed 3 weeks ago by Don-vip

Keywords: modularization added
Type: defectenhancement

comment:4 Changed 3 weeks ago by Don-vip

Description: modified (diff)

comment:5 in reply to:  2 Changed 3 weeks ago by 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.

comment:6 Changed 3 weeks ago by Don-vip

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

comment:7 Changed 3 weeks ago by 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

Changed 3 weeks ago by bastiK

comment:8 Changed 3 weeks ago by 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.

comment:9 Changed 3 weeks ago by Don-vip

nice :)

comment:10 Changed 3 weeks ago by bastiK

In 12725/josm:

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

comment:11 Changed 3 weeks ago by Don-vip

In 12729/josm:

see #15229 - fix typos, warnings

comment:12 Changed 3 weeks ago by Don-vip

In 12734/josm:

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

comment:13 Changed 3 weeks ago by bastiK

In 12735/josm:

see #15229 - move CoordinateFormat code out of LatLon class

comment:14 Changed 3 weeks ago by bastiK

In 12738/josm:

see #15229 - checkstyle warnings

comment:15 Changed 3 weeks ago by bastiK

In 12739/josm:

see #15229 - PMD warnings

comment:16 Changed 3 weeks ago by Don-vip

In 12741/josm:

see #15229 - fix javadoc warnings

comment:17 Changed 3 weeks ago by 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

comment:18 Changed 3 weeks ago by Don-vip

In 12743/josm:

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

comment:19 Changed 3 weeks ago by Don-vip

In 12744/josm:

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

comment:20 Changed 3 weeks ago by bastiK

In 12745/josm:

see #15229 - deprecate LatLon#toStringCSV

removes last references of LatLon to CoordinateFormat classes

comment:21 Changed 3 weeks ago by bastiK

In 12746/josm:

see #15229 - fix deprecations in tests

comment:22 Changed 3 weeks ago by Don-vip

In 12747/josm:

see #15229 - see #15182 - fix unit tests

comment:23 Changed 3 weeks ago by Don-vip

In 12748/josm:

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

comment:24 Changed 3 weeks ago by Don-vip

In 12749/josm:

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

comment:25 Changed 3 weeks ago by Don-vip

In 12750/josm:

see #15229 - see #15182 - fix unit tests

comment:26 Changed 3 weeks ago by Don-vip

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 Changed 2 weeks ago by Don-vip

In 12751/josm:

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

comment:28 Changed 2 weeks ago by Don-vip

In 12752/josm:

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

comment:29 in reply to:  26 ; Changed 2 weeks ago by 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?

comment:30 Changed 2 weeks ago by Don-vip

In 12754/josm:

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

comment:31 in reply to:  29 Changed 2 weeks ago by 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 :)

comment:32 Changed 2 weeks ago by Don-vip

In 12755/josm:

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

comment:33 Changed 2 weeks ago by Don-vip

In 12756/josm:

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

comment:34 in reply to:  29 Changed 2 weeks ago by 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.

comment:35 Changed 2 weeks ago by Don-vip

In 12757/josm:

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

comment:36 Changed 2 weeks ago by Don-vip

In 12758/josm:

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

comment:37 Changed 2 weeks ago by Don-vip

In 12760/josm:

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

comment:38 Changed 2 weeks ago by Don-vip

In 12761/josm:

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

comment:39 Changed 2 weeks ago by Don-vip

In 12763/josm:

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

comment:40 Changed 2 weeks ago by 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

     
    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}

comment:41 in reply to:  40 Changed 2 weeks ago by 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.

comment:42 Changed 2 weeks ago by 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... :)

comment:43 Changed 2 weeks ago by 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!

comment:44 in reply to:  42 Changed 2 weeks ago by 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.

comment:45 Changed 2 weeks ago by Don-vip

In 12764/josm:

see #15229 - see #15182 - fix unit test

comment:46 in reply to:  43 Changed 2 weeks ago by 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.

comment:47 Changed 2 weeks ago by wiktorn

In 12765/josm:

Use tools.Logging instead of FeatureAdapter.getLogger

See: #15229

comment:48 Changed 2 weeks ago by Don-vip

In 12766/josm:

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

comment:49 Changed 2 weeks ago by 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

comment:50 Changed 2 weeks ago by 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)

comment:51 Changed 2 weeks ago by bastiK

In 12772/josm:

see #15229 - move compression methods from Utils to Compression

comment:52 Changed 2 weeks ago by 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

comment:53 Changed 2 weeks ago by bastiK

In 12777/josm:

see #15229 - add registry for NTV2 grid file sources

removes dependency of NTV2GridShiftFileWrapper on Main

comment:54 Changed 2 weeks ago by 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

comment:55 Changed 2 weeks ago by Don-vip

In 12779/josm:

see #15229 - fix unit test

comment:56 Changed 2 weeks ago by Don-vip

In 12780/josm:

see #15229 - fix PMD warning

comment:57 Changed 2 weeks ago by Don-vip

In 12781/josm:

see #15229 - add javadoc

comment:58 Changed 2 weeks ago by Don-vip

In 12782/josm:

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

comment:59 Changed 2 weeks ago by Don-vip

In 12783/josm:

see #15229 - fix warnings

comment:60 Changed 2 weeks ago by bastiK

In 12784/josm:

see #15229 - minor cleanup

comment:61 Changed 2 weeks ago by bastiK

In 12788/josm:

see #15229 - PMD warnings

comment:62 Changed 2 weeks ago by Don-vip

In 12790/josm:

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

comment:63 Changed 2 weeks ago by Don-vip

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

Last edited 2 weeks ago by Don-vip (previous) (diff)

Changed 2 weeks ago by Don-vip

Attachment: jenkins.txt added

comment:64 Changed 2 weeks ago by Don-vip

In 12791/josm:

see #15229 - see #15182 - javadoc fixes

comment:65 Changed 2 weeks ago by bastiK

In [12786]:

see #15182 - remove GUI references from Projections class

comment:66 in reply to:  63 Changed 2 weeks ago by 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.

Last edited 2 weeks ago by Don-vip (previous) (diff)

comment:67 Changed 2 weeks ago by 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

comment:68 Changed 2 weeks ago by bastiK

In 12793/josm:

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

comment:69 Changed 2 weeks ago by bastiK

In 12795/josm:

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

comment:70 Changed 2 weeks ago by Don-vip

In 12796/josm:

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

comment:71 Changed 2 weeks ago by Don-vip

In 12799/josm:

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

comment:72 Changed 2 weeks ago by Don-vip

In 12800/josm:

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

comment:73 Changed 2 weeks ago by Don-vip

In 12803/josm:

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

comment:74 Changed 2 weeks ago by Don-vip

In 12804/josm:

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

comment:75 Changed 2 weeks ago by Don-vip

In 12805/josm:

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

comment:76 Changed 2 weeks ago by Don-vip

In 12806/josm:

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

comment:77 Changed 2 weeks ago by Don-vip

In 12807/josm:

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

comment:78 Changed 13 days ago by Don-vip

In 12816/josm:

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

comment:79 Changed 13 days ago by bastiK

In 12817/josm:

see #15229 - deprecate unused method

comment:80 Changed 13 days ago by 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

comment:81 Changed 13 days ago by bastiK

In 12819/josm:

see #15229 - trace world bounds in finer steps

no efficiency problem as result is cached

comment:82 Changed 13 days ago by bastiK

In 12820/josm:

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

comment:83 Changed 13 days ago by Don-vip

In 12821/josm:

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

comment:84 Changed 12 days ago by Don-vip

In 12823/josm:

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

comment:85 Changed 12 days ago by 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:

  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 Changed 12 days ago by Don-vip

In 12825/josm:

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

comment:87 Changed 12 days ago by Don-vip

In 12826/josm:

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

comment:88 in reply to:  85 ; Changed 12 days ago by 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:

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

comment:89 in reply to:  88 ; Changed 11 days ago by Don-vip

Replying to bastiK:

gui.mappaint.gui ;)

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

comment:90 Changed 11 days ago by 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. :)

comment:91 Changed 11 days ago by Don-vip

In 12828/josm:

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

comment:92 Changed 11 days ago by bastiK

In 12831/josm:

see #15229 - extract GUI from StyleSetting class

comment:93 in reply to:  89 Changed 11 days ago by 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).

comment:94 Changed 11 days ago by bastiK

In 12832/josm:

see #15229 - fix checkstyle warning

comment:95 Changed 11 days ago by Don-vip

In 12834/josm:

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

comment:96 Changed 11 days ago by Don-vip

In 12835/josm:

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

comment:97 Changed 10 days ago by 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
    

comment:98 Changed 10 days ago by bastiK

In 12841/josm:

see #15229 - fix deprecations caused by [12840]

comment:99 Changed 10 days ago by bastiK

In 12842/josm:

see #15229 - fix test deprecations

comment:100 Changed 10 days ago by bastiK

In 12843/josm:

see #15229 - minor fixes (Jenkins)

comment:101 Changed 10 days ago by bastiK

In 12844/josm:

see #15229 - one more minor style fix

comment:102 Changed 10 days ago by 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

     
    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 Changed 10 days ago by 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 :)

comment:104 Changed 10 days ago by 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)

comment:105 Changed 10 days ago by bastiK

In 12846/josm:

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

comment:106 Changed 10 days ago by bastiK

In 12847/josm:

see #15229 - move new classes to spi package

comment:107 in reply to:  105 Changed 10 days ago by 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

Last edited 10 days ago by Don-vip (previous) (diff)

comment:108 Changed 10 days ago by bastiK

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

comment:109 Changed 10 days ago by bastiK

In 12848/josm:

see #15229 - set up Config for tests

comment:110 Changed 10 days ago by floscher

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

But still impressive @bastiK ;) 😉

comment:111 Changed 10 days ago by bastiK

In 12849/josm:

see #15229 - use Config in tests

comment:112 Changed 10 days ago by bastiK

In 12850/josm:

see #15229 - use Config in functional tests

comment:113 in reply to:  110 Changed 10 days ago by Don-vip

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 Changed 10 days ago by bastiK

In 12851/josm:

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

comment:115 Changed 10 days ago by bastiK

In 12852/josm:

see #15229 - fix taginfo script

comment:116 Changed 10 days ago by bastiK

In 12853/josm:

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

comment:117 Changed 9 days ago by bastiK

In 12854/josm:

see #15229 - fix checkstyle warnings

comment:118 Changed 9 days ago by bastiK

In 12855/josm:

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

comment:119 Changed 9 days ago by bastiK

In 12856/josm:

see #15229 - add parameter to base directory methods

comment:120 Changed 9 days ago by bastiK

In 12857/josm:

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

comment:121 Changed 9 days ago by Don-vip

In 12858/josm:

see #15229 - fix integration tests

comment:122 Changed 9 days ago by Don-vip

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

comment:123 Changed 9 days ago by 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

comment:124 Changed 9 days ago by bastiK

In 12860/josm:

see #15229 - add package-info.java

comment:125 in reply to:  122 Changed 9 days ago by bastiK

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 Changed 9 days ago by Don-vip

It displays the package description on doc.

comment:127 Changed 8 days ago by Don-vip

In 12869/josm:

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

comment:128 Changed 7 days ago by Don-vip

In 12870/josm:

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

comment:129 Changed 4 days ago by bastiK

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

comment:130 Changed 2 days ago by 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

comment:131 Changed 2 days ago by bastiK

In 12882/josm:

see #15229 - fix @since

comment:132 in reply to:  130 Changed 44 hours ago by Don-vip

Replying to bastiK:

In 12881/josm:

* move `*Setting` classes

Tests must be updated.

comment:133 Changed 41 hours ago by bastiK

In 12884/josm:

see #15229 - fix unit tests

comment:134 Changed 40 hours ago by bastiK

In 12885/josm:

see #15229 - fix checkstyle warnings

comment:135 Changed 27 hours ago by simon04

In 12886/josm:

see #15229 - see #15182 - fix typo

Modify Ticket

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

Add Comment


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

 
Note: See TracTickets for help on using tickets.