Modify

Opened 3 months ago

Closed 8 weeks ago

Last modified 7 weeks ago

#16249 closed enhancement (fixed)

[PATCH] Imagery definitions refactor

Reported by: wiktorn Owned by: team
Priority: major Milestone: 18.05
Component: Core imagery Version:
Keywords: wms wmts endpoint Cc: Don-vip, Klumbumbus

Description (last modified by 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

Attachments (3)

16249_ui.png (21.2 KB) - added by Don-vip 2 months ago.
16249_ui2.png (43.1 KB) - added by Don-vip 2 months ago.
16249.png (20.8 KB) - added by Klumbumbus 2 months ago.

Download all attachments as: .zip

Change History (64)

comment:1 Changed 3 months ago by wiktorn

Description: modified (diff)

comment:2 Changed 3 months ago by 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?

comment:3 Changed 3 months ago by Don-vip

Keywords: wms wmts endpoint added

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.

comment:4 Changed 3 months ago by Don-vip

Priority: normalmajor

comment:5 in reply to:  2 Changed 3 months ago by 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.

comment:6 Changed 3 months ago by wiktorn

Description: modified (diff)

comment:7 in reply to:  3 Changed 3 months ago by 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.

comment:8 Changed 3 months ago by wiktorn

Description: modified (diff)

comment:9 Changed 3 months ago by wiktorn

Description: modified (diff)

comment:10 Changed 2 months ago by 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

comment:11 Changed 2 months ago by 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

comment:12 Changed 2 months ago by 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

comment:13 Changed 2 months ago by Klumbumbus

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

comment:14 in reply to:  13 Changed 2 months ago by wiktorn

Replying to Klumbumbus:

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

Documentation updated

Changed 2 months ago by Don-vip

Attachment: 16249_ui.png added

comment:15 Changed 2 months ago by 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

comment:16 Changed 2 months ago by Don-vip

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

comment:17 Changed 2 months ago by Don-vip

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

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

comment:18 Changed 2 months ago by wiktorn

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

comment:19 Changed 2 months ago by wiktorn

In 13745/josm:

Layout fixes

See: #16249

comment:20 Changed 2 months ago by wiktorn

In 13746/josm:

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

See: #16249

comment:21 Changed 2 months ago by wiktorn

In 13747/josm:

Report parse exceptions as WMTSGetCapabilitiesException

See: #16249

comment:22 Changed 2 months ago by wiktorn

In 13749/josm:

Remove unused function & better error reporting

See: #16249

comment:23 Changed 2 months ago by wiktorn

In 13751/josm:

More precise add imagery verification if all data is provided

See: #16249

Changed 2 months ago by Don-vip

Attachment: 16249_ui2.png added

comment:24 Changed 2 months ago by Don-vip

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


comment:25 Changed 2 months ago by 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)

comment:26 Changed 2 months ago by wiktorn

In 13755/josm:

Fix NPE when no layer selected

See: #16249

comment:27 Changed 2 months ago by wiktorn

In 13756/josm:

Fix layout for "is properly georeferenced"

See: #16249

comment:28 Changed 2 months ago by wiktorn

In 13757/josm:

Change camelCase to hyphen-case for imagery attributes

See: #16249, #15940

comment:29 Changed 2 months ago by 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.

comment:30 in reply to:  29 Changed 2 months ago by 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?

Changed 2 months ago by Klumbumbus

Attachment: 16249.png added

comment:31 Changed 2 months ago by Klumbumbus

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

comment:32 Changed 2 months ago by 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.

comment:33 in reply to:  31 Changed 2 months ago by 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?

comment:34 Changed 2 months ago by Klumbumbus

yes

comment:35 Changed 2 months ago by 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.

comment:36 Changed 2 months ago by wiktorn

In 13772/josm:

Fix Add WMS Panel layout

See: #16249

comment:37 in reply to:  35 Changed 2 months ago by 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?

comment:38 Changed 2 months ago by wiktorn

In 13773/josm:

Add numbering to get layers button on Add WMTS Layer

See: #16249

comment:39 Changed 2 months ago by 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.

comment:40 Changed 2 months ago by wiktorn

In 13774/josm:

Remember Add Imagery dialog position

See: #16249

comment:41 in reply to:  40 ; Changed 2 months ago by 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.

comment:42 in reply to:  41 Changed 2 months ago by 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. 😊

comment:43 Changed 2 months ago by wiktorn

In 13777/josm:

Use setRememberWindowGeometry instead overriding setVisible

See: #16249

comment:44 Changed 2 months ago by wiktorn

In 13778/josm:

Prefer Cache-Control header over Expires header

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

See: #16249

comment:45 Changed 8 weeks ago by Don-vip

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

Last edited 8 weeks ago by Don-vip (previous) (diff)

comment:46 Changed 8 weeks ago by 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.

comment:47 Changed 8 weeks ago by 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)?

comment:48 Changed 8 weeks ago by wiktorn

In 13828/josm:

Sonar fixes, javadocs.

See: #16249

comment:49 in reply to:  47 Changed 8 weeks ago by 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.

comment:50 Changed 8 weeks ago by Don-vip

So can we close them as fixed?

comment:51 Changed 8 weeks ago by wiktorn

Resolution: fixed
Status: newclosed

comment:52 Changed 8 weeks ago by anonymous

This is so nice. Thank you so much.

comment:53 Changed 8 weeks ago by 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.

comment:54 in reply to:  53 Changed 7 weeks ago by 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>

comment:55 Changed 7 weeks ago by 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?

comment:56 in reply to:  55 Changed 7 weeks ago by 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?

comment:57 Changed 7 weeks ago by 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.

comment:58 in reply to:  57 Changed 7 weeks ago by 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)?

comment:59 Changed 7 weeks ago by 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.

comment:60 in reply to:  59 Changed 7 weeks ago by 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.

comment:61 Changed 7 weeks ago by 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?

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.