[Patch] Some of the key listeners forgot to remove themselves

[rickmastfan67]

This is similar to #10233 from 2014.

Tested this with my normal and a fresh profile and was able to duplicate it in both.

Steps to reproduce:
1) Once JOSM is started, download a small area of data (doesn't matter how big).
2) Close JOSM.

What happens:
The following errors show up after JOSM closes the RemoteControl connections in the command line.

I manually typed out the errors due to only being able to snag them via a screenshot (which I can add later if requested).

WARNING: Some of the key listeners forgot to remove themselves: [org.openstreetmap.josm.actions.mapmode.SelectAction@606899f0]
WARNING: Some of the key modifier listeners forgot to remove themselves: org.openstreetmap.josm.tools.ListenerList@57b834ee

Errors only show up if you download data. If you start and then close JOSM right away, the issues don't show up.

Also, did some testing to find the regression window. This was introduced between r17977 (no issue) & r17992 (issue shows up).

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-07-13 00:27:59 +0200 (Tue, 13 Jul 2021)
Build-Date:2021-07-13 01:31:01
Revision:18009
Relative:URL: ^/trunk

Identification: JOSM/1.5 (18009 en) Windows 7 64-Bit
OS Build number: Windows 7 Professional (7601)
Memory Usage: 498 MB / 1820 MB (262 MB allocated, but free)
Java version: 1.8.0_201-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: en_US
Numbers with default locale: 1234567890 -> 1234567890

Plugins:
+ OpeningHoursEditor (35640)
+ buildings_tools (35756)
+ measurement (35640)
+ reverter (35732)
+ tageditor (35640)
+ turnlanes-tagging (288)
+ turnrestrictions (35640)
+ undelete (35640)
+ utilsplugin2 (35691)

[skyper]

Yes, can reproduce. Similar to #13095. I think r17991 was the problematical change.

1. Start JOSM
2. Download some data
3. Exit JOSM

2021-07-13 12:52:37.290 WARNING: Some of the key listeners forgot to remove themselves: [org.openstreetmap.josm.actions.mapmode.SelectAction@2e42d101]
2021-07-13 12:52:37.291 WARNING: Some of the key modifier listeners forgot to remove themselves: org.openstreetmap.josm.tools.ListenerList@786f1077

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-07-13 00:27:59 +0200 (Tue, 13 Jul 2021)
Revision:18009
Build-Date:2021-07-13 01:31:01
URL:https://josm.openstreetmap.de/svn/trunk

[Bjoeni]

It does not happen if my patch from #20989 is fully applied (preventing the layerAvailabilityListeners from being fired).

Replying to Don-vip:

I've only applied the first part, I think we still need to fire layer availability events even when the worker is shut down.

I did not really see a reason to fire the listeners at that point because when shutting down it doesn't matter if you destroy the MapFrame and display the welcome message instead or not. That's only relevant when destroying the MapFrame (removing the last layer) while JOSM is still up and running.

@Vincent what do you think?

[Don-vip]

I'm not sure. We should not assume what the listeners are doing, that's why I kept this part untouched. Maybe some of them can/could/will perform some tasks needed even when JOSM closes down, like saving some data, etc.

[Bjoeni]

Yes, you're right. Since it's used only once though why not stop it there:

src/org/openstreetmap/josm/gui/MainPanel.java

@@ -159 +159 @@
 
             @Override
             public void afterLastLayerRemoved(LayerAvailabilityEvent e) {
-                updateContent(false);
+                if (!MainApplication.worker.isShutdown()) {
+                    updateContent(false);
+                }
             }
         });
         GuiHelper.runInEDTAndWait(() -> updateContent(!layerManager.getLayers().isEmpty()));

Another option would obviously be to shutdown the worker later (after all layers are removed), but I don't think that's of any use (except that it delays the exit by updating the content panel which is about to be destroyed).

[Don-vip]

Looks fine!

[Don-vip]

Mmm nop. It skips the entire destroy() stuff. Skipping this opens a huge can of worms. For example some window geometries are being remembered in user preferences at this stage, and so on.

SelectAction should have unregistered its listeners. It's being called in SelectAction.exitMode() and I'm quite sure the current map mode was properly exited when closing JOSM at some time.

In fact I may have applied #20989 too fast, this chain of events must be met when JOSM is closed:
realRemoveSingleLayer => setActiveLayer(null, true) => fireActiveLayerChange => activeOrEditLayerChanged => mapMode.exitMode() => SelectAction.exitMode() => keyDetector.remove*

So I'm going to revert r17991 and find another way to fix #20989

[Don-vip]

In 18010/josm:

fix #21107 - see #20989 - revert of r17991 - restore previous chain of events when closing JOSM