[patch] invisible layer becomes active when removing a layer

[dieterdreist]

When you remove the active layer, the first layer (top in list) in the stack becomes active, even if it is invisible. This is inconsistent with the normal behavior, e.g. when you select the next or previous layer, where invisible layers are skipped. It would seem more intuitive if the next lower visible data layer (counting from the deleted layer) became active instead, and if there is none, the next higher visible data layer counting from the deleted layer

[gaben]

Please attach or paste the status report.

(@team: is there a comment template for needinfo comments?)

[dieterdreist]

ok, I can’t currently access the machine, but will send it later. It’s on latest josm and was working as expected some months ago. I also noticed that after deleting the validator layer there continue to be issues displayed in the validator window.

[dieterdreist]

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2023-07-10 22:19:01 +0200 (Mon, 10 Jul 2023)
Revision:18776
Build-Date:2023-07-11 01:30:57
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18776 en) Mac OS X 13.4.1
OS Build number: macOS 13.4.1 (22F82)
Memory Usage: 1066 MB / 4096 MB (457 MB allocated, but free)
Java version: 20.0.1, Homebrew, OpenJDK 64-Bit Server VM
Look and Feel: javax.swing.plaf.metal.MetalLookAndFeel
Screen: Display 1 1920×1080 (scaling 1.00×1.00) Display 2 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→16×16, 32×32→32×32
System property file.encoding: UTF-8
System property sun.jnu.encoding: UTF-8
Locale info: en_IT
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djosm.home=<josm.pref>, -Dsun.java2d.opengl=true]
Dataset consistency test: No problems found

Plugins:
+ apache-commons (36034)
+ buildings_tools (36097)
+ editgpx (36097)
+ ejml (35924)
+ geotools (36068)
+ jackson (36034)
+ jaxb (35952)
+ jts (36004)
+ mbtiles (v2.6.1)
+ measurement (36097)
+ opendata (36097)
+ openqa (v0.3.3)
+ pbf (36034)
+ poly (36097)
+ todo (123)
+ undelete (36066)
+ utilsplugin2 (36097)

Tagging presets:
removed

[gaben]

Thank you!

[taylor.smock]

In 18837/josm:

Fix #23057: An invisible layer should not become active when removing a layer

[taylor.smock]

Ticket retargeted after milestone deleted

[gaben]

Hmm, after this patch aerial imagery usually gets selected.

To reproduce:

1. have three layers in this order:
   • data layer 2
   • data layer 1 <- select this as an active
   • an aerial image
2. delete "data layer 1"

The selection should go to "data layer 2", but instead the background image becomes active.

[gaben]

I cannot run tests in dialogs/layer/DeleteLayerActionTest.java, as I'm getting an exception in IntelliJ:

java.lang.NullPointerException: Cannot enter synchronized block because "this.ownedWindowList" is null

	at java.desktop/java.awt.Window.addOwnedWindow(Window.java:2838)
	at java.desktop/java.awt.Window.ownedInit(Window.java:649)
	at java.desktop/java.awt.Window.<init>(Window.java:610)
	at java.desktop/java.awt.Dialog.<init>(Dialog.java:675)
	at java.desktop/javax.swing.JDialog.<init>(JDialog.java:593)
	at java.desktop/javax.swing.JDialog.<init>(JDialog.java:526)
	at org.openstreetmap.josm.gui.io.SaveLayersDialog.<init>(SaveLayersDialog.java:152)
	at org.openstreetmap.josm.gui.io.SaveLayersDialog.saveUnsavedModifications(SaveLayersDialog.java:117)
	at org.openstreetmap.josm.gui.dialogs.layer.DeleteLayerAction.actionPerformed(DeleteLayerAction.java:48)
	at org.openstreetmap.josm.gui.dialogs.layer.DeleteLayerActionTest.testSetActiveLayerOnlyOneVisible0to8(DeleteLayerActionTest.java:57)

Do you have any idea what's the cause? Other tests which I was working on ran just fine.

[taylor.smock]

I use -ea -Djava.awt.headless=true -javaagent:/Users/tsmock/.ivy2/cache/org.jmockit/jmockit/jars/jmockit-1.49.a.jar in the VM options. You'll have to update the /Users/tsmock bit to point at your home directory though.

I actually have that set in the JUnit template so things "just work".

EDIT: I suspect the issue is due to jmockit not getting loaded.

[gaben]

Ah thanks! The -Djava.awt.headless=true option was missing. Is it documented somewhere? I'd like to add to the dev guide.

[taylor.smock]

I have no clue. I know that the tests are run headless on the server, so I tend to just write the tests so they work under headless mode.

[taylor.smock]

See source:trunk/build.xml@18891:467-506#L467​ for other flags that JOSM adds for the tests.

[gaben]

Mentioned in DevelopersGuide/CompilingUsingIntelliJ?action=diff&version=26&old_version=24.

[gaben]

I have a patch for this, but also a question from user perspective. What should happen to the selection when all the layers are invisible (hidden)?

[taylor.smock]

What should happen to the selection when all the layers are invisible (hidden)?

I have no clue. I'd probably just pick one in a consistent manner. E.g., the next layer down.

[gaben]

Replying to taylor.smock:

I'd probably just pick one in a consistent manner. E.g., the next layer down.

That's fair. The attached patch implements this with some usual doc fixes and unit tests. Please test it :)

[gaben]

patch reupload

[gaben]

I just realized it's no longer unit testing the DeleteLayerAction 🤦‍♂️. Anyway, maybe it still fits there. Sorry for spamming.

[taylor.smock]

Meh. There are plenty of integration tests in the unit test directory. As long as you aren't making a call to a server (or something else that is slow), I don't really care. :)

So what we are doing is testing the expected behavior of the DeleteLayerAction from a test class called DeleteLayerActionTest. Which is perfectly fine, IMO.

[gaben]

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

[taylor.smock]

@gaben: Why did you modify the Layer#toString method? Was it just for debugging, or was there an additional reason behind it?

[gaben]

Yes, it was mostly for debugging, but I thought this parameter was worth enough to remain there.

What are toString methods used for anyway, if not debugging?

[taylor.smock]

Ticket retargeted after milestone deleted

[taylor.smock]

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

[taylor.smock]

In 18895/josm:

Fix #23057: data layers should be selected next, not non-data layers (patch by gaben, modified)

Modifications are as follows:

• Fix test pollution uncovered by change in non-obvious determinate order of tests
• Fix code so that MainLayerManagerTest.testRemoveLayerUnsetsActiveLayer passes

[gaben]

Thank you! I was thinking about using better variable names as in the pushed version, but attaching patches for each ticket is a pain.

[taylor.smock]

No problem -- I would have pushed it yesterday, but I had to figure out why the tests were failing locally first. And fix those. I finished that yesterday, and then spent this morning figuring out the best way to fix MainLayerManagerTest.testRemoveLayerUnsetsActiveLayer.