[PATCH] The imagery preferences code should be more generic

[taylor.smock]

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

[Don-vip]

Please see also #13446 #16872 #16869

[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).

[taylor.smock]

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

[taylor.smock]

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}.

[taylor.smock]

Working patch (I haven't done extensive testing)

[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.

[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. :)

[taylor.smock]

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

[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.

[taylor.smock]

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

[taylor.smock]

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

[simon04]

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

[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.

[taylor.smock]

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

[taylor.smock]

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

[simon04]

In 16545/josm:

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

[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.

[taylor.smock]

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

[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".

[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.

[taylor.smock]

Fix unit tests

[simon04]

In 16572/josm:

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

[simon04]

Thank you!

[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.

[simon04]

This caused two plugin incompatibilities:

• https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/lastCompletedBuild/jdk=JDK8/warnings3Result/package.1848369773/
• https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/lastCompletedBuild/jdk=JDK8/warnings3Result/package.-1187277412/

[simon04]

In 16605/josm:

see #19026 - Add ImageryInfo.setBounds for binary compatibility

[stoecker]

In 16650/josm:

fix #19390, see #19026 - fix ImageryCompare script

[simon04]

In 16680/josm:

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