Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Woazboat
Copy link

@Woazboat Woazboat commented Aug 22, 2022

@Woazboat Woazboat changed the title Add option to toggle layer read-only status to popup menu JOSM #22308 - Add option to toggle layer read-only status to popup menu Aug 22, 2022
Copy link

@tsmock tsmock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Woazboat Woazboat Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the xxx to lowercase


@Override
public boolean supportLayers(List<Layer> layers) {
return layers.size() == 1 && layers.get(0) instanceof Lockable;
Copy link

@tsmock tsmock Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@Woazboat
Copy link
Author

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

Done

Copy link

@tsmock tsmock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* Construct a new {@code ToggleEditLockLayerAction}
* @param layer the layer for which to toggle the {@linkplain Lockable#lock() read-only flag}
*
* @since xxx
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@Woazboat Woazboat marked this pull request as draft February 18, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants