Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8987 closed enhancement (fixed)

immutable coordinates

Reported by: shinigami Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

LatLon and EastNorth were declared to be immutable, but they were derived from mutable Point2D.
I made their parent - Coordinate - really immutable. It needed few other changes in code.
This patch will allow other subsequent changes - removing defensive copying, better caching of coord objects and so..

Attachments (4)

coords.diff (13.7 KB) - added by shinigami 6 years ago.
coords2.diff (499 bytes) - added by shinigami 6 years ago.
equals() NPE fix
core.diff (10.5 KB) - added by shinigami 6 years ago.
plugins.diff (796 bytes) - added by shinigami 6 years ago.

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by shinigami

Attachment: coords.diff added

comment:1 Changed 6 years ago by shinigami

Type: defectenhancement

comment:2 Changed 6 years ago by bastiK

Resolution: fixed
Status: newclosed

In 6162/josm:

applied #8987 - immutable coordinates (patch by shinigami)

comment:3 Changed 6 years ago by bastiK

Great, thanks!

comment:5 Changed 6 years ago by Don-vip

In 6163/josm:

fix #8987 - fix Coordinate.equals() broken in r6162

comment:6 Changed 6 years ago by Don-vip

And most plugins must be recompiled '

comment:7 Changed 6 years ago by Don-vip

Resolution: fixed
Status: closedreopened

comment:8 Changed 6 years ago by Don-vip

Description	Resource	Path	Location	Type
The method setLocation(double, double) is undefined for the type EastNorth	DownloadSVGTask.java	/JOSM-Cadastre-fr/src/cadastre_fr	line 158	Java Problem
The method setLocation(double, double) is undefined for the type EastNorth	SpreadSheetReader.java	/JOSM-OpenData/src/org/openstreetmap/josm/plugins/opendata/core/io/tabular	line 215	Java Problem
The method distanceSq(LatLon) is undefined for the type LatLon	Building.java	/JOSM-buildings_tools/src/buildings_tools	line 213	Java Problem
Type mismatch: cannot convert from LatLon to Point2D	ReplaceGeometryUtils.java	/JOSM-utilsplugin2/src/org/openstreetmap/josm/plugins/utilsplugin2/replacegeometry	line 483	Java Problem
The method transform(Point2D, Point2D) in the type AffineTransform is not applicable for the arguments (Point2D, EastNorth)	FilePlacement.java	/JOSM-pdfimport/src/pdfimport	line 206	Java Problem
The method setLocation(double, double) is undefined for the type EastNorth	SpreadSheetReader.java	/JOSM-OpenData/src/org/openstreetmap/josm/plugins/opendata/core/io/tabular	line 221	Java Problem
The method contains(Point2D) in the type Area is not applicable for the arguments (LatLon)	ReplaceGeometryUtils.java	/JOSM-utilsplugin2/src/org/openstreetmap/josm/plugins/utilsplugin2/replacegeometry	line 402	Java Problem
The method distance(EastNorth) in the type EastNorth is not applicable for the arguments (double, double)	WMSAdjustAction.java	/JOSM-Cadastre-fr/src/cadastre_fr	line 130	Java Problem
The method distance(EastNorth) in the type EastNorth is not applicable for the arguments (double, double)	WMSAdjustAction.java	/JOSM-Cadastre-fr/src/cadastre_fr	line 131	Java Problem
The method clone() from the type Object is not visible	TrackStoplistNameCommand.java	/JOSM-public_transport/src/public_transport	line 37	Java Problem
The method setLocation(double, double) is undefined for the type EastNorth	DownloadSVGBuilding.java	/JOSM-Cadastre-fr/src/cadastre_fr	line 186	Java Problem
Type mismatch: cannot convert from EastNorth to Point2D	OsbDownloadLoop.java	/JOSM-openstreetbugs/src/org/openstreetmap/josm/plugins/osb	line 72	Java Problem
The method contains(Point2D) in the type Area is not applicable for the arguments (LatLon)	SplittingMultipolygons.java	/JOSM-reltoolbox/src/relcontext/actions	line 29	Java Problem
The method distance(LatLon) in the type LatLon is not applicable for the arguments (Point2D)	ReplaceGeometryUtils.java	/JOSM-utilsplugin2/src/org/openstreetmap/josm/plugins/utilsplugin2/replacegeometry	line 486	Java Problem

comment:9 Changed 6 years ago by Don-vip

In 6165/josm:

see #8987 - Move distanceSq() from EastNorth to Coordinate to fix broken plugins (comes from old Point2D)

comment:10 Changed 6 years ago by Don-vip

In 6166/josm:

see #8987 - refactor/add missing distance() methods to fix broken plugins

comment:11 Changed 6 years ago by Don-vip

In 6167/josm:

see #8987 - add missing clone() methods to fix broken plugins

comment:12 Changed 6 years ago by Don-vip

In 6168/josm:

see #8987 - make it public

comment:13 Changed 6 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

Fixed in [o29854] and [o29857]. For next patches, please think about plugins too :)

comment:14 Changed 6 years ago by rickmastfan67

Resolution: fixed
Status: closedreopened

Don, seems the update to the "openstreetbugs" plugin didn't get done correctly. Just got a NPE with it after just downloading a small bit of data from the OSM server.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2013-08-21 01:34:58
Last Changed Author: Don-vip
Revision: 6168
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2013-08-21 03:17:02 +0200 (Wed, 21 Aug 2013)
Last Changed Rev: 6168

Identification: JOSM/1.5 (6168 en) Windows 7 64-Bit
Memory Usage: 218 MB / 1820 MB (112 MB allocated, but free)
Java version: 1.7.0_25, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
VM arguments: [-Xmx2048M]
Dataset consistency test: No problems found

Plugin: ImageryCache (29769)
Plugin: OpeningHoursEditor (29854)
Plugin: buildings_tools (29854)
Plugin: mapdust (29854)
Plugin: measurement (29854)
Plugin: mirrored_download (29854)
Plugin: openstreetbugs (29854)
Plugin: osmarender (29639)
Plugin: reverter (29854)
Plugin: turnrestrictions (29854)
Plugin: undelete (29854)
Plugin: utilsplugin2 (29854)

java.lang.NullPointerException
	at org.openstreetmap.josm.data.coor.Coordinate.equals(Coordinate.java:107)
	at org.openstreetmap.josm.data.coor.EastNorth.equals(EastNorth.java:11)
	at org.openstreetmap.josm.plugins.osb.OsbDownloadLoop.run(OsbDownloadLoop.java:73)
Last edited 6 years ago by rickmastfan67 (previous) (diff)

comment:15 Changed 6 years ago by AlfonZ

Introduced findbugs warnings:

<BugInstance type="NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT" priority="2" abbrev="NP" category="BAD_PRACTICE">
  <Class classname="org.openstreetmap.josm.data.coor.Coordinate">
    <SourceLine classname="org.openstreetmap.josm.data.coor.Coordinate" start="29" end="114" sourcefile="Coordinate.java" sourcepath="org/openstreetmap/josm/data/coor/Coordinate.java"/>
  </Class>
  <Method classname="org.openstreetmap.josm.data.coor.Coordinate" name="equals" signature="(Ljava/lang/Object;)Z" isStatic="false">
    <SourceLine classname="org.openstreetmap.josm.data.coor.Coordinate" start="105" end="114" startBytecode="0" endBytecode="181" sourcefile="Coordinate.java" sourcepath="org/openstreetmap/josm/data/coor/Coordinate.java"/>
  </Method>
  <LocalVariable name="obj" register="1" pc="0" role="LOCAL_VARIABLE_NAMED"/>
</BugInstance>
<BugInstance type="CN_IDIOM_NO_SUPER_CALL" priority="2" abbrev="CN" category="BAD_PRACTICE">
  <Class classname="org.openstreetmap.josm.data.coor.EastNorth">
    <SourceLine classname="org.openstreetmap.josm.data.coor.EastNorth" start="11" end="135" sourcefile="EastNorth.java" sourcepath="org/openstreetmap/josm/data/coor/EastNorth.java"/>
  </Class>
  <Method classname="org.openstreetmap.josm.data.coor.EastNorth" name="clone" signature="()Lorg/openstreetmap/josm/data/coor/EastNorth;" isStatic="false">
    <SourceLine classname="org.openstreetmap.josm.data.coor.EastNorth" start="135" end="135" startBytecode="0" endBytecode="57" sourcefile="EastNorth.java" sourcepath="org/openstreetmap/josm/data/coor/EastNorth.java"/>
  </Method>
</BugInstance>
<BugInstance type="CN_IDIOM_NO_SUPER_CALL" priority="1" abbrev="CN" category="BAD_PRACTICE">
  <Class classname="org.openstreetmap.josm.data.coor.LatLon">
    <SourceLine classname="org.openstreetmap.josm.data.coor.LatLon" start="28" end="400" sourcefile="LatLon.java" sourcepath="org/openstreetmap/josm/data/coor/LatLon.java"/>
  </Class>
  <Method classname="org.openstreetmap.josm.data.coor.LatLon" name="clone" signature="()Lorg/openstreetmap/josm/data/coor/LatLon;" isStatic="false">
    <SourceLine classname="org.openstreetmap.josm.data.coor.LatLon" start="400" end="400" startBytecode="0" endBytecode="57" sourcefile="LatLon.java" sourcepath="org/openstreetmap/josm/data/coor/LatLon.java"/>
  </Method>
  <Class classname="org.openstreetmap.josm.data.coor.CachedLatLon" role="CLASS_SUBCLASS">
    <SourceLine classname="org.openstreetmap.josm.data.coor.CachedLatLon" start="21" end="54" sourcefile="CachedLatLon.java" sourcepath="org/openstreetmap/josm/data/coor/CachedLatLon.java"/>
  </Class>
</BugInstance>

http://findbugs.sourceforge.net/bugDescriptions.html#NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT
http://findbugs.sourceforge.net/bugDescriptions.html#CN_IDIOM_NO_SUPER_CALL

Changed 6 years ago by shinigami

Attachment: coords2.diff added

equals() NPE fix

comment:16 Changed 6 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 6169/josm:

fix #8987 - Fix NPE and Findbugs/Sonar violations

comment:17 Changed 6 years ago by shinigami

I am sorry guys, did not have plugins checked out.

When I checkout http://svn.openstreetmap.org/applications/editors/josm, the core dir is the same as http://josm.openstreetmap.de/svn/trunk ?

comment:18 in reply to:  17 Changed 6 years ago by Don-vip

Replying to shinigami:

I am sorry guys, did not have plugins checked out.

No problem, it happened to everyone :)

When I checkout http://svn.openstreetmap.org/applications/editors/josm, the core dir is the same as http://josm.openstreetmap.de/svn/trunk ?

Yep :) "core" is an svn:external link to our SVN repo.

Changed 6 years ago by shinigami

Attachment: core.diff added

Changed 6 years ago by shinigami

Attachment: plugins.diff added

comment:19 Changed 6 years ago by shinigami

This removes Cloneable from LatLon/EastNorth and uses of copy constructors. Plugins included;)

comment:20 Changed 6 years ago by Don-vip

Resolution: fixed
Status: closedreopened

comment:21 Changed 6 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 6173/josm:

fix #8987 - immutable coordinates (patch by shinigami)

comment:22 Changed 6 years ago by Don-vip

In 6226/josm:

fix #9057 - see #8987 - fix Coordinate.hashCode() broken in r6162

Modify Ticket

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

Add Comment


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

 
Note: See TracTickets for help on using tickets.