Modify

Opened 2 years ago

Closed 2 years ago

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

Download all attachments as: .zip

Change History (30)

comment:1 Changed 2 years ago by Don-vip

Cc: michael2402 wiktorn added

Please see also #13446 #16872 #16869

comment:2 Changed 2 years 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 2 years ago by taylor.smock

Attachment: 19026.patch added

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

comment:3 Changed 2 years 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 2 years ago by taylor.smock

Attachment: 19026.1.patch added

Working patch (I haven't done extensive testing)

comment:4 in reply to:  2 ; Changed 2 years 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 2 years ago by taylor.smock

Attachment: 19026.2.patch added

comment:5 in reply to:  4 Changed 2 years 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 2 years ago by taylor.smock

Attachment: 19026.3.patch added

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

comment:6 Changed 2 years 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 2 years 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 2 years 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 2 years ago by taylor.smock

Attachment: 19026.4.patch added

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

comment:9 Changed 2 years 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 2 years 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 2 years ago by taylor.smock

Attachment: 19026.5.patch added

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

Changed 2 years ago by taylor.smock

Attachment: 19026.6.patch added

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

comment:11 Changed 2 years 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 2 years 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 2 years ago by taylor.smock

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

comment:14 Changed 2 years 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 2 years 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 2 years ago by taylor.smock

Attachment: 19026.7.patch added

Fix unit tests

comment:16 Changed 2 years ago by simon04

In 16572/josm:

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

comment:17 Changed 2 years ago by simon04

Thank you!

comment:18 Changed 2 years 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 2 years ago by simon04

In 16605/josm:

see #19026 - Add ImageryInfo.setBounds for binary compatibility

comment:21 Changed 2 years ago by stoecker

In 16650/josm:

fix #19390, see #19026 - fix ImageryCompare script

comment:22 Changed 2 years 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.