Modify

Opened 3 months ago

Closed 4 weeks ago

Last modified 2 weeks ago

#19026 closed enhancement (fixed)

[PATCH] The imagery preferences code should be more generic

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 20.06
Component: Core Version:
Keywords: imagery, plugin, preferences Cc: michael2402, wiktorn

Description

The imagery preferences should be generic. I pretty much copied the code into the MapWithAI plugin, since I'm improving support for third-party data sources, and it would be nice for the users to see where the source is located.

I'm going to work on refactoring the code for this particular use case (so I'm not duplicating code). I'm probably going to be moving most of the code to a class by the same name, but with Source instead of Imagery, and then recreate the original Imagery classes extending the Source class. This means that any plugins which directly reference the classes shouldn't need (too much) adaption. Where Imagery/imagery is used, I'm going to add a constructor that takes that as a variable. I don't know how this is going to affect the translations (I assume that tr("Modify list of {0} layers displayed in the {1} menu", tr("imagery"), tr("Imagery") isn't going to be exactly the same as "Modify list of imagery layers displayed in the Imagery menu" -- I might just add an abstract method that gets the description).

Classes I will need to touch:

  • ImageryProvidersPanel
  • ImageryLayerTableModel
  • ImageryDefaultLayerTableModel
  • ImageryLayerInfo
  • ImageryInfo
  • ImageryBounds (I probably won't have to modify this class greatly -- I'll probably rename it to SourceBounds

A couple of enums will have to be modified to implement an interface (specifically ImageryType and ImageryCategory).

Attachments (8)

19026.patch (128.3 KB) - added by taylor.smock 3 months ago.
Initial WIP patch (very broken, its a work in progress :) )
19026.1.patch (115.5 KB) - added by taylor.smock 3 months ago.
Working patch (I haven't done extensive testing)
19026.2.patch (115.2 KB) - added by taylor.smock 3 months ago.
19026.3.patch (115.0 KB) - added by taylor.smock 4 weeks ago.
Update to JOSM 16539, keep new streams, fix some checkstyle issues.
19026.4.patch (115.9 KB) - added by taylor.smock 4 weeks ago.
Keep function calls for binary and source compatibility, but mark as deprecated
19026.5.patch (115.5 KB) - added by taylor.smock 4 weeks ago.
Remove @Deprecated (annotation), modify most @deprecated (javadoc) to @see
19026.6.patch (91.8 KB) - added by taylor.smock 4 weeks ago.
Remove most method updates (keep using the old methods in JOSM source)
19026.7.patch (555 bytes) - added by taylor.smock 4 weeks ago.
Fix unit tests

Download all attachments as: .zip

Change History (30)

comment:1 Changed 3 months ago by Don-vip

Cc: michael2402 wiktorn added

Please see also #13446 #16872 #16869

comment:2 Changed 3 months ago by taylor.smock

@Don-vip: Thanks for the links.

I don't think I'm going to muck around with preferences for this particular bug -- I'd rather refactor and ensure that the new interfaces are stable. I'm coming up on a period of time where I will be much more constrained on how I contribute to JOSM, so I don't want to have to worry about breaking people's preferences. In fact, I'm hoping I have enough time to get this done. :)

If I do modify the preferences, I'd probably change the API call for the maps so that it gets json instead of xml ( https://josm.openstreetmap.de/maps?format=geojson ), and serialize/deserialize the json to preferences, instead of trying to do that with xml. I would have to do this as a two-parter, for backwards compatibility (so two preference entries, one for current schema, one for the new schema, with code keeping both of them in sync).

Changed 3 months ago by taylor.smock

Attachment: 19026.patch added

Initial WIP patch (very broken, its a work in progress :) )

comment:3 Changed 3 months ago by taylor.smock

Summary: The imagery preferences code should be more generic[WIP PATCH] The imagery preferences code should be more generic

The current WIP patch does some additional modifications:

  • Adds a Utils.intern method to the Utils.java file (mostly so that I could use the same logic throughout the patch, without duplicating code)
  • Some methods have been renamed so that they are not specific to Imagery
  • Actually works. :)

Notes: I haven't ported the MapWithAI plugin to use this yet, but I think it is now at the point where I can do that. (That is next.)
I did edit the patch in vim (to remove a .classpath modification), so you may need to apply the patch via patch -p0 < ${PATH_TO_PATCH}.

Changed 3 months ago by taylor.smock

Attachment: 19026.1.patch added

Working patch (I haven't done extensive testing)

comment:4 in reply to:  2 ; Changed 3 months ago by stoecker

Replying to taylor.smock:

If I do modify the preferences, I'd probably change the API call for the maps so that it gets json instead of xml ( https://josm.openstreetmap.de/maps?format=geojson ), and serialize/deserialize the json to preferences, instead of trying to do that with xml. I would have to do this as a two-parter, for backwards compatibility (so two preference entries, one for current schema, one for the new schema, with code keeping both of them in sync).

That does not work. GeoJSON isn't a replacement for the XML. It has different contents.

And the XML is JOSMs primary format and it also should stay this way.

Changed 3 months ago by taylor.smock

Attachment: 19026.2.patch added

comment:5 in reply to:  4 Changed 3 months ago by taylor.smock

19026.2.patch adds a new method, which recurses up a class to get its parent @StructEntry fields.

I did spend some time modifying my code in MapWithAI to use this, which is how I found that particular bug.

Replying to stoecker:

That does not work. GeoJSON isn't a replacement for the XML. It has different contents.

And the XML is JOSMs primary format and it also should stay this way.

I guess its a good thing I wasn't intending to do anything with preferences then. :)

Changed 4 weeks ago by taylor.smock

Attachment: 19026.3.patch added

Update to JOSM 16539, keep new streams, fix some checkstyle issues.

comment:6 Changed 4 weeks ago by taylor.smock

Summary: [WIP PATCH] The imagery preferences code should be more generic[PATCH] The imagery preferences code should be more generic

comment:7 Changed 4 weeks ago by simon04

The class ImageryInfo is being used by many plugins. So we should strive for binary compatibility (or at minimum source compatibility). This implies, that setImageryType and setImageryCategoryOriginalString (and their getters) should be kept in ImageryInfo (and forward the calls to SourceInfo). Also, this makes the patch considerably smaller.

comment:8 Changed 4 weeks ago by taylor.smock

Should I mark them as deprecated, since I'm readding them but just calling the appropriate methods for binary compatibility?

Changed 4 weeks ago by taylor.smock

Attachment: 19026.4.patch added

Keep function calls for binary and source compatibility, but mark as deprecated

comment:9 Changed 4 weeks ago by simon04

Milestone: 20.06

I'd keep using the old methods in the code. Then we don't have to housekeep the deprecated methods. (I've had enough of plugin updates due to #19208).

comment:10 Changed 4 weeks ago by taylor.smock

#19208 was a pain. Some plugins updated quickly, some didn't. And then there were some followup issues.

It had to be done, if not now, then in a year or two. I suppose we could have shipped a version or two with the old jcs library co-existing with the new jcs3 library to avoid many of the pain points. But some of the pain points would still exist.

I've gone ahead and removed the @Deprecated, but I did change most of the javadocs to @see.

Changed 4 weeks ago by taylor.smock

Attachment: 19026.5.patch added

Remove @Deprecated (annotation), modify most @deprecated (javadoc) to @see

Changed 4 weeks ago by taylor.smock

Attachment: 19026.6.patch added

Remove most method updates (keep using the old methods in JOSM source)

comment:11 Changed 4 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 16545/josm:

fix #19026 - Make imagery preferences code more generic (patch by taylor.smock)

comment:12 Changed 4 weeks ago by simon04

This patch broke two tests: https://josm.openstreetmap.de/jenkins/job/JOSM/6489/testReport/

The source type used to default to WMS, but org.openstreetmap.josm.data.imagery.ImageryInfo.ImageryType#getDefault is nowhere used now.

comment:13 Changed 4 weeks ago by taylor.smock

That's odd. It didn't break any tests on my end. I'll look into it.

comment:14 Changed 4 weeks ago by taylor.smock

Its probably going to be a little while before I can fix it -- I'm trying to check and see if my patch for #16567 "missed" the failing tests, or if they were passing on Mac (I'm pretty certain that I ran tests before submitting the last patch). I'm waiting on aptitude to finish "Trying to fix broken packages".

comment:15 in reply to:  13 Changed 4 weeks ago by taylor.smock

Replying to taylor.smock:

That's odd. It didn't break any tests on my end. I'll look into it.

I was always looking at the html report. Which was never regenerated, since I was not calling ant test-html.

Changed 4 weeks ago by taylor.smock

Attachment: 19026.7.patch added

Fix unit tests

comment:16 Changed 4 weeks ago by simon04

In 16572/josm:

see #19026 - Fix default in ImageryInfo.getImageryType (patch by taylor.smock)

comment:17 Changed 4 weeks ago by simon04

Thank you!

comment:18 Changed 4 weeks ago by taylor.smock

No problem. I should have fixed it earlier, but I wanted to check and see if there was a problem with my patch for #16567. Which there is. I think it got broken around the time we were switching to Ivy (the ant test-html that I ran last was around April 20th), and ant test-html is currently failing at test-perf.

comment:20 Changed 3 weeks ago by simon04

In 16605/josm:

see #19026 - Add ImageryInfo.setBounds for binary compatibility

comment:21 Changed 2 weeks ago by stoecker

In 16650/josm:

fix #19390, see #19026 - fix ImageryCompare script

comment:22 Changed 2 weeks ago by simon04

In 16680/josm:

fix #19417, see #19026 - Add ImageryInfo.getBounds for binary compatibility

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.