Subject: [PATCH] See #22677: remove man_made=communications_tower The root problem is that people are (apparently) doing a search for communication and selecting the entry with communication in the name. --- Index: src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java b/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java --- a/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java (revision 18719) +++ b/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java (date 1682618919285) @@ -4,6 +4,7 @@ import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.BorderLayout; +import java.awt.Color; import java.awt.Component; import java.awt.Dimension; import java.awt.event.ActionEvent; @@ -11,12 +12,15 @@ import java.util.Collection; import java.util.Collections; import java.util.EnumSet; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -43,10 +47,12 @@ import org.openstreetmap.josm.gui.tagging.presets.items.ComboMultiSelect; import org.openstreetmap.josm.gui.tagging.presets.items.Key; import org.openstreetmap.josm.gui.tagging.presets.items.KeyedItem; +import org.openstreetmap.josm.gui.tagging.presets.items.Label; import org.openstreetmap.josm.gui.tagging.presets.items.Roles; import org.openstreetmap.josm.gui.tagging.presets.items.Roles.Role; import org.openstreetmap.josm.gui.widgets.PopupMenuLauncher; import org.openstreetmap.josm.gui.widgets.SearchTextResultListPanel; +import org.openstreetmap.josm.tools.ColorHelper; import org.openstreetmap.josm.tools.Destroyable; import org.openstreetmap.josm.tools.Utils; @@ -63,6 +69,8 @@ private static final int CLASSIFICATION_TAGS_MATCH = 100; private static final Pattern PATTERN_PUNCTUATION = Pattern.compile("\\p{Punct}", Pattern.UNICODE_CHARACTER_CLASS); + private static final Pattern PATTERN_WORD = Pattern.compile("\\s", Pattern.UNICODE_CHARACTER_CLASS); + private static final Pattern PATTERN_GROUP = Pattern.compile("[\\s/]", Pattern.UNICODE_CHARACTER_CLASS); private static final BooleanProperty SEARCH_IN_TAGS = new BooleanProperty("taggingpreset.dialog.search-in-tags", true); private static final BooleanProperty ONLY_APPLICABLE = new BooleanProperty("taggingpreset.dialog.only-applicable-to-selection", true); @@ -73,24 +81,104 @@ private boolean typesInSelectionDirty = true; private final transient PresetClassifications classifications = new PresetClassifications(); - private static class ResultListCellRenderer implements ListCellRenderer { + private class ResultListCellRenderer implements ListCellRenderer { private final DefaultListCellRenderer def = new DefaultListCellRenderer(); @Override public Component getListCellRendererComponent(JList list, TaggingPreset tp, int index, boolean isSelected, boolean cellHasFocus) { JLabel result = (JLabel) def.getListCellRendererComponent(list, tp, index, isSelected, cellHasFocus); + result.setToolTipText(null); result.setText(tp.getName()); + if (list.getModel().getSize() < 200 // Maybe this should be configurable? + && TaggingPresetSelector.this.ckSearchInTags != null + && TaggingPresetSelector.this.ckSearchInTags.isSelected()) { + final String searchText = getSearchText(); + final PresetClassification classification = new PresetClassification(tp); + final String[] wordSet = PATTERN_WORD.split(searchText, -1); + if (classification.isMatchingTags(wordSet) > 0) { + final String matchingTags = buildMatchingTagText(getSearchableTags(tp), wordSet); + if (matchingTags.length() > 0) { + result.setToolTipText(tr("Matching tags: {0}", matchingTags)); + Color complement = ColorHelper.complement(result.getBackground()); + // This gray works for light/dark themes + if (result.getBackground().getRGB() == Color.WHITE.getRGB()) { + complement = Color.GRAY; + } + String col = ColorHelper.color2html(complement); + result.setText("" + tp.getName() + "    " + + matchingTags + ""); + } + } + } result.setIcon((Icon) tp.getValue(Action.SMALL_ICON)); return result; } + + /** + * Get the text to show a user for tags that match the given search string + * @param tagMap The map of searchable tags + * @param wordSet The words to filter on + * @return The string to show the user + */ + private String buildMatchingTagText(Map> tagMap, String[] wordSet) { + final Predicate matchesWords = word -> { + for (String checkWord : wordSet) { + if (word.contains(checkWord)) { + return true; + } + } + return false; + }; + final StringBuilder sb = new StringBuilder(); + for (Map.Entry> entry : tagMap.entrySet()) { + if (sb.length() > 0 && sb.lastIndexOf(" ") != sb.length() - 1) { + sb.append(' '); + } + int length = sb.length(); + // Add the entry if it matches + if (matchesWords.test(entry.getKey())) { + sb.append(entry.getKey()); + if (entry.getValue().size() == 1) { + sb.append('=').append(entry.getValue().get(0)); + } + } + + if (!entry.getValue().isEmpty() && (length == sb.length() || entry.getValue().size() > 1)) { + // Add values if they match + boolean added = false; + for (String value : entry.getValue()) { + if (matchesWords.test(value)) { + // Add the key if it hasn't been added already + if (!added) { + if (length == sb.length()) { + sb.append(entry.getKey()).append('='); + } else if (length + entry.getKey().length() == sb.length()) { + sb.append('='); + } + } + if (added) { + sb.append(';'); + } else { + added = true; + } + sb.append(value); + } + } + } + } + return sb.toString(); + } } /** * Computes the match ration of a {@link TaggingPreset} wrt. a searchString. */ public static class PresetClassification implements Comparable { + /** The preset for this {@link PresetClassification} */ public final TaggingPreset preset; + /** Used for sorting presets that match a query */ public int classification; + /** Used to indicate that this was used last by a user (for "history") */ public int favoriteIndex; private final Collection groups; private final Collection names; @@ -100,31 +188,16 @@ this.preset = preset; Set groupSet = new HashSet<>(); Set nameSet = new HashSet<>(); - Set tagSet = new HashSet<>(); TaggingPreset group = preset.group; while (group != null) { addLocaleNames(groupSet, group); group = group.group; } addLocaleNames(nameSet, preset); - for (TaggingPresetItem item: preset.data) { - if (item instanceof KeyedItem) { - tagSet.add(((KeyedItem) item).key); - if (item instanceof ComboMultiSelect) { - final ComboMultiSelect cms = (ComboMultiSelect) item; - if (cms.values_searchable) { - tagSet.addAll(cms.getDisplayValues()); - } - } - if (item instanceof Key && ((Key) item).value != null) { - tagSet.add(((Key) item).value); - } - } else if (item instanceof Roles) { - for (Role role : ((Roles) item).roles) { - tagSet.add(role.key); - } - } - } + Map> searchableTags = getSearchableTags(preset); + Set tagSet = new HashSet<>(searchableTags.keySet()); + searchableTags.values().forEach(tagSet::addAll); + this.groups = Utils.toUnmodifiableList(groupSet); this.names = Utils.toUnmodifiableList(nameSet); this.tags = Utils.toUnmodifiableList(tagSet); @@ -133,7 +206,7 @@ private static void addLocaleNames(Collection collection, TaggingPreset preset) { String locName = preset.getLocaleName(); if (locName != null) { - Collections.addAll(collection, locName.toLowerCase(Locale.ENGLISH).split("\\s", -1)); + Collections.addAll(collection, PATTERN_WORD.split(locName.toLowerCase(Locale.ENGLISH), -1)); } } @@ -189,12 +262,52 @@ return result; } + @Override + public int hashCode() { + return Objects.hash(this.preset); + } + + @Override + public boolean equals(Object obj) { + return obj != null && + obj.getClass().equals(this.getClass()) && + Objects.equals(((PresetClassification) obj).preset, this.preset); + } + @Override public String toString() { return Integer.toString(classification) + ' ' + preset; } } + /** + * Get the searchable tag map for a preset + * @param preset The preset to parse + * @return A map of key to values that are searchable + */ + private static Map> getSearchableTags(TaggingPreset preset) { + final Map> tagMap = new HashMap<>(); + for (TaggingPresetItem item: preset.data) { + if (item instanceof KeyedItem) { + Collection list = tagMap.computeIfAbsent(((KeyedItem) item).key, k -> new ArrayList<>()); + if (item instanceof ComboMultiSelect) { + final ComboMultiSelect cms = (ComboMultiSelect) item; + if (cms.values_searchable) { + list.addAll(cms.getDisplayValues()); + } + } + if (item instanceof Key && ((Key) item).value != null) { + list.add(((Key) item).value); + } + } else if (item instanceof Roles) { + for (Role role : ((Roles) item).roles) { + tagMap.computeIfAbsent(role.key, k -> new ArrayList<>()); // new list just in case a role is also a tag + } + } + } + return tagMap; + } + /** * Constructs a new {@code TaggingPresetSelector}. * @param displayOnlyApplicable if {@code true} display "Show only applicable to selection" checkbox @@ -256,7 +369,7 @@ boolean inTags = ckSearchInTags != null && ckSearchInTags.isSelected(); DataSet ds = OsmDataManager.getInstance().getEditDataSet(); - Collection selected = (ds == null) ? Collections.emptyList() : ds.getSelected(); + Collection selected = (ds == null) ? Collections.emptyList() : ds.getSelected(); final List result = classifications.getMatchingPresets( text, onlyApplicable, inTags, getTypesInSelection(), selected); @@ -279,22 +392,41 @@ private final List classifications = new ArrayList<>(); + /** + * Get the matching presets given some queries + * @param searchText The search text to use. Split on {@code "/"} and whitespace. + * @param onlyApplicable {@code true} if only presets that match the {@code presetTypes} or {@code selectedPrimitives} are desired + * @param inTags Check if {@code nameWords} are in the tags of the preset + * @param presetTypes The preset types to filter on. See {@link TaggingPreset#typeMatches(Collection)}. + * @param selectedPrimitives The selected primitives to filter against (mostly for roles) + * @return The list of matching preset classifications + */ public List getMatchingPresets(String searchText, boolean onlyApplicable, boolean inTags, Set presetTypes, final Collection selectedPrimitives) { final String[] groupWords; final String[] nameWords; if (searchText.contains("/")) { - groupWords = searchText.substring(0, searchText.lastIndexOf('/')).split("[\\s/]", -1); - nameWords = searchText.substring(searchText.indexOf('/') + 1).split("\\s", -1); + groupWords = PATTERN_GROUP.split(searchText.substring(0, searchText.lastIndexOf('/')), -1); + nameWords = PATTERN_WORD.split(searchText.substring(searchText.indexOf('/') + 1), -1); } else { groupWords = null; - nameWords = searchText.split("\\s", -1); + nameWords = PATTERN_WORD.split(searchText, -1); } return getMatchingPresets(groupWords, nameWords, onlyApplicable, inTags, presetTypes, selectedPrimitives); } + /** + * Get the matching presets given some queries + * @param groupWords See {@link PresetClassification#isMatchingGroup(String...)} (may be {@code null}). + * @param nameWords The {@link PresetClassification} is checked to see if a name, group, or tags (if {@code inTags = true}) match + * @param onlyApplicable {@code true} if only presets that match the {@code presetTypes} or {@code selectedPrimitives} are desired + * @param inTags Check if {@code nameWords} are in the tags of the preset + * @param presetTypes The preset types to filter on. See {@link TaggingPreset#typeMatches(Collection)}. + * @param selectedPrimitives The selected primitives to filter against (mostly for roles) + * @return The list of matching preset classifications + */ public List getMatchingPresets(String[] groupWords, String[] nameWords, boolean onlyApplicable, boolean inTags, Set presetTypes, final Collection selectedPrimitives) {