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 #21944 - Add 'replace selected' action to relation editor #90

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

Conversation

Woazboat
Copy link

Add a new action + button to the relation editor to replace chosen member primitives with objects from selection (while keeping the roles).

@grischard
Copy link
Collaborator

Don't forget to open a ticket on the JOSM side and to link the ticket here and this PR there.

@Woazboat
Copy link
Author

Ah sorry, forgot to add the reverse link here
https://josm.openstreetmap.de/ticket/21944

@stoecker stoecker changed the title Add 'replace selected' action to relation editor JOSM #21944 - Add 'replace selected' action to relation editor May 19, 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.

One comment with the replaceselectedright.svg: As we are replacing something, should some portion of the blue bar be red (maybe two pixels or something?).

return filterConfirmedPrimitives(primitives, false);
}

protected List<OsmPrimitive> filterConfirmedPrimitives(List<OsmPrimitive> primitives, boolean abortOnSkip) throws AddAbortException {
Copy link

Choose a reason for hiding this comment

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

Can you please add javadocs here?

Suggested change
protected List<OsmPrimitive> filterConfirmedPrimitives(List<OsmPrimitive> primitives, boolean abortOnSkip) throws AddAbortException {
/**
* Filter confirmed primitives
* @param primitives The primitives to filter on
* @param abortOnSkip If the user decides to not add a primitive, abort (throw {@code AddAbortException})
* @return The primitives to add to the relation. Never {@code null}, but may be an empty list.
* @throws AddAbortException when {@code abortOnSkip} is {@code true} <i>and</i> the user decides to not add a primitive.
* @since xxx
*/
protected List<OsmPrimitive> filterConfirmedPrimitives(List<OsmPrimitive> primitives, boolean abortOnSkip) throws AddAbortException {

You'll want to check this with ant pmd checkstyle, but it should be right.

As a side note, I'd prefer to make this method more generic (e.g. protected <O extends IPrimitive> List<O> filterConfirmedPrimitives(List<O> primitives, boolean abortOnSkip) throws AddAbortException), but I don't know how hard it would be to do (I suspect there would be a cascading chain of methods to change).

Copy link
Author

Choose a reason for hiding this comment

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

Added the javadocs. Unfortunately making the method more generic would require rewriting pretty much all of the relation editor code from OsmPrimitive to IPrimitive as far as I can tell from a quick glance.

@Woazboat Woazboat force-pushed the relation-editor-replace-action branch from 6af03cb to 0eae26d Compare August 31, 2022 14:21
@Woazboat
Copy link
Author

One comment with the replaceselectedright.svg: As we are replacing something, should some portion of the blue bar be red (maybe two pixels or something?).

I'm pretty sure a better icon than the one I quickly added here could be created. Unfortunately I didn't really have any good ideas how to represent replacement (and have limited art skills), so any suggestions or icon contributions are welcome.

@Woazboat Woazboat force-pushed the relation-editor-replace-action branch from 0eae26d to cadddb6 Compare January 11, 2023 01:21
@Woazboat
Copy link
Author

I changed the icon to be partially red and fixed some checkstyle issues

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.

3 participants