JOSM #22308 - Add option to toggle layer read-only status to popup menu

[Woazboat]

https://josm.openstreetmap.de/ticket/22308

[tsmock]

Overall, this looks good.

One additional thing I'd like to see is basic sanity tests, i.e. something like this:

@Test
void testNullLayer() {
assertThrows(NullPointerException.class, () -> new ToggleEditLockLayerAction(null));
}
@ParameterizedTest
@ValueSource(booleans = {true, false})
void testLayerLock(boolean locked) {
OsmDataLayer testLayer = new OsmDataLayer(new DataSet(), "testLayerLock", null);
if (locked) {
testLayer.lock();
}
// Sanity check
assertEquals(locked, testLayer.isLocked());
ToggleEditLockLayerAction action = new ToggleEditLockLayerAction(testLayer);
action.actionPerformed(null);
assertNotEquals(locked, testLayer.isLocked());
action.actionPerformed(null);
assertEquals(locked, testLayer.isLocked());
}

Please note that I wrote that in GitHub, so I probably mistyped something somewhere.

[tsmock]

Suggested change

	* @since XXX
	* @since xxx

The script to replace the xxx only looks for since xxx. See scripts/since_xxx.py for details, if you want them.

[Woazboat]

Hmmm, a comment starting with uppercase XXX has the added benefit that it's recognized as a TODO by a lot of editors. Is it okay if I simply modify the regex in the script to also accept uppercase XXX?

[tsmock]

Maybe, but we are going to want to tag bastiK and stoecker on it -- I don't know if there was a reason to only do lower case xxx.

[Woazboat]

Here's the PR for it in any case: #101

[tsmock]

Stupid question: Is there any reason why you want the @since XXX to be flagged as a TODO? Especially since it should be replaced by a script?

[Woazboat]

No particular reason and it being flagged as TODO probably doesn't really matter. Might be useful to highlight it in case something gets missed somehow (however unlikely that might be). The main thing is that I don't really see a reason against doing it.

[Woazboat]

Changed the xxx to lowercase

[tsmock]

Any particular reason why you are requiring layer size to be 1? Why not

Suggested change

	return layers.size() == 1 && layers.get(0) instanceof Lockable;
	return layers.stream().allMatch(Lockable.class::isInstance);

EDIT: You'll want to implement MultiLayerAction if you want it to apply to multiple layers.

[Woazboat]

I modeled this after the prevent upload toggle action which works exactly the same way. Not sure if locking/unlocking multiple layers at once is ever really needed.

[tsmock]

Works for me.

[Woazboat]

One additional thing I'd like to see is basic sanity tests, i.e. something like this:

Done

[tsmock]

Looks good to me. I'll try to merge it today or tomorrow.

EDIT: skyper brought up a good comment in the actual ticket. I'll wait for a response there.

[tsmock]

Suggested change

* @since xxx

Not needed since this is an entirely new class -- the @since xxx on L25 covers everything in the file.

[tsmock]

Works for me.