Index: /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputDialog.java =================================================================== --- /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputDialog.java (revision 36180) +++ /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputDialog.java (revision 36181) @@ -4,5 +4,4 @@ import static org.openstreetmap.josm.tools.I18n.tr; -import java.awt.Choice; import java.awt.Color; import java.awt.Container; @@ -11,10 +10,12 @@ import java.awt.GridBagLayout; import java.awt.event.ActionEvent; -import java.util.ArrayList; import java.util.Iterator; +import java.util.List; +import java.util.Set; import java.util.TreeSet; import javax.swing.BoxLayout; import javax.swing.JCheckBox; +import javax.swing.JComponent; import javax.swing.JLabel; import javax.swing.JPanel; @@ -32,4 +33,5 @@ import org.openstreetmap.josm.gui.tagging.ac.AutoCompletionManager; import org.openstreetmap.josm.gui.util.WindowGeometry; +import org.openstreetmap.josm.gui.widgets.JosmComboBox; import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.GBC; @@ -37,5 +39,5 @@ /** * The HouseNumberInputDialog is the layout of the house number input logic. - * + *

* This dialog is concerned with the layout, all logic goes into the * HouseNumberinputHandler class. @@ -58,24 +60,18 @@ private final String buildingType; private final boolean relationExists; - final ArrayList housenumbers; + final List houseNumbers; protected static final String DEFAULT_MESSAGE = tr("Enter housenumbers or amount of segments"); private Container jContentPane; private JPanel inputPanel; - private JLabel loLabel; JTextField lo; - private JLabel hiLabel; JTextField hi; private JLabel numbersLabel; JTextField numbers; - private JLabel streetLabel; AutoCompComboBox streetComboBox; - private JLabel buildingLabel; AutoCompComboBox buildingComboBox; - private JLabel segmentsLabel; JTextField segments; JTextArea messageLabel; - private JLabel interpolationLabel; - Choice interpolation; + JosmComboBox interpolationType; JCheckBox handleRelationCheckBox; JCheckBox keepOutlineCheckBox; @@ -84,4 +80,5 @@ /** + * Create a new dialog to get settings for the current operation * @param street If street is not null, we assume, the name of the street to be fixed * and just show a label. If street is null, we show a ComboBox/InputField. @@ -91,8 +88,9 @@ * @param buildingType The value to add for building key * @param relationExists If the buildings can be added to an existing relation or not. - * @param housenumbers a list of house numbers in this outline (may be empty) + * @param houseNumbers a list of house numbers in this outline (may be empty) + * @param handler The callback for the inputs */ public HouseNumberInputDialog(HouseNumberInputHandler handler, Way street, String streetName, - String buildingType, boolean relationExists, ArrayList housenumbers) { + String buildingType, boolean relationExists, List houseNumbers) { super(MainApplication.getMainFrame(), tr("Terrace a house"), @@ -105,9 +103,9 @@ this.buildingType = buildingType; this.relationExists = relationExists; - this.housenumbers = housenumbers; + this.houseNumbers = houseNumbers; handler.dialog = this; JPanel content = getInputPanel(); setContent(content); - setButtonIcons(new String[] {"ok", "cancel" }); + setButtonIcons("ok", "cancel"); getJContentPane(); initialize(); @@ -128,5 +126,5 @@ this.hi.addFocusListener(this.inputHandler); this.segments.addFocusListener(this.inputHandler); - this.interpolation.addItemListener(this.inputHandler); + this.interpolationType.addItemListener(this.inputHandler); } @@ -165,17 +163,17 @@ messageLabel.setFocusable(false); // Needed so that lowest number can have focus immediately - interpolationLabel = new JLabel(tr("Interpolation")); - segmentsLabel = new JLabel(tr("Segments")); - streetLabel = new JLabel(tr("Street")); - buildingLabel = new JLabel(tr("Building")); - loLabel = new JLabel(tr("Lowest Number")); + JLabel interpolationLabel = new JLabel(tr("Interpolation")); + JLabel segmentsLabel = new JLabel(tr("Segments")); + JLabel streetLabel = new JLabel(tr("Street")); + JLabel buildingLabel = new JLabel(tr("Building")); + JLabel loLabel = new JLabel(tr("Lowest Number")); loLabel.setPreferredSize(new Dimension(111, 16)); loLabel.setToolTipText(tr("Lowest housenumber of the terraced house")); - hiLabel = new JLabel(tr("Highest Number")); + JLabel hiLabel = new JLabel(tr("Highest Number")); numbersLabel = new JLabel(tr("List of Numbers")); loLabel.setPreferredSize(new Dimension(111, 16)); final String txt = relationExists ? tr("add to existing associatedStreet relation") : tr("create an associatedStreet relation"); - handleRelationCheckBox = new JCheckBox(txt, relationExists ? Config.getPref().getBoolean(HANDLE_RELATION, true) : false); + handleRelationCheckBox = new JCheckBox(txt, relationExists && Config.getPref().getBoolean(HANDLE_RELATION, true)); keepOutlineCheckBox = new JCheckBox(tr("keep outline way"), Config.getPref().getBoolean(KEEP_OUTLINE, false)); @@ -187,13 +185,13 @@ inputPanel.add(loLabel, GBC.std().insets(3, 3, 0, 0)); - inputPanel.add(getLo(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0)); + inputPanel.add(getLo(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0)); inputPanel.add(hiLabel, GBC.std().insets(3, 3, 0, 0)); - inputPanel.add(getHi(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0)); + inputPanel.add(getHi(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0)); inputPanel.add(numbersLabel, GBC.std().insets(3, 3, 0, 0)); - inputPanel.add(getNumbers(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0)); + inputPanel.add(getNumbers(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0)); inputPanel.add(interpolationLabel, GBC.std().insets(3, 3, 0, 0)); inputPanel.add(getInterpolation(), GBC.eol().insets(5, 3, 0, 0)); inputPanel.add(segmentsLabel, GBC.std().insets(3, 3, 0, 0)); - inputPanel.add(getSegments(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0)); + inputPanel.add(getSegments(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0)); if (streetName == null) { inputPanel.add(streetLabel, GBC.std().insets(3, 3, 0, 0)); @@ -215,7 +213,7 @@ hi.setEnabled(false); interpolationLabel.setVisible(false); - interpolation.setVisible(false); - interpolation.setEnabled(false); - segments.setText(String.valueOf(housenumbers.size())); + interpolationType.setVisible(false); + interpolationType.setEnabled(false); + segments.setText(String.valueOf(houseNumbers.size())); segments.setEditable(false); } @@ -267,5 +265,5 @@ numbers = new JTextField(); - Iterator it = housenumbers.iterator(); + Iterator it = houseNumbers.iterator(); StringBuilder s = new StringBuilder(256); if (it.hasNext()) { @@ -338,16 +336,17 @@ * @return java.awt.Choice */ - private Choice getInterpolation() { - if (interpolation == null) { - interpolation = new Choice(); - interpolation.add(tr("All")); - interpolation.add(tr("Even/Odd")); + private JComponent getInterpolation() { + if (interpolationType == null) { + interpolationType = new JosmComboBox<>(); + interpolationType.setEditable(false); + interpolationType.addItem(tr("All")); + interpolationType.addItem(tr("Even/Odd")); if (Config.getPref().getInt(INTERPOLATION, 2) == 1) { - interpolation.select(tr("All")); + interpolationType.setSelectedItemText(tr("All")); } else { - interpolation.select(tr("Even/Odd")); - } - } - return interpolation; + interpolationType.setSelectedItemText(tr("Even/Odd")); + } + } + return interpolationType; } @@ -355,6 +354,7 @@ * Generates a list of all visible names of highways in order to do * autocompletion on the road name. - */ - TreeSet createAutoCompletionInfo() { + * @return The visible names + */ + Set createAutoCompletionInfo() { final TreeSet names = new TreeSet<>(); for (OsmPrimitive osm : MainApplication.getLayerManager().getEditDataSet() Index: /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputHandler.java =================================================================== --- /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputHandler.java (revision 36180) +++ /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputHandler.java (revision 36181) @@ -8,13 +8,11 @@ import java.awt.Container; import java.awt.event.ActionEvent; -import java.awt.event.ActionListener; import java.awt.event.FocusEvent; import java.awt.event.FocusListener; import java.awt.event.ItemEvent; import java.awt.event.ItemListener; -import java.util.ArrayList; +import java.util.List; import javax.swing.JButton; -import javax.swing.JOptionPane; import javax.swing.JTextField; @@ -26,6 +24,4 @@ import org.openstreetmap.josm.gui.tagging.ac.AutoCompComboBox; import org.openstreetmap.josm.spi.preferences.Config; -import org.openstreetmap.josm.tools.Logging; -import org.openstreetmap.josm.tools.UserCancelException; import org.openstreetmap.josm.tools.Utils; @@ -33,5 +29,5 @@ * The Class HouseNumberInputHandler contains all the logic * behind the house number input dialog. - * + *

* From a refactoring viewpoint, this class is indeed more interested in the fields * of the HouseNumberInputDialog. This is desired design, as the HouseNumberInputDialog @@ -40,12 +36,13 @@ * @author casualwalker - Copyright 2009 CloudMade Ltd */ -public class HouseNumberInputHandler extends JosmAction implements ActionListener, FocusListener, ItemListener { +public class HouseNumberInputHandler extends JosmAction implements FocusListener, ItemListener { private final TerracerAction terracerAction; - private final Way outline, street; + private final Way outline; + private final Way street; private final String streetName; private final Node init; private final Relation associatedStreet; - private final ArrayList housenumbers; - public HouseNumberInputDialog dialog; + private final List housenumbers; + HouseNumberInputDialog dialog; /** @@ -67,5 +64,5 @@ final Way outline, final Node init, final Way street, final String streetName, final String buildingType, final Relation associatedStreet, - final ArrayList housenumbers, final String title) { + final List housenumbers, final String title) { this.terracerAction = terracerAction; this.outline = outline; @@ -109,33 +106,31 @@ * When the validation fails, a red message is * displayed and the OK button is disabled. - * + *

* Should be triggered each time the input changes. + * @return {@code true} if the inputs are ok */ private boolean validateInput() { boolean isOk = true; - StringBuffer message = new StringBuffer(); - - isOk = isOk && checkNumberOrder(message); - isOk = isOk && checkSegmentsFromHousenumber(message); - isOk = isOk && checkSegments(message); + final StringBuilder message = new StringBuilder(); + + isOk &= checkNumberOrder(message); + isOk &= checkSegmentsFromHousenumber(message); + isOk &= checkSegments(message); // Allow non numeric characters for the low number as long as there is // no high number of the segmentcount is 1 if (dialog.hi.getText().length() > 0 && (segments() != null || segments() < 1)) { - isOk = isOk - && checkNumberStringField(dialog.lo, tr("Lowest number"), + isOk &= checkNumberStringField(dialog.lo, tr("Lowest number"), message); } - isOk = isOk - && checkNumberStringField(dialog.hi, tr("Highest number"), + isOk &= checkNumberStringField(dialog.hi, tr("Highest number"), message); - isOk = isOk - && checkNumberStringField(dialog.segments, tr("Segments"), + isOk &= checkNumberStringField(dialog.segments, tr("Segments"), message); + JButton okButton = getButton(dialog, "OK"); + if (okButton != null) + okButton.setEnabled(isOk); if (isOk) { - JButton okButton = getButton(dialog, "OK"); - if (okButton != null) - okButton.setEnabled(true); // For some reason the messageLabel doesn't want to show up @@ -144,8 +139,4 @@ return true; } else { - JButton okButton = getButton(dialog, "OK"); - if (okButton != null) - okButton.setEnabled(false); - // For some reason the messageLabel doesn't want to show up, so a // MessageDialog is shown instead. Someone more knowledgeable might fix this. @@ -168,11 +159,9 @@ * @return true, if successful */ - private boolean checkNumberOrder(final StringBuffer message) { - if (numberFrom() != null && numberTo() != null) { - if (numberFrom().intValue() > numberTo().intValue()) { - appendMessageNewLine(message); - message.append(tr("Lowest housenumber cannot be higher than highest housenumber")); - return false; - } + private boolean checkNumberOrder(final StringBuilder message) { + if (numberFrom() != null && numberTo() != null && numberFrom() > numberTo()) { + appendMessageNewLine(message); + message.append(tr("Lowest housenumber cannot be higher than highest housenumber")); + return false; } return true; @@ -182,5 +171,5 @@ * Obtain the number segments from the house number fields and check, * if they are valid. - * + *

* Also disables the segments field, if the house numbers contain * valid information. @@ -190,10 +179,10 @@ * @return true, if successful */ - private boolean checkSegmentsFromHousenumber(final StringBuffer message) { + private boolean checkSegmentsFromHousenumber(final StringBuilder message) { if (!dialog.numbers.isVisible()) { dialog.segments.setEditable(true); if (numberFrom() != null && numberTo() != null) { - int segments = numberTo().intValue() - numberFrom().intValue(); + int segments = numberTo() - numberFrom(); if (segments % stepSize() != 0) { @@ -222,6 +211,6 @@ * @return true, if successful */ - private boolean checkSegments(final StringBuffer message) { - if (segments() == null || segments().intValue() < 1) { + private boolean checkSegments(final StringBuilder message) { + if (segments() == null || segments() < 1) { appendMessageNewLine(message); message.append(tr("Segment must be a number greater 1")); @@ -241,6 +230,6 @@ * @return true, if successful */ - private boolean checkNumberStringField(final JTextField field, - final String label, final StringBuffer message) { + private static boolean checkNumberStringField(final JTextField field, + final String label, final StringBuilder message) { final String content = field.getText(); if (content != null && content.length() != 0) { @@ -267,5 +256,5 @@ * @param message the message */ - private void appendMessageNewLine(final StringBuffer message) { + private static void appendMessageNewLine(final StringBuilder message) { if (message.length() > 0) { message.append("\n"); @@ -287,21 +276,17 @@ saveValues(); - try { - terracerAction.terraceBuilding( - outline, - init, - street, - associatedStreet, - segments(), - dialog.lo.getText(), - dialog.hi.getText(), - stepSize(), - housenumbers, - streetName(), - doHandleRelation(), - doKeepOutline(), buildingType()); - } catch (UserCancelException ex) { - Logging.trace(ex); - } + terracerAction.terraceBuilding( + outline, + init, + street, + associatedStreet, + segments(), + dialog.lo.getText(), + dialog.hi.getText(), + stepSize(), + housenumbers, + streetName(), + doHandleRelation(), + doKeepOutline(), buildingType()); this.dialog.setVisible(false); @@ -323,5 +308,5 @@ */ public Integer stepSize() { - return dialog.interpolation.getSelectedItem().equals(tr("All")) ? 1 : 2; + return tr("All").equals(dialog.interpolationType.getSelectedItem()) ? 1 : 2; } @@ -405,19 +390,13 @@ * Whether the user likes to create a relation or add to * an existing one. + * @return {@code true} if the user wants to create a relation */ public boolean doHandleRelation() { - if (this.dialog == null) { - JOptionPane.showMessageDialog(null, "dialog", "alert", JOptionPane.ERROR_MESSAGE); - } - if (this.dialog.handleRelationCheckBox == null) { - JOptionPane.showMessageDialog(null, "checkbox", "alert", JOptionPane.ERROR_MESSAGE); - return true; - } else { - return this.dialog.handleRelationCheckBox.isSelected(); - } + return this.dialog.handleRelationCheckBox.isSelected(); } /** * Whether the user likes to keep the outline way. + * @return {@code true} if the user wants to keep the selected outline */ public boolean doKeepOutline() { Index: /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/ReverseTerraceAction.java =================================================================== --- /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/ReverseTerraceAction.java (revision 36180) +++ /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/ReverseTerraceAction.java (revision 36181) @@ -6,8 +6,11 @@ import java.awt.event.ActionEvent; import java.awt.event.KeyEvent; +import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; +import java.util.Deque; import java.util.HashSet; import java.util.LinkedList; +import java.util.Set; import javax.swing.JOptionPane; @@ -26,15 +29,19 @@ /** * Tool to reverse the house numbers in a terrace. - * + *

* Useful for when you're using the Terracer tool and the house numbers come out * in the wrong direction, or when someone has added house numbers in the wrong * direction anyway. - * - * Finds all connected ways which have a building=* tag on them in order (breadth + *

+ * Finds all connected ways which have a building=* and addr tag on them in order (breadth * first search) and then changes the tags to be the reverse of the order in which * they were found. */ public class ReverseTerraceAction extends JosmAction { + private static final String ADDR_HOUSENUMBER = "addr:housenumber"; + /** + * Create a new action for reversing a terrace + */ public ReverseTerraceAction() { super(tr("Reverse a terrace"), @@ -54,29 +61,16 @@ public void actionPerformed(ActionEvent e) { Collection selectedWays = MainApplication.getLayerManager().getEditDataSet().getSelectedWays(); + reverseTerracedAddresses(selectedWays); + } + static void reverseTerracedAddresses(Collection selectedWays) { // Set to keep track of all the nodes that have been visited - that is: if // we encounter them again we will not follow onto the connected ways. - HashSet visitedNodes = new HashSet<>(); + Set visitedNodes = new HashSet<>(); // Set to keep track of the ways the algorithm has seen, but not yet visited. // Since when a way is visited all of its nodes are marked as visited, there // is no need to keep a visitedWays set. - HashSet front = new HashSet<>(); - - // Find the first or last way from the teracced houses. - // It should be connected to exactly one other way. - for (Way w : selectedWays) { - int conn = 0; - for (Way v : selectedWays) { - if (w.equals(v)) continue; - if (!Collections.disjoint(w.getNodes(), v.getNodes())) { - ++conn; - } - } - if (conn == 1) { - front.add(w); - break; - } - } + final Deque front = findFirstWay(selectedWays); if (front.isEmpty()) { @@ -86,4 +80,5 @@ } + // This is like a visitedWays set, but in a linear order. LinkedList orderedWays = new LinkedList<>(); @@ -92,7 +87,7 @@ LinkedList houseNumbers = new LinkedList<>(); - while (front.size() > 0) { + while (!front.isEmpty()) { // Java apparently doesn't have useful methods to get single items from sets... - Way w = front.iterator().next(); + Way w = front.pop(); // Visit all the nodes in the way, adding the building's they're members of @@ -101,5 +96,6 @@ if (!visitedNodes.contains(n)) { for (OsmPrimitive prim : n.getReferrers()) { - if (prim.keySet().contains("building") && prim instanceof Way) { + if (prim.hasKey("building") && prim.hasKey(ADDR_HOUSENUMBER) + && prim instanceof Way && !front.contains(prim)) { front.add((Way) prim); } @@ -111,7 +107,6 @@ // We've finished visiting this way, so record the attributes we're interested // in for re-writing. - front.remove(w); orderedWays.addLast(w); - houseNumbers.addFirst(w.get("addr:housenumber")); + houseNumbers.addFirst(w.get(ADDR_HOUSENUMBER)); } @@ -120,5 +115,5 @@ commands.add(new ChangePropertyCommand( orderedWays.get(i), - "addr:housenumber", + ADDR_HOUSENUMBER, houseNumbers.get(i))); } @@ -128,4 +123,24 @@ } + private static Deque findFirstWay(Collection selectedWays) { + // Find the first or last way from the terraced houses. + // It should be connected to exactly one other way. + for (Way w : selectedWays) { + int conn = 0; + for (Way v : selectedWays) { + if (!w.equals(v) && !Collections.disjoint(w.getNodes(), v.getNodes())) { + ++conn; + if (conn > 1) { + break; + } + } + } + if (conn == 1) { + return new ArrayDeque<>(Collections.singletonList(w)); + } + } + return new ArrayDeque<>(); + } + @Override protected void updateEnabledState() { Index: /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerAction.java =================================================================== --- /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerAction.java (revision 36180) +++ /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerAction.java (revision 36181) @@ -14,5 +14,4 @@ import java.util.HashMap; import java.util.HashSet; -import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -55,5 +54,5 @@ * a street (highway=*, name=*) then the given street will be added * to the 'associatedStreet' relation. - * + *

* * At present it only works on quadrilaterals, but there is no reason @@ -64,4 +63,7 @@ */ public final class TerracerAction extends JosmAction { + private static final String BUILDING = "building"; + private static final String ADDR_HOUSENUMBER = "addr:housenumber"; + private static final String ADDR_STREET = "addr:street"; private Collection commands; @@ -123,8 +125,6 @@ } else if (sel.size() > 1) { List ways = new ArrayList<>(Utils.filteredCollection(sel, Way.class)); - Iterator wit = ways.iterator(); - while (wit.hasNext()) { - Way way = wit.next(); - if (way.hasKey("building")) { + for (Way way : ways) { + if (way.hasKey(BUILDING)) { if (outline != null) // already have a building @@ -140,5 +140,5 @@ throw new InvalidUserInputException("street does not have any name"); } else - throw new InvalidUserInputException(way+" is neither a building nor a highway"); + throw new InvalidUserInputException(way + " is neither a building nor a highway"); } @@ -147,11 +147,9 @@ List nodes = new ArrayList<>(Utils.filteredCollection(sel, Node.class)); - Iterator nit = nodes.iterator(); // Actually this should test if the selected address nodes lie // within the selected outline. Any ideas how to do this? - while (nit.hasNext()) { - Node node = nit.next(); - if (node.hasKey("addr:housenumber")) { - String nodesStreetName = node.get("addr:street"); + for (Node node : nodes) { + if (node.hasKey(ADDR_HOUSENUMBER)) { + String nodesStreetName = node.get(ADDR_STREET); // if a node has a street name if must be equal // to the one of the other address nodes @@ -174,5 +172,5 @@ } - Collections.sort(housenumbers, new HousenumberNodeComparator()); + housenumbers.sort(new HousenumberNodeComparator()); } @@ -182,6 +180,6 @@ } catch (InvalidUserInputException ex) { Logging.warn("Terracer: "+ex.getMessage()); - new ExtendedDialog(MainApplication.getMainFrame(), tr("Invalid selection"), new String[] {"OK"}) - .setButtonIcons(new String[] {"ok"}).setIcon(JOptionPane.INFORMATION_MESSAGE) + new ExtendedDialog(MainApplication.getMainFrame(), tr("Invalid selection"), "OK") + .setButtonIcons("ok").setIcon(JOptionPane.INFORMATION_MESSAGE) .setContent(tr("Select a single, closed way of at least four nodes. " + "(Optionally you can also select a street for the addr:street tag " + @@ -217,14 +215,10 @@ // Special case of one outline and one address node. // Don't open the dialog - try { - terraceBuilding(outline, init, street, associatedStreet, 0, null, null, 0, - housenumbers, streetname, associatedStreet != null, false, "yes"); - } catch (UserCancelException ex) { - Logging.trace(ex); - } + terraceBuilding(outline, init, street, associatedStreet, 0, null, null, 0, + housenumbers, streetname, associatedStreet != null, false, "yes"); } else { String title = trn("Change {0} object", "Change {0} objects", sel.size(), sel.size()); // show input dialog. - new HouseNumberInputHandler(this, outline, init, street, streetname, outline.get("building"), + new HouseNumberInputHandler(this, outline, init, street, streetname, outline.get(BUILDING), associatedStreet, housenumbers, title).dialog.showDialog(); } @@ -238,5 +232,5 @@ } - public Integer getNumber(String number) { + private static Integer getNumber(String number) { try { return Integer.parseInt(number); @@ -251,5 +245,5 @@ */ static class HousenumberNodeComparator implements Comparator { - private final Pattern pat = Pattern.compile("^(\\d+)\\s*(.*)"); + private static final Pattern PATTERN_HOUSE_NUMBER = Pattern.compile("^(\\d+)\\s*(.*)", Pattern.UNICODE_CHARACTER_CLASS); @Override @@ -259,11 +253,11 @@ // doesn't work for numbers with different number of digits, // e.g. 9 is higher than 11 - String node1String = node1.get("addr:housenumber"); - String node2String = node2.get("addr:housenumber"); - Matcher mat = pat.matcher(node1String); + String node1String = node1.get(ADDR_HOUSENUMBER); + String node2String = node2.get(ADDR_HOUSENUMBER); + Matcher mat = PATTERN_HOUSE_NUMBER.matcher(node1String); if (mat.find()) { Integer node1Int = Integer.valueOf(mat.group(1)); String node1Rest = mat.group(2); - mat = pat.matcher(node2String); + mat = PATTERN_HOUSE_NUMBER.matcher(node2String); if (mat.find()) { Integer node2Int = Integer.valueOf(mat.group(1)); @@ -285,5 +279,5 @@ /** * Terraces a single, closed, quadrilateral way. - * + *

* Any node must be adjacent to both a short and long edge, we naively * choose the longest edge and its opposite and interpolate along them @@ -307,18 +301,18 @@ * @param keepOutline If the outline way should be kept * @param buildingValue The value for {@code building} key to add - * @throws UserCancelException if user cancels the operation */ public void terraceBuilding(final Way outline, Node init, Way street, Relation associatedStreet, Integer segments, String start, String end, int step, List housenumbers, String streetName, boolean handleRelations, - boolean keepOutline, String buildingValue) throws UserCancelException { + boolean keepOutline, String buildingValue) { final int nb; - Integer to = null, from = null; + Integer to; + Integer from = null; if (housenumbers == null || housenumbers.isEmpty()) { to = getNumber(end); from = getNumber(start); if (to != null && from != null) { - nb = 1 + (to.intValue() - from.intValue()) / step; + nb = 1 + (to - from) / step; } else if (segments != null) { - nb = segments.intValue(); + nb = segments; } else { // if we get here, there is is a bug in the input validation. @@ -335,5 +329,5 @@ Pair interp = findFrontAndBack(outline); - final boolean swap = init != null && (interp.a.lastNode().equals(init) || interp.b.lastNode().equals(init)); + final boolean swap = init != null && (init.equals(interp.a.lastNode()) || init.equals(interp.b.lastNode())); final double frontLength = wayLength(interp.a); @@ -418,5 +412,5 @@ // Remove the address nodes since their tags have been incorporated into the terraces. // Or should removing them also be an option? - if (!housenumbers.isEmpty()) { + if (housenumbers != null && !housenumbers.isEmpty()) { commands.add(DeleteCommand.delete(housenumbers, true, true)); } @@ -511,14 +505,16 @@ * @param associatedStreet The associated street. Used to determine if addr:street should be set or not. * @param buildingValue The value for {@code building} key to add - * @throws UserCancelException if user cancels the operation + * @param houseNumbers The house numbers to use + * @param i The index to use in {@code houseNumbers} for a replacement house number (preferential) + * @param defaultNumber The number to use if there was not an underlying house number */ private void addressBuilding(Way outline, Way street, String streetName, Relation associatedStreet, - List housenumbers, int i, String defaultNumber, String buildingValue) throws UserCancelException { - Node houseNum = (housenumbers != null && i >= 0 && i < housenumbers.size()) ? housenumbers.get(i) : null; + List houseNumbers, int i, String defaultNumber, String buildingValue) { + Node houseNum = (houseNumbers != null && i >= 0 && i < houseNumbers.size()) ? houseNumbers.get(i) : null; boolean buildingAdded = false; boolean numberAdded = false; Map tags = new HashMap<>(); if (houseNum != null) { - primitives = Arrays.asList(new OsmPrimitive[]{houseNum, outline}); + primitives = Arrays.asList(houseNum, outline); TagCollection tagsToCopy = TagCollection.unionOfAllPrimitives(primitives).getTagsFor(houseNum.keySet()); @@ -530,19 +526,19 @@ } - buildingAdded = houseNum.hasKey("building"); - numberAdded = houseNum.hasKey("addr:housenumber"); + buildingAdded = houseNum.hasKey(BUILDING); + numberAdded = houseNum.hasKey(ADDR_HOUSENUMBER); } if (!buildingAdded && buildingValue != null && !buildingValue.isEmpty()) { - tags.put("building", buildingValue); + tags.put(BUILDING, buildingValue); } if (defaultNumber != null && !numberAdded) { - tags.put("addr:housenumber", defaultNumber); + tags.put(ADDR_HOUSENUMBER, defaultNumber); } // Only put addr:street if no relation exists or if it has no name if (associatedStreet == null || !associatedStreet.hasKey("name")) { if (street != null) { - tags.put("addr:street", street.get("name")); + tags.put(ADDR_STREET, street.get("name")); } else if (streetName != null && !streetName.trim().isEmpty()) { - tags.put("addr:street", streetName.trim()); + tags.put(ADDR_STREET, streetName.trim()); } } @@ -555,5 +551,5 @@ * Creates a node at a certain distance along a way, as calculated by the * great circle distance. - * + *

* Note that this really isn't an efficient way to do this and leads to * O(N^2) running time for the main algorithm, but its simple and easy @@ -564,5 +560,5 @@ * @return A node at a distance l along w from the first point. */ - private Node interpolateAlong(Way w, double l) { + private static Node interpolateAlong(Way w, double l) { List> pairs = w.getNodePairs(false); for (int i = 0; i < pairs.size(); ++i) { @@ -587,5 +583,5 @@ * @return The length of the way. */ - private double wayLength(Way w) { + private static double wayLength(Way w) { double length = 0.0; for (Pair p : w.getNodePairs(false)) { @@ -603,5 +599,5 @@ * @return A pair of ways (front, back) pointing in the same directions. */ - private Pair findFrontAndBack(Way w) { + private static Pair findFrontAndBack(Way w) { // calculate the "side-ness" score for each segment of the way double[] sideness = calculateSideness(w); @@ -658,6 +654,10 @@ /** * returns the distance of two segments of a closed polygon - */ - private int indexDistance(int i1, int i2, int n) { + * @param i1 The first segment index + * @param i2 The second segment index + * @param n The number of segments in the polygon + * @return The distance between the two segments + */ + private static int indexDistance(int i1, int i2, int n) { return Math.min(positiveModulus(i1 - i2, n), positiveModulus(i2 - i1, n)); } @@ -665,6 +665,9 @@ /** * return the modulus in the range [0, n) - */ - private int positiveModulus(int a, int n) { + * @param a dividend + * @param n divisor + * @return The positive modulus (if {@code a} is negative) + */ + private static int positiveModulus(int a, int n) { if (n <= 0) throw new IllegalArgumentException(); @@ -679,6 +682,9 @@ * Calculate the length of a side (from node i to i+1) in a way. This assumes that * the way is closed, but I only ever call it for buildings. - */ - private double sideLength(Way w, int i) { + * @param w The way + * @param i The side (0 indexed) + * @return The length of that way segment + */ + private static double sideLength(Way w, int i) { Node a = w.getNode(i); Node b = w.getNode((i + 1) % (w.getNodesCount() - 1)); @@ -690,5 +696,5 @@ * into order and return the array of indexes such that, for a returned array * x, a[x[i]] is sorted for ascending index i. - * + *

* This isn't efficient at all, but should be fine for the small arrays we're * expecting. If this gets slow - replace it with some more efficient algorithm. @@ -698,19 +704,5 @@ * is in sorted order. */ - private int[] sortedIndexes(final double[] a) { - class SortWithIndex implements Comparable { - public double x; - public int i; - - SortWithIndex(double a, int b) { - x = a; - i = b; - } - - @Override - public int compareTo(SortWithIndex o) { - return Double.compare(x, o.x); - } - } + private static int[] sortedIndexes(final double[] a) { final int length = a.length; @@ -731,6 +723,8 @@ /** * Calculate "sideness" metric for each segment in a way. - */ - private double[] calculateSideness(Way w) { + * @param w The way to get the sideness metric for + * @return The sideness for each segment of the way + */ + private static double[] calculateSideness(Way w) { final int length = w.getNodesCount() - 1; double[] sideness = new double[length]; @@ -752,6 +746,11 @@ * segment and its previous and next segments in order. Sideness is calculated * for the segment b-c. - */ - private double calculateSideness(Node a, Node b, Node c, Node d) { + * @param a The previous node + * @param b The first node of the current segment + * @param c The last node of the current segment + * @param d The next node + * @return the sideness + */ + private static double calculateSideness(ILatLon a, ILatLon b, ILatLon c, ILatLon d) { final double ndx = b.lon() - a.lon(); final double pdx = d.lon() - c.lon(); @@ -766,5 +765,5 @@ * Creates a new node at the interpolated position between the argument * nodes. Interpolates linearly in projected coordinates. - * + *

* If new node coordinate matches a or b coordinates, a or b is returned. * @@ -774,7 +773,7 @@ * @return A new node at the interpolated position (or a or b in case if f ≈ 0 or f ≈ 1). */ - private Node interpolateNode(Node a, Node b, double f) { + private static Node interpolateNode(Node a, Node b, double f) { Node n = new Node(a.getEastNorth().interpolate(b.getEastNorth(), f)); - if (n.equalsEpsilon(a, ILatLon.MAX_SERVER_PRECISION)) + if (n.equalsEpsilon(a, ILatLon.MAX_SERVER_PRECISION)) return a; if (n.equalsEpsilon(b, ILatLon.MAX_SERVER_PRECISION)) @@ -787,3 +786,24 @@ setEnabled(getLayerManager().getEditDataSet() != null); } + + private static class SortWithIndex implements Comparable { + /** + * The value to sort + */ + public final double x; + /** + * The index in the original array + */ + public final int i; + + SortWithIndex(double a, int b) { + x = a; + i = b; + } + + @Override + public int compareTo(SortWithIndex o) { + return Double.compare(x, o.x); + } + } } Index: /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerPlugin.java =================================================================== --- /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerPlugin.java (revision 36180) +++ /applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerPlugin.java (revision 36181) @@ -26,17 +26,17 @@ */ public class TerracerPlugin extends Plugin implements Destroyable { - private List actions = Arrays.asList(new TerracerAction(), new ReverseTerraceAction()); + private final List actions = Arrays.asList(new TerracerAction(), new ReverseTerraceAction()); public TerracerPlugin(PluginInformation info) { super(info); for (JosmAction action : actions) { - MainMenu.add(MainApplication.getMenu().moreToolsMenu, action); + MainMenu.add(MainApplication.getMenu().moreToolsMenu, action); } } - @Override - public void destroy() { + @Override + public void destroy() { final JMenu moreToolsMenu = MainApplication.getMenu().moreToolsMenu; - final Map actionsMap = Arrays.asList(moreToolsMenu.getMenuComponents()).stream() + final Map actionsMap = Arrays.stream(moreToolsMenu.getMenuComponents()) .filter(JMenuItem.class::isInstance).map(JMenuItem.class::cast) .collect(Collectors.toMap(JMenuItem::getAction, component -> component)); @@ -47,5 +47,5 @@ } } - actions.forEach(JosmAction::destroy); - } + actions.forEach(JosmAction::destroy); + } } Index: /applications/editors/josm/plugins/terracer/test/unit/org/openstreetmap/josm/plugins/terracer/ReverseTerraceActionTest.java =================================================================== --- /applications/editors/josm/plugins/terracer/test/unit/org/openstreetmap/josm/plugins/terracer/ReverseTerraceActionTest.java (revision 36181) +++ /applications/editors/josm/plugins/terracer/test/unit/org/openstreetmap/josm/plugins/terracer/ReverseTerraceActionTest.java (revision 36181) @@ -0,0 +1,72 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.plugins.terracer; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.testutils.annotations.Main; +import org.openstreetmap.josm.testutils.annotations.Projection; + +/** + * Test class for {@link ReverseTerraceAction} + */ +@Main +@Projection +class ReverseTerraceActionTest { + private static final String ADDR_HOUSENUMBER = "addr:housenumber"; + private static Node[] generateNodes(double... latLons) { + assertEquals(0, latLons.length % 2); + final Node[] nodes = new Node[latLons.length / 2]; + for (int i = 0; i < nodes.length; i++) { + nodes[i] = new Node(new LatLon(latLons[2 * i], latLons[2 * i + 1])); + } + return nodes; + } + + @Test + void testNonRegression6855() { + final Way way1 = TestUtils.newWay("building=apartments", generateNodes(49.766198, 4.3270878, 49.7663026, 4.3270911, + 49.7663282, 4.3269524, 49.7662016, 4.3269471)); + final Way way2 = TestUtils.newWay("building=apartments", generateNodes(49.7655382, 4.3269193, 49.7655362, 4.3270665)); + final DataSet dataSet = new DataSet(); + MainApplication.getLayerManager().addLayer(new OsmDataLayer(dataSet, "ReverseTerraceActionTest#testNonRegression6855", null)); + dataSet.addPrimitiveRecursive(way1); + dataSet.addPrimitiveRecursive(way2); + way2.addNode(0, way1.lastNode()); + way2.addNode(way1.firstNode()); + way1.addNode(way1.firstNode()); + way2.addNode(way2.firstNode()); + dataSet.setSelected(way2); + new TerracerAction().terraceBuilding(way2, null, null, null, 5, + "80", "88", 2, Collections.emptyList(), null, false, false, "apartments"); + final List addressedWays = dataSet.getWays().stream().filter(w -> w != way1) + .sorted(Comparator.comparingLong(w -> -w.getUniqueId())).collect(Collectors.toList()); + assertAll("Terrace should work as expected", () -> assertEquals(5, addressedWays.size()), + () -> assertEquals("80", addressedWays.get(0).get(ADDR_HOUSENUMBER)), + () -> assertEquals("82", addressedWays.get(1).get(ADDR_HOUSENUMBER)), + () -> assertEquals("84", addressedWays.get(2).get(ADDR_HOUSENUMBER)), + () -> assertEquals("86", addressedWays.get(3).get(ADDR_HOUSENUMBER)), + () -> assertEquals("88", addressedWays.get(4).get(ADDR_HOUSENUMBER))); + dataSet.setSelected(addressedWays); + new ReverseTerraceAction().actionPerformed(null); + assertAll("Reverse Terrace should work as expected", () -> assertEquals(5, addressedWays.size()), + () -> assertEquals("88", addressedWays.get(0).get(ADDR_HOUSENUMBER)), + () -> assertEquals("86", addressedWays.get(1).get(ADDR_HOUSENUMBER)), + () -> assertEquals("84", addressedWays.get(2).get(ADDR_HOUSENUMBER)), + () -> assertEquals("82", addressedWays.get(3).get(ADDR_HOUSENUMBER)), + () -> assertEquals("80", addressedWays.get(4).get(ADDR_HOUSENUMBER))); + } +}