[PATCH] Imagery definitions refactor

[wiktorn]

Attached patch extends imagery definitions with:

• ability to define default layers for WMS_ENDPOINT and WMTS
• ability to set custom headers from GUI
• ability to set imagery as properly georeferenced
• minimum time for which imagery tiles can be cached (workaround misconfigured tile servers which set short expiry dates, such as aerial imageries that we could keep in cache for months)

Apart from that, with this patch comes new WMS GetCapabilities parser based on SAX.

There are also easily separated fixes for CachedFile that I came across when developing this changes:

• cache validity check
• too long paths on Windows

The long vision is to move all (or all except some crippled imagery providers that do not provide GetCapabilities service) WMS imagery definitions to WMS_ENDPOINT type.

Features yet to be developed:

• add edit option in ImageryProvidersPanel which will open prefilled GUI as when adding
• add context menu in Layers window to change layer if we use WMS_ENDPOINT / WMTS
• imagery boundary generator from Nominatim / relation geomatry
• add "copy as maps entry" which would allow to easily add new entry in josm.openstreetmap.de/maps based on user imageries
• add "copy as string" which would allow easy share of entries within community

As this is bigger change that is hard to split, any testers are welcome.

For easier review - I've created pull request on my GitHub:
​https://github.com/wiktorn/josm/pull/1

You may comment there or here.

See: #15981, #7953, #16224, #15940

[stoecker]

The long vision is to move all (or all except some crippled imagery providers that do not provide GetCapabilities service) WMS imagery definitions to WMS_ENDPOINT type.

It would be very fine when these two are exchangeable, so we could do this, but I don't see that we really should do this...

ability to set custom headers from GUI
ability to set imagery as properly georeferenced

I hope these are expert settings?

add "copy as maps entry" which would allow to easily add new entry in josm.openstreetmap.de/maps based on user imageries

There also should be a more simple way to create boundaries. I still find it rather complex with the plugin. A nice solution would be to select a relation or way and say "My image boundary". It's then stripped to the geometry, checked for consistency, shown to verify and accepted. That would be fine 😊

add "copy as string" which would allow easy share of entries within community

With boundaries I think you can forget this 😏, without an URI like format would be a good idea, so it can be easily detected and parsed at different places (E.g. Drag and Drop on main window).

Question: Long code changes I did not really look at, but why do you change a regression test file?

[Don-vip]

I commented to the PR. Code is very clean, I didn't test it yet. I'd say we can give it a try once 18.04 is released. I made the remark only once but there are several public methods in different classes that must include a javadoc.

[wiktorn]

Replying to stoecker:

It would be very fine when these two are exchangeable, so we could do this, but I don't see that we really should do this...

I'm not sure that I understand what you have in mind here. We have wms and wms_endpoint for some time already. I do not plan to phase out support of wms type.

ability to set custom headers from GUI
ability to set imagery as properly georeferenced

I hope these are expert settings?

They are now. Added some commits to address this.

add "copy as maps entry" which would allow to easily add new entry in josm.openstreetmap.de/maps based on user imageries

There also should be a more simple way to create boundaries. I still find it rather complex with the plugin. A nice solution would be to select a relation or way and say "My image boundary". It's then stripped to the geometry, checked for consistency, shown to verify and accepted. That would be fine 😊

I guess we can use Nominatim to search for relation, fetch its geometry, and in the end - simplify geometry to reduce its size.

add "copy as string" which would allow easy share of entries within community

With boundaries I think you can forget this 😏, without an URI like format would be a good idea, so it can be easily detected and parsed at different places (E.g. Drag and Drop on main window).

Question: Long code changes I did not really look at, but why do you change a regression test file?

I moved regression file from one place to the other so folder structure is compatibile with MockServer. With this move I also converted line endings from DOS to UNIX.

[wiktorn]

Replying to Don-vip:

I commented to the PR. Code is very clean, I didn't test it yet.

Please do. I fear that I might have missed some corner cases.

I'd say we can give it a try once 18.04 is released.

Sounds like a plan.

I made the remark only once but there are several public methods in different classes that must include a javadoc.

I've just updated branch with commits with javadocs and other fixes.

[wiktorn]

In 13731/josm:

Fix caching long filenames and expiration

• when long name is passed to CachedFile it will be truncated and some part will be substituted with hash of the filename
• fix expiration time calculation where we were using seconds when miliseconds were expected

See: #16249

[wiktorn]

In 13733/josm:

Imagery definition refactor

Extend imagery definitions by:

• allowing setting default layers for WMS_ENDPOINT and WMTS
• allowing setting minimum expires time for tile for this imagery
• allowing setting custom headers that will be sent for all requests

(get map, get capabilities) for this imagery

Additional changes in code:

• use TileJobOptions to pass miscellaneous options to loaders
• refactor WMSImagery to use SAX parser

See: #15981, #7953, #16224, #15940, #16249

[wiktorn]

In 13734/josm:

GUI for imagery definitions refactor

Extend preferences panel by:

• allowing setting default layers for WMS / WMTS
• allowing setting minimum expires (expert mode)
• allowing setting custom headers (expert mode)

See: #15981, #7953, #16224, #15940, #16249

[Klumbumbus]

Please add end user docu at wiki:Maps#Documentation regarding all changes/additions too.

[wiktorn]

Replying to Klumbumbus:

Please add end user docu at wiki:Maps#Documentation regarding all changes/additions too.

Documentation updated

[Don-vip]

The WMTS dialog layout can be improved: more vertical space should be used by the list of layers, see screenshot:

Tested with ​https://gibs.earthdata.nasa.gov/wmts/epsg3857/best/1.0.0/WMTSCapabilities.xml

[Don-vip]

Bonus feature: a filter option would be awesome for NASA, as their WMTS server provides a very high number of layers.

[Don-vip]

Jenkins build was also failing (my fault). I have fixed my error and now we can see some tools unhappy:

• Findbugs: +11 warnings
• Checkstyle: +90 warnings
• PMD: +7 warnings
• Javadoc: +4 warnings

4 Findbugs warnings are for me, the rest is for you ;)

[wiktorn]

Javadoc: [13740]
PMD: [13741]
Checkstyle: [13742]
Findbugs: [13743]

[wiktorn]

In 13745/josm:

Layout fixes

See: #16249

[wiktorn]

In 13746/josm:

Add filter for WMTS layers, calculate preferred widths for columns.

See: #16249

[wiktorn]

In 13747/josm:

Report parse exceptions as WMTSGetCapabilitiesException

See: #16249

[wiktorn]

In 13749/josm:

Remove unused function & better error reporting

See: #16249

[wiktorn]

In 13751/josm:

More precise add imagery verification if all data is provided

See: #16249

[Don-vip]

Thanks! One final fix for the bottom part and the WMTS dialog will be OK :)

[Don-vip]

One bug found:

1. Filter with "landsat" as shown above
2. in the filter field, put the cursor at the beginning, before "landsat"
3. Try to enter "global" -> NPE

Build-Date:2018-05-13 14:04:25
Revision:13754
Is-Local-Build:true

Identification: JOSM/1.5 (13754 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1803 (17134)
Memory Usage: 1272 MB / 3634 MB (196 MB allocated, but free)
Java version: 1.8.0_172-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080, \Display1 1920x1080, \Display2 1280x1024
Maximum Screen Size: 1920x1080
VM arguments: [-Dfile.encoding=UTF-8]

Last errors/warnings:
- E: Handled by bug report queue: java.lang.NullPointerException

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (19) of main
java.lang.NullPointerException
	at org.openstreetmap.josm.gui.preferences.imagery.AddWMTSLayerPanel.lambda$2(AddWMTSLayerPanel.java:72)
	at javax.swing.DefaultListSelectionModel.fireValueChanged(DefaultListSelectionModel.java:184)
	at javax.swing.DefaultListSelectionModel.fireValueChanged(DefaultListSelectionModel.java:164)
	at javax.swing.DefaultListSelectionModel.fireValueChanged(DefaultListSelectionModel.java:211)
	at javax.swing.DefaultListSelectionModel.changeSelection(DefaultListSelectionModel.java:405)
	at javax.swing.DefaultListSelectionModel.changeSelection(DefaultListSelectionModel.java:415)
	at javax.swing.DefaultListSelectionModel.removeSelectionIntervalImpl(DefaultListSelectionModel.java:576)
	at javax.swing.DefaultListSelectionModel.clearSelection(DefaultListSelectionModel.java:420)
	at javax.swing.JTable$SortManager.restoreSelection(JTable.java:4035)
	at javax.swing.JTable$SortManager.processChange(JTable.java:4003)
	at javax.swing.JTable.sortedTableChanged(JTable.java:4135)
	at javax.swing.JTable.sorterChanged(JTable.java:3825)
	at javax.swing.RowSorter.fireRowSorterChanged(RowSorter.java:341)
	at javax.swing.RowSorter.fireRowSorterChanged(RowSorter.java:332)
	at javax.swing.DefaultRowSorter.sort(DefaultRowSorter.java:612)
	at javax.swing.DefaultRowSorter.setRowFilter(DefaultRowSorter.java:424)
	at org.openstreetmap.josm.gui.layer.imagery.WMTSLayerSelection$1.update(WMTSLayerSelection.java:127)
	at org.openstreetmap.josm.gui.layer.imagery.WMTSLayerSelection$1.insertUpdate(WMTSLayerSelection.java:112)
	at javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:201)
	at javax.swing.text.AbstractDocument.handleInsertString(AbstractDocument.java:748)
	at javax.swing.text.AbstractDocument.insertString(AbstractDocument.java:707)
	at javax.swing.text.PlainDocument.insertString(PlainDocument.java:130)
	at javax.swing.text.AbstractDocument.replace(AbstractDocument.java:669)
	at javax.swing.text.JTextComponent.replaceSelection(JTextComponent.java:1328)
	at javax.swing.text.DefaultEditorKit$DefaultKeyTypedAction.actionPerformed(DefaultEditorKit.java:884)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1668)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2882)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2929)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2845)
	at java.awt.Component.processEvent(Component.java:6316)
	at java.awt.Container.processEvent(Container.java:2239)
	at java.awt.Component.dispatchEventImpl(Component.java:4889)
	at java.awt.Container.dispatchEventImpl(Container.java:2297)
	at java.awt.Component.dispatchEvent(Component.java:4711)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1954)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:835)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1103)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:974)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:800)
...
	at org.openstreetmap.josm.gui.preferences.imagery.ImageryPreference$ImageryProvidersPanel$NewEntryAction.actionPerformed(ImageryPreference.java:535)
...
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

[wiktorn]

In 13755/josm:

Fix NPE when no layer selected

See: #16249

[wiktorn]

In 13756/josm:

Fix layout for "is properly georeferenced"

See: #16249

[wiktorn]

In 13757/josm:

Change camelCase to hyphen-case for imagery attributes

See: #16249, #15940

[Don-vip]

Thanks. The "Get layers" string should reuse the same string as other dialogs (with a number prefix). Maybe the numbering could dynamically change when we enable the "set default layer?" checkbox.

[wiktorn]

Replying to Don-vip:

Thanks. The "Get layers" string should reuse the same string as other dialogs (with a number prefix). Maybe the numbering could dynamically change when we enable the "set default layer?" checkbox.

I've just implemented it. But it looks a bit strange, if there are two "3." on screen and one is inactive. So I guess that dynamic numbering is not necessary.

Though I see another case - expert mode vs non-expert mode. Maybe we should use dynamic numbering based on whether we ask about headers, minimum expiry time and valid georeference?

[Klumbumbus]

The header table is too flat in the WMS window, see attachment:16249.png​ (while in the WMTS window it is fine).

[Klumbumbus]

Also this window has a base size which is larger than my 1680*1050 screen (deducting the height of the task bar). I think the box with the layers could be flatter by default. (see same screenshot). When I resize the WMS window, close it and reopen it is back at its large size.

[anonymous]

Replying to Klumbumbus:

The header table is too flat in the WMS window, see attachment:16249.png​ (while in the WMTS window it is fine).

When you write too flat you mean too small?

[Klumbumbus]

yes

[stoecker]

When I resize the WMS window, close it and reopen it is back at its large size.

Which is an error in itself. We usually remember geometries.

[wiktorn]

In 13772/josm:

Fix Add WMS Panel layout

See: #16249

[wiktorn]

Replying to stoecker:

When I resize the WMS window, close it and reopen it is back at its large size.

Which is an error in itself. We usually remember geometries.

Can you give a hint how this is achieved?

[wiktorn]

In 13773/josm:

Add numbering to get layers button on Add WMTS Layer

See: #16249

[stoecker]

Long time since I did this. Have a look at WindowGeometry.java. I shutdown my laptop already, but a short look indicated this may be the right place.

[wiktorn]

In 13774/josm:

Remember Add Imagery dialog position

See: #16249

[wiktorn]

Replying to wiktorn:

In 13774/josm:

Remember Add Imagery dialog position

See: #16249

After giving it some more thought, I guess that I did it wrong. I guess that I should just use

ExtendedDialog.setRememberWindowGeometry(
  saveGeometryEntryName, 
  WindowGeometry.centerInWindow(Main.parent, new Dimension(800, 600))
);

But couldn't we just use getClass().getName() + ".geometry" as a default value for preference entry?

And why is default geometry at all needed? Can't we rely on AWT do draw it properly when preferences are broken? Then we could make it invisible to all clients of ExtendedDialog.

[stoecker]

Replying to wiktorn:

But couldn't we just use getClass().getName() + ".geometry" as a default value for preference entry?

I'd say, because class name and geometry name must not be an 1:1 assignment (a class may have either multiple windows or same window with multiple different purposes). Or because passing the class pointer to get the name or passing the name is no real big difference. Or both.

And why is default geometry at all needed? Can't we rely on AWT do draw it properly when preferences are broken? Then we could make it invisible to all clients of ExtendedDialog.

Either because of historic reasons or because we wanted it this way. You could extend it that a "null" parameter means auto-default if you want and it is possible. 😊

[wiktorn]

In 13777/josm:

Use setRememberWindowGeometry instead overriding setVisible

See: #16249

[wiktorn]

In 13778/josm:

Prefer Cache-Control header over Expires header

According to RFC2616 Cache-Control takes precedence over Expires header.

See: #16249

[Don-vip]

@wiktorn: this is strange. Did you meant == null?

[wiktorn]

Indeed, it looks like I meant == null, as I skimmed through Sonar I have a few things to look on. I'll try fixing them during the weekend.

[Don-vip]

OK :) I plan to release the new version Sunday evening if i18n is OK. Do you have other things in mind for targeted imagery tickets (#7953, #15981, #15988, #16249)?

[wiktorn]

In 13828/josm:

Sonar fixes, javadocs.

See: #16249

[wiktorn]

Replying to Don-vip:

OK :) I plan to release the new version Sunday evening if i18n is OK. Do you have other things in mind for targeted imagery tickets (#7953, #15981, #15988, #16249)?

OK, I'll keep that in mind. I do not have anything else in mind for this release.

[Don-vip]

So can we close them as fixed?

[anonymous]

This is so nice. Thank you so much.

[nkamapper]

Nice addition, however I am getting the following error message when trying to enter a wmts default layer in Maps/Norway:

XML validation for map failed: Element '{http://josm.openstreetmap.de/maps-1.0}layer': Character content is not allowed, because the content type is empty.

Here is the entry i tried to add for NPI Jan Mayen:

        <default-layers>
            <layer>
                <name>Basisdata_NP_Basiskart_JanMayen_WMTS_25829</name>
                <style>default</style>
                <tile-matrix-set>default028mm</tile-matrix-set>
            </layer>
        </default-layers>

There is also an error message when trying to add the new category attribute.

[anonymous]

Replying to nkamapper:

Nice addition, however I am getting the following error message when trying to enter a wmts default layer in Maps/Norway:

XML validation for map failed: Element '{http://josm.openstreetmap.de/maps-1.0}layer': Character content is not allowed, because the content type is empty.

Here is the entry i tried to add for NPI Jan Mayen:

        <default-layers>
            <layer>
                <name>Basisdata_NP_Basiskart_JanMayen_WMTS_25829</name>
                <style>default</style>
                <tile-matrix-set>default028mm</tile-matrix-set>
            </layer>
        </default-layers>

It should be as follows:

        <default-layers>
            <layer name="Basisdata_NP_Basiskart_JanMayen_WMTS_25829" style="default" tile-matrix-set="default028mm" />
        </default-layers>

[nkamapper]

Got it, and added in Maps/Norway.

But should we not be able to get directly to the default layer now without selecting it again in the "Select wmts layer" window in JOSM?

[wiktorn]

Replying to nkamapper:

Got it, and added in Maps/Norway.

But should we not be able to get directly to the default layer now without selecting it again in the "Select wmts layer" window in JOSM?

I've just checked. Downloaded area around Jan Mayen, went to Imagery/NPI Jan Mayen TOPO and without any further prompts - I got the imagery added.

I tried changing my projection to any supported by the server (25829, 84) and in all three cases - there was no prompt.

Are you sure you're using the entry fetched from JOSM list and not your own?

[nkamapper]

Yes, I pan to Jan Mayen area and select Imagery->"NPI Jan Mayen topo" (at the end of that menu, among the imagery applicable for that area), but get the prompt.
I have no local entry for this imagery. Even restarted JOSM a couple of times.

[stoecker]

Replying to nkamapper:

Yes, I pan to Jan Mayen area and select Imagery->"NPI Jan Mayen topo" (at the end of that menu, among the imagery applicable for that area), but get the prompt.
I have no local entry for this imagery. Even restarted JOSM a couple of times.

Clear the cache (press reload in imagery settings)?

[nkamapper]

Clear the cache (press reload in imagery settings)?

Yes, the "Update default entries" button - did not help.

I am not getting any updates from Maps, it seems, e.g. the recently added "Norway Orthophoto" today...
JOSM 13860.

[wiktorn]

Replying to nkamapper:

Clear the cache (press reload in imagery settings)?

Yes, the "Update default entries" button - did not help.

I am not getting any updates from Maps, it seems, e.g. the recently added "Norway Orthophoto" today...
JOSM 13860.

Indeed, I tried right now updating other layer and had the same problem. I had to manually remove cached imagery list (on Linux):

$ rm ~/.cache/JOSM/mirror_https___josm.openstreetmap.de_maps

And afterwards new definitions were properly downloaded and it started to work as intended. Looks like we're caching imagery definitions too aggressively now.

[wiktorn]

@stoecker - can you explain parameter in CachedFile? The problem is, that we do:

CachedFile.cleanup("https://josm.openstreetmap.de/maps%<?ids=>");

in ImageryLayerInfo but we will store the entry under the key: https://josm.openstreetmap.de/maps%<?ids=>. Wouldn't it better to pass clearCache through ImageryReader up to CachedFile?