Opened 14 years ago
Closed 14 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)
Change History (19)
Changed 14 years ago by
Attachment: | timestamp.patch added |
---|
Changed 14 years ago by
Attachment: | tagged.patch added |
---|
Patch to change hasDirectionKeys and isTagged to read only properties
Changed 14 years ago by
Attachment: | plugins_tagged.patch added |
---|
Patch to replace hasDirectionKeys and isTagged with readonly properties (in plugins)
comment:1 Changed 14 years ago by
If patches are applied I will prepare another patches for rest of public fields
comment:2 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
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 14 years ago by
Attachment: | tagged.2.patch added |
---|
Changed 14 years ago by
Attachment: | plugins_tagged.2.patch added |
---|
comment:5 Changed 14 years ago by
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 14 years ago by
They will kill me after this again, as it will break plugins. Phew.
Checkin begin of next week I would say.
comment:7 Changed 14 years ago by
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:9 Changed 14 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 14 years ago by
Attachment: | coor.patch added |
---|
comment:10 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
In r1636.
Pleas open a new report next time. Thanks.
comment:11 Changed 14 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 14 years ago by
Attachment: | getcoor.patch added |
---|
comment:12 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
In r1640.
I do not use eclipse, so such refactorings must be supplied as patches :-)
Patch to replace timestamp and parsedTimestap with setter/getter