Modify

Opened 11 years ago

Closed 11 years ago

#2302 closed defect (fixed)

[PATCH] OsmPrimite shouldn't expose public fields

Reported by: jttt Owned by: team
Priority: major Milestone:
Component: Core Version:
Keywords: Cc:

Description

Currently OsmPrimitive and its descendants (Node, Way and Relation) expose public fields which makes it hard to implement caching or spatial index to speed up rendering.

It should be changed so that internal state of OsmPrimitive is modifiable only using methods or at least accessors.

Attachments (7)

timestamp.patch (16.5 KB) - added by jttt 11 years ago.
Patch to replace timestamp and parsedTimestap with setter/getter
tagged.patch (12.5 KB) - added by jttt 11 years ago.
Patch to change hasDirectionKeys and isTagged to read only properties
plugins_tagged.patch (3.3 KB) - added by jttt 11 years ago.
Patch to replace hasDirectionKeys and isTagged with readonly properties (in plugins)
tagged.2.patch (14.1 KB) - added by jttt 11 years ago.
plugins_tagged.2.patch (3.8 KB) - added by jttt 11 years ago.
coor.patch (15.3 KB) - added by jttt 11 years ago.
getcoor.patch (43.9 KB) - added by jttt 11 years ago.

Download all attachments as: .zip

Change History (19)

Changed 11 years ago by jttt

Attachment: timestamp.patch added

Patch to replace timestamp and parsedTimestap with setter/getter

Changed 11 years ago by jttt

Attachment: tagged.patch added

Patch to change hasDirectionKeys and isTagged to read only properties

Changed 11 years ago by jttt

Attachment: plugins_tagged.patch added

Patch to replace hasDirectionKeys and isTagged with readonly properties (in plugins)

comment:1 Changed 11 years ago by jttt

If patches are applied I will prepare another patches for rest of public fields

comment:2 Changed 11 years ago by stoecker

A short review:

The plugins_tagged.patch for Validator fails. These two calls are there to ensure the two variables containing the uninteresting/direction keys are initialized. If you remove them, we need another method to ensure this.

comment:3 Changed 11 years ago by jttt

I've removed that two variable completely. isTagged/hasDirectionTags value is calculated every time that methods are called. Caching will return after keys are made private so OsmPrimitive knows that cache has to be invalidated.

comment:4 Changed 11 years ago by anonymous

You misunderstood. Validator does not use these variables, but the two string lists pointing to the keys. They have been initialized using the two functions you removed.

Changed 11 years ago by jttt

Attachment: tagged.2.patch added

Changed 11 years ago by jttt

Attachment: plugins_tagged.2.patch added

comment:5 Changed 11 years ago by jttt

I see I didn't notice that. New patch simply adds getter for uninteresting/directionKeys that make sure the lists are initialized.

comment:6 Changed 11 years ago by stoecker

They will kill me after this again, as it will break plugins. Phew.
Checkin begin of next week I would say.

comment:7 Changed 11 years ago by anonymous

Yes, plugins that are not in osm repository will not work. Maybe it should be all changed in one go so plugins will break only once. Or keep public fields for now, only make them deprecated?

comment:8 Changed 11 years ago by stoecker

Resolution: fixed
Status: newclosed

In r1499.

comment:9 Changed 11 years ago by jttt

Resolution: fixed
Status: closedreopened

Here is another patch, this time for eastNorth and coor on Node. The patch provides setCoor and setEastNorth setters and makes sure eastNorth and coor are kept in sync.

Getters should be provided as well but it's one click job in Eclipse to use getCoor in all places instead of direct access to the coor field. So I guess it would be easier to do refactoring directly instead of reviewing the patch.

Fields coor and eastNorth should be kept public and plugins should not switch to setters and getters until this gets into josm-tested.jar. This way no api breakage should occur.

When coor becames private then it might be worthy to replace LatLon instance with two doubles and create new LatLon instance in getCoor method. That would save about 20 bytes per node on 32b machine, that's 5% of JOSM memory usage.

Changed 11 years ago by jttt

Attachment: coor.patch added

comment:10 Changed 11 years ago by stoecker

Resolution: fixed
Status: reopenedclosed

In r1636.

Pleas open a new report next time. Thanks.

comment:11 Changed 11 years ago by jttt

Resolution: fixed
Status: closedreopened

Can you please refactor to getCoor/getEastNorth as well? Either using refactoring in IDE or using the getcoor.patch. I was hoping to encalupse latLon later to save some more memory.

Btw. JOSM memory usage is not that bad now. I've managed to open 830MB osm file (using 1700MB heap memory). If mappaint was a bit faster then JOSM might became good tool for mass edits.

Changed 11 years ago by jttt

Attachment: getcoor.patch added

comment:12 Changed 11 years ago by stoecker

Resolution: fixed
Status: reopenedclosed

In r1640.

I do not use eclipse, so such refactorings must be supplied as patches :-)

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.