Unexpected exception after cancelling WMTS imagery selection

[Gilbert54]

What steps will reproduce the problem?

1. open the map
2. Add imagery from wms:http://geoservices.informatievlaanderen.be/raadpleegdiensten/GRB-selectie/wms?FORMAT=image/jpeg&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=GRB_SEL&STYLES=&SRS={proj}&WIDTH={width}&HEIGHT={height}&BBOX={bbox}
3. Open menu for imagery from wmts:http://tile.informatievlaanderen.be/ws/raadpleegdiensten/wmts?request=getcapabilities&service=wmts&version=1.0.0
4. Cancel this menu selection

What is the expected result?

Cancel command is accepted and you're back where you were after step 2

What happens instead?

Unexpected exception pops up. After acknowledgement program continues to work correctly.
The same happens if you close the selection window i.s.o. clicking the "Cancel" button

Please provide any additional information below. Attach a screenshot if possible.

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2016-09-06 00:16:07 +0200 (Tue, 06 Sep 2016)
Build-Date:2016-09-05 22:21:00
Revision:10966
Relative:URL: ^/trunk

Identification: JOSM/1.5 (10966 en_GB) Windows 10 64-Bit
Memory Usage: 430 MB / 910 MB (210 MB allocated, but free)
Java version: 1.8.0_111-b14, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1200, \Display1 1280x1024
Maximum Screen Size: 1920x1200
Dataset consistency test: No problems found

Plugins:
+ turnrestrictions (32796)


=== REPORTED CRASH DATA ===
MapView#layerAdded:
 - layer: WMTSLayer [info=ImageryInfo{name='AGIV : WTMS', countryCode='', url='http://tile.informatievlaanderen.be/ws/raadpleegdiensten/wmts?request=getcapabilities&service=wmts&version=1.0.0', imageryType=WMTS}]

LayerManager#fireLayerAdded:
 - listener: org.openstreetmap.josm.gui.MapView[,0,76,1036x891,alignmentX=0.0,alignmentY=0.0,border=,flags=0,maximumSize=,minimumSize=java.awt.Dimension[width=10,height=0],preferredSize=]
 - event: LayerAddEvent [addedLayer=WMTSLayer [info=ImageryInfo{name='AGIV : WTMS', countryCode='', url='http://tile.informatievlaanderen.be/ws/raadpleegdiensten/wmts?request=getcapabilities&service=wmts&version=1.0.0', imageryType=WMTS}]]

BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (20) of main
java.lang.IllegalArgumentException: No layer selected
	at org.openstreetmap.josm.data.imagery.WMTSTileSource.userSelectLayer(WMTSTileSource.java:270)
	at org.openstreetmap.josm.data.imagery.WMTSTileSource.initProjection(WMTSTileSource.java:666)
	at org.openstreetmap.josm.gui.layer.WMTSLayer.projectionChanged(WMTSLayer.java:110)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.initializeIfRequired(AbstractTileSourceLayer.java:702)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.attachToMapView(AbstractTileSourceLayer.java:678)
	at org.openstreetmap.josm.gui.MapView.layerAdded(MapView.java:652)
	at org.openstreetmap.josm.gui.layer.LayerManager.fireLayerAdded(LayerManager.java:379)
	at org.openstreetmap.josm.gui.layer.LayerManager.realAddLayer(LayerManager.java:191)
	at org.openstreetmap.josm.gui.layer.MainLayerManager.realAddLayer(MainLayerManager.java:267)
	at org.openstreetmap.josm.gui.layer.LayerManager.lambda$addLayer$0(LayerManager.java:180)
	at org.openstreetmap.josm.gui.util.GuiHelper.runInEDTAndWaitWithException(GuiHelper.java:138)
	at org.openstreetmap.josm.gui.layer.LayerManager.addLayer(LayerManager.java:180)
	at org.openstreetmap.josm.actions.AddImageryLayerAction.actionPerformed(AddImageryLayerAction.java:72)
	at javax.swing.AbstractButton.fireActionPerformed(Unknown Source)
	at javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source)
	at javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source)
	at javax.swing.DefaultButtonModel.setPressed(Unknown Source)
	at javax.swing.AbstractButton.doClick(Unknown Source)
	at javax.swing.plaf.basic.BasicMenuItemUI.doClick(Unknown Source)
	at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(Unknown Source)
	at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
	at java.awt.Component.processMouseEvent(Unknown Source)
	at javax.swing.JComponent.processMouseEvent(Unknown Source)
	at java.awt.Component.processEvent(Unknown Source)
	at java.awt.Container.processEvent(Unknown Source)
	at java.awt.Component.dispatchEventImpl(Unknown Source)
	at java.awt.Container.dispatchEventImpl(Unknown Source)
	at java.awt.Component.dispatchEvent(Unknown Source)
	at java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
	at java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
	at java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
	at java.awt.Container.dispatchEventImpl(Unknown Source)
	at java.awt.Window.dispatchEventImpl(Unknown Source)
	at java.awt.Component.dispatchEvent(Unknown Source)
	at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
	at java.awt.EventQueue.access$500(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue$4.run(Unknown Source)
	at java.awt.EventQueue$4.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue.dispatchEvent(Unknown Source)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.run(Unknown Source)

[Gilbert54]

Status report

[wiktorn]

IMHO this is the same problem that was reported in #13828

We still might need a better approach.

[simon04]

The stacktrace back at r9329

     [java] java.lang.IllegalArgumentException: No layer selected
     [java] 	at org.openstreetmap.josm.data.imagery.WMTSTileSource.userSelectLayer(WMTSTileSource.java:251)
     [java] 	at org.openstreetmap.josm.data.imagery.WMTSTileSource.initProjection(WMTSTileSource.java:607)
     [java] 	at org.openstreetmap.josm.gui.layer.WMTSLayer.projectionChanged(WMTSLayer.java:114)
     [java] 	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.hookUpMapView(AbstractTileSourceLayer.java:515)
     [java] 	at org.openstreetmap.josm.Main.addLayer(Main.java:768)
     [java] 	at org.openstreetmap.josm.Main.addLayer(Main.java:751)
     [java] 	at org.openstreetmap.josm.Main.addLayer(Main.java:738)
     [java] 	at org.openstreetmap.josm.actions.AddImageryLayerAction.actionPerformed(AddImageryLayerAction.java:84)
     [java] 	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2022)
     [java] 	at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2348)
     [java] 	at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
     [java] 	at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
     [java] 	at javax.swing.AbstractButton.doClick(AbstractButton.java:376)
     [java] 	at javax.swing.plaf.basic.BasicMenuItemUI.doClick(BasicMenuItemUI.java:833)
     [java] 	at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(BasicMenuItemUI.java:877)
     [java] 	at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
     [java] 	at java.awt.Component.processMouseEvent(Component.java:6533)
     [java] 	at javax.swing.JComponent.processMouseEvent(JComponent.java:3324)

The stacktrace at r11209

2016-11-02 10:50:35.241 SEVERE: Handled by bug report queue: java.lang.IllegalArgumentException: No layer selected
java.lang.IllegalArgumentException: No layer selected
	at org.openstreetmap.josm.data.imagery.WMTSTileSource.userSelectLayer(WMTSTileSource.java:262)
	at org.openstreetmap.josm.data.imagery.WMTSTileSource.initProjection(WMTSTileSource.java:540)
	at org.openstreetmap.josm.gui.layer.WMTSLayer.projectionChanged(WMTSLayer.java:110)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.initializeIfRequired(AbstractTileSourceLayer.java:690)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.attachToMapView(AbstractTileSourceLayer.java:667)
	at org.openstreetmap.josm.gui.MapView.layerAdded(MapView.java:325)
	at org.openstreetmap.josm.gui.layer.LayerManager.fireLayerAdded(LayerManager.java:405)
	at org.openstreetmap.josm.gui.layer.LayerManager.realAddLayer(LayerManager.java:200)
	at org.openstreetmap.josm.gui.layer.MainLayerManager.realAddLayer(MainLayerManager.java:251)
	at org.openstreetmap.josm.gui.layer.LayerManager.lambda$addLayer$0(LayerManager.java:189)
	at org.openstreetmap.josm.gui.util.GuiHelper.runInEDTAndWaitWithException(GuiHelper.java:139)
	at org.openstreetmap.josm.gui.layer.LayerManager.addLayer(LayerManager.java:189)
	at org.openstreetmap.josm.actions.AddImageryLayerAction.actionPerformed(AddImageryLayerAction.java:77)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2022)
	at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2348)
	at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
	at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
	at javax.swing.AbstractButton.doClick(AbstractButton.java:376)
	at javax.swing.plaf.basic.BasicMenuItemUI.doClick(BasicMenuItemUI.java:833)
	at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(BasicMenuItemUI.java:877)
	at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
	at java.awt.Component.processMouseEvent(Component.java:6533)
	at javax.swing.JComponent.processMouseEvent(JComponent.java:3324)

[simon04]

What about unwrapping ReportedException?

src/org/openstreetmap/josm/actions/AddImageryLayerAction.java

@@ -32 +32 @@
 import org.openstreetmap.josm.io.imagery.WMSImagery.WMSGetCapabilitiesException;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.ImageProvider;
+import org.openstreetmap.josm.tools.bugreport.ReportedException;
 
 /**
  * Action displayed in imagery menu to add a new imagery layer.
@@ -69 +70 @@
         try {
             final ImageryInfo infoToAdd = ImageryType.WMS_ENDPOINT.equals(info.getImageryType())
                     ? getWMSLayerInfo() : info;
-            if (infoToAdd != null) {
-                Main.getLayerManager().addLayer(ImageryLayer.create(infoToAdd));
+            if (infoToAdd == null) {
+                return;
+            }
+            ImageryLayer layer = null;
+            try {
+                layer = ImageryLayer.create(infoToAdd);
+                Main.getLayerManager().addLayer(layer);
                 AlignImageryPanel.addNagPanelIfNeeded(infoToAdd);
+            } catch (ReportedException ex) {
+                if (layer != null) {
+                    try {
+                        Main.getLayerManager().removeLayer(layer);
+                    } catch (Exception ignore) {
+                        Main.debug(ignore);
+                    }
+                }
+                // unwrap IllegalArgumentException for error reporting below, see #13868
+                throw ex.getCause() instanceof IllegalArgumentException ? ((IllegalArgumentException) ex.getCause()) : ex;
             }
         } catch (IllegalArgumentException ex) {
             if (ex.getMessage() == null || ex.getMessage().isEmpty() || GraphicsEnvironment.isHeadless()) {

[michael2402]

I am no big fan of this design.

1. We remove the layer during hookUpMapView. Removing the layer let's the remove listeners fire. This is one of those cases that may lead to unexpected behavior since listeners are informed or: layerAdded, layerRemoved, attachToMapView.
2. We check the layer during attachToMapView. When supporting multiple map views in the future this will lead to issues.
3. We should not rely on that exception. We should not use IllegalArgumentException and subclass our own runtime exception.

I'd suggest to work on this todo:

// TODO: save layer information into ImageryInfo / ImageryPreferences?

Can't we simply select the layer before we even create the WMTSTileSource/WMTSLayer? We could move the checks in getTileSource to the WMTSLayer constructor as well, so that problems with the info object are reported early.

[simon04]

Replying to michael2402:

I am no big fan of this design.

I agree. I'm just trying to get this issue fixed for a release.

I'd suggest to work on this todo:

// TODO: save layer information into ImageryInfo / ImageryPreferences?

Not sure. This would no longer allow to add a WMTS capabilities entry in the imagery preferences and select a different layer on every use of this entry. Maybe wiktorn can also comment on this.

Can't we simply select the layer before we even create the WMTSTileSource/WMTSLayer? We could move the checks in getTileSource to the WMTSLayer constructor as well, so that problems with the info object are reported early.

wiktorn attempted initialising the tile source r8860, but reverted in r8862.

[wiktorn]

Replying to simon04:

Replying to michael2402:

I am no big fan of this design.

I agree. I'm just trying to get this issue fixed for a release.

I'd suggest to work on this todo:

// TODO: save layer information into ImageryInfo / ImageryPreferences?

Not sure. This would no longer allow to add a WMTS capabilities entry in the imagery preferences and select a different layer on every use of this entry. Maybe wiktorn can also comment on this.

This todo is meant to store the chosen layer in preferences, but anyhow - form time to time we would show this dialog during creation of the layer

Can't we simply select the layer before we even create the WMTSTileSource/WMTSLayer? We could move the checks in getTileSource to the WMTSLayer constructor as well, so that problems with the info object are reported early.

wiktorn attempted initialising the tile source r8860, but reverted in r8862.

Yes, and AFAIR it was due to SessionReader/SessionWriter incompatibilities. But I can't find right now any clues, why I did such a change.

I've checked also wms_endpoint and maybe this is a way forward for current release - move layer choice outside Layer/TileSource constructors - in AddImageryLayerAction.actionPerformed(...)?

[michael2402]

Replying to wiktorn:

I've checked also wms_endpoint and maybe this is a way forward for current release - move layer choice outside Layer/TileSource constructors - in AddImageryLayerAction.actionPerformed(...)?

Yes, this is where I'd expect it. We may be able to do a better/safer error handling there.

[wiktorn]

So I'm starting work on the patch

[simon04]

Do you plan finishing the patch soon, i.e., ready for the 16.10 release? Since this bug is around for a few months and typically not encountered in the daily use, IMO, this is no blocker for a release.

[wiktorn]

I plan to publish a patch tonight for review, as we're tight on schedule, so any additional pair of eyes is most welcome.

[wiktorn]

This is my take on this.

Changes in AddImageryLayerAction.getWMSLayerInfo() are mainly due to movement of exception handling to convertImagery()

Any comments welcome :-)

[michael2402]

+1. Nice ;-)

[simon04]

Please do not change the i18n strings in org.openstreetmap.josm.data.imagery.WMTSTileSource.SelectLayerDialog#SelectLayerDialog/getColumnName although it is slightly incorrect. Otherwise all translations are incomplete.

Adding/cancelling WMS_ENDPOINT and WMTS layers via the user dialog seems to be working nicely.

[wiktorn]

In 11216/josm:

Move WMTS layer selection out of WMTSLayer constructor and hookUpMapView.

When user declines to choose any WMTSLayer we should just not add the layer at all instead of showing a warning & empty layer content.

This change alignes behaviour of layer between WMTS and WMS_ENDPOINT imagery sources.

This change includes some parts of Work In Progress to unify WMS and WMS_ENDPOINT sources.

Closes: #13868

[simon04]

Ticket #13974 has been marked as a duplicate of this ticket.