JOSM #22308 - Add option to toggle layer read-only status to popup menu#99
JOSM #22308 - Add option to toggle layer read-only status to popup menu#99Woazboat wants to merge 1 commit into
Conversation
705978b to
e836838
Compare
tsmock
left a comment
There was a problem hiding this comment.
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.
| * An action enabling/disabling the {@linkplain AbstractModifiableLayer#lock() read-only flag} | ||
| * of the layer specified in the constructor. | ||
| * | ||
| * @since XXX |
There was a problem hiding this comment.
| * @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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public boolean supportLayers(List<Layer> layers) { | ||
| return layers.size() == 1 && layers.get(0) instanceof Lockable; |
There was a problem hiding this comment.
Any particular reason why you are requiring layer size to be 1? Why not
| 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.
There was a problem hiding this comment.
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.
e836838 to
6c5273f
Compare
6c5273f to
6cc9da9
Compare
Done |
| * Construct a new {@code ToggleEditLockLayerAction} | ||
| * @param layer the layer for which to toggle the {@linkplain Lockable#lock() read-only flag} | ||
| * | ||
| * @since xxx |
There was a problem hiding this comment.
| * @since xxx |
Not needed since this is an entirely new class -- the @since xxx on L25 covers everything in the file.
|
|
||
| @Override | ||
| public boolean supportLayers(List<Layer> layers) { | ||
| return layers.size() == 1 && layers.get(0) instanceof Lockable; |
https://josm.openstreetmap.de/ticket/22308