Changeset 18254 in josm for trunk/src/org/openstreetmap

Timestamp:

2021-10-06T13:24:19+02:00 (3 years ago)

Author:

Don-vip

Message:

fix #21408 - fix #19013, fix #21385, fix #21404 - Fix MultiSelect issues (patch by marcello)

Location:

trunk/src/org/openstreetmap/josm/gui/tagging/presets

Files:

• TaggingPresetItemGuiSupport.java (modified) (2 diffs)
• items/Check.java (modified) (1 diff)
• items/Combo.java (modified) (9 diffs)
• items/ComboMultiSelect.java (modified) (12 diffs)
• items/KeyedItem.java (modified) (9 diffs)
• items/MultiSelect.java (modified) (2 diffs)
• items/PresetListEntry.java (modified) (6 diffs)
• items/Text.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetItemGuiSupport.java

@@ -25 +25 @@
 
     private final Collection<OsmPrimitive> selected;
+    /** True if all selected primitives matched this preset at the moment it was openend. */
     private final boolean presetInitiallyMatches;
     private final Supplier<Collection<Tag>> changedTagsSupplier;
@@ -84 +85 @@
 
     /**
-     * Returns whether the preset initially matched (before opening the dialog)
+     * Returns true if all selected primitives matched this preset (before opening the dialog)
      *
-     * @return whether the preset initially matched
+     * @return true if the preset initially matched
      */
     public boolean isPresetInitiallyMatches() {

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/Check.java

@@ -40 +40 @@
         // find out if our key is already used in the selection.
         final Usage usage = determineBooleanUsage(support.getSelected(), key);
-        final String oneValue = usage.values.isEmpty() ? null : usage.values.last();
+        final String oneValue = usage.map.isEmpty() ? null : usage.map.lastKey();
         def = "on".equals(default_) ? Boolean.TRUE : "off".equals(default_) ? Boolean.FALSE : null;
 
         initializeLocaleText(null);
 
-        if (usage.values.size() < 2 && (oneValue == null || value_on.equals(oneValue) || value_off.equals(oneValue))) {
+        if (usage.map.size() < 2 && (oneValue == null || value_on.equals(oneValue) || value_off.equals(oneValue))) {
             if (def != null && !PROP_FILL_DEFAULT.get()) {
                 // default is set and filling default values feature is disabled - check if all primitives are untagged

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/Combo.java

@@ -13 +13 @@
 import java.util.Arrays;
 import java.util.Comparator;
+import java.util.TreeMap;
 
 import javax.swing.AbstractAction;
@@ -18 +19 @@
 import javax.swing.JColorChooser;
 import javax.swing.JComponent;
+import javax.swing.JLabel;
 import javax.swing.JPanel;
 
@@ -30 +32 @@
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItemGuiSupport;
 import org.openstreetmap.josm.gui.widgets.JosmComboBox;
+import org.openstreetmap.josm.gui.widgets.OrientationAction;
 import org.openstreetmap.josm.tools.ColorHelper;
 import org.openstreetmap.josm.tools.GBC;
@@ -75 +78 @@
     }
 
+    private void addEntry(PresetListEntry entry) {
+        if (!seenValues.containsKey(entry.value)) {
+            dropDownModel.addElement(entry);
+            seenValues.put(entry.value, entry);
+        }
+    }
+
     @Override
-    protected JComponent addToPanelAnchor(JPanel p, String def, TaggingPresetItemGuiSupport support) {
-        if (!usage.unused()) {
-            for (String s : usage.values) {
-                presetListEntries.add(new PresetListEntry(s));
-            }
-        }
-        if (def != null) {
-            presetListEntries.add(new PresetListEntry(def));
-        }
-        presetListEntries.add(new PresetListEntry(""));
-
+    protected boolean addToPanel(JPanel p, TaggingPresetItemGuiSupport support) {
+        initializeLocaleText(null);
+        usage = determineTextUsage(support.getSelected(), key);
+        seenValues = new TreeMap<>();
+        // get the standard values from the preset definition
+        initListEntries();
+
+        // init the model
         dropDownModel = new AutoCompComboBoxModel<PresetListEntry>(Comparator.naturalOrder());
-        autoCompModel = new AutoCompComboBoxModel<AutoCompletionItem>(Comparator.naturalOrder());
-        presetListEntries.forEach(dropDownModel::addElement);
+
+        if (!usage.hasUniqueValue() && !usage.unused()) {
+            addEntry(PresetListEntry.ENTRY_DIFFERENT);
+        }
+        presetListEntries.forEach(this::addEntry);
+        if (default_ != null) {
+            addEntry(new PresetListEntry(default_, this));
+        }
+        addEntry(PresetListEntry.ENTRY_EMPTY);
+
+        usage.map.forEach((value, count) -> {
+            addEntry(new PresetListEntry(value, this));
+        });
 
         combobox = new JosmComboBox<>(dropDownModel);
@@ -98 +116 @@
         // the dropdown list.  We don't want that to happen because we want to show taller items in
         // the list than in the editor.  We can't use
-        // {@code combobox.setPrototypeDisplayValue(new PresetListEntry(" "));} because that would
+        // {@code combobox.setPrototypeDisplayValue(PresetListEntry.ENTRY_EMPTY);} because that would
         // set a fixed cell height in JList.
         combobox.setPreferredHeight(combobox.getPreferredSize().height);
@@ -107 +125 @@
         combobox.setEditable(editable);
 
+        autoCompModel = new AutoCompComboBoxModel<AutoCompletionItem>(Comparator.naturalOrder());
         getAllForKeys(Arrays.asList(key)).forEach(autoCompModel::addElement);
         getDisplayValues().forEach(s -> autoCompModel.addElement(new AutoCompletionItem(s, AutoCompletionPriority.IS_IN_STANDARD)));
@@ -120 +139 @@
         }
 
+        JLabel label = addLabel(p);
+
         if (key != null && ("colour".equals(key) || key.startsWith("colour:") || key.endsWith(":colour"))) {
-            p.add(combobox, GBC.std().fill(GBC.HORIZONTAL));
+            p.add(combobox, GBC.std().fill(GBC.HORIZONTAL)); // NOSONAR
             JButton button = new JButton(new ChooseColorAction());
             button.setOpaque(true);
             button.setBorderPainted(false);
             button.setCursor(Cursor.getPredefinedCursor(Cursor.HAND_CURSOR));
-            p.add(button, GBC.eol().fill(GBC.VERTICAL));
+            p.add(button, GBC.eol().fill(GBC.VERTICAL)); // NOSONAR
             ActionListener updateColor = ignore -> button.setBackground(getColor());
             updateColor.actionPerformed(null);
             combobox.addActionListener(updateColor);
         } else {
-            p.add(combobox, GBC.eol().fill(GBC.HORIZONTAL));
-        }
-
-        Object itemToSelect = getItemToSelect(default_, support, false);
-        combobox.setSelectedItemText(itemToSelect == null ? null : itemToSelect.toString());
-        combobox.addActionListener(l -> support.fireItemValueModified(this, key, getSelectedValue()));
+            p.add(combobox, GBC.eol().fill(GBC.HORIZONTAL)); // NOSONAR
+        }
+
+        String valueToSelect = getInitialValue(default_);
+        if (valueToSelect != null) {
+            PresetListEntry selItem = find(valueToSelect);
+            if (selItem != null) {
+                combobox.setSelectedItem(selItem);
+            } else {
+                combobox.setText(valueToSelect);
+            }
+        }
+        combobox.addActionListener(l -> support.fireItemValueModified(this, key, getSelectedItem().value));
         combobox.addComponentListener(new ComponentListener());
-        return combobox;
+
+        label.setLabelFor(combobox);
+        combobox.setToolTipText(getKeyTooltipText());
+        combobox.applyComponentOrientation(OrientationAction.getValueOrientation(key));
+
+        return true;
+    }
+
+    /**
+     * Finds the PresetListEntry that matches value.
+     * <p>
+     * Looks in the model for an element whose {@code value} matches {@code value}.
+     *
+     * @param value The value to match.
+     * @return The entry or null
+     */
+    private PresetListEntry find(String value) {
+        return dropDownModel.asCollection().stream().filter(o -> o.value.equals(value)).findAny().orElse(null);
     }
 
@@ -161 +206 @@
 
     protected Color getColor() {
-        String colorString = String.valueOf(getSelectedValue());
+        String colorString = getSelectedItem().value;
         return colorString.startsWith("#")
                 ? ColorHelper.html2color(colorString)
@@ -168 +213 @@
 
     @Override
-    protected Object getSelectedItem() {
-        return combobox.getSelectedItem();
-    }
-
-    @Override
-    protected String getDisplayIfNull() {
-        if (combobox.isEditable())
-            return combobox.getEditor().getItem().toString();
-        else
-            return null;
+    protected PresetListEntry getSelectedItem() {
+        Object sel = combobox.getSelectedItem();
+        if (sel instanceof PresetListEntry)
+            // selected from the dropdown
+            return (PresetListEntry) sel;
+        if (sel instanceof String) {
+            // free edit.  If the free edit corresponds to a known entry, use that entry.  This is
+            // to avoid that we write a display_value to the tag's value, eg. if the user did an
+            // undo.
+            PresetListEntry selItem = dropDownModel.find((String) sel);
+            if (selItem != null)
+                return selItem;
+            return new PresetListEntry((String) sel, this);
+        }
+        return PresetListEntry.ENTRY_EMPTY;
     }
 }

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/ComboMultiSelect.java

@@ -3 +3 @@
 
 import static org.openstreetmap.josm.tools.I18n.tr;
-import static org.openstreetmap.josm.tools.I18n.trc;
 
 import java.awt.Component;
@@ -13 +12 @@
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.Map;
+import java.util.TreeMap;
 import java.util.stream.Collectors;
 
-import javax.swing.JComponent;
 import javax.swing.JLabel;
 import javax.swing.JList;
@@ -25 +22 @@
 
 import org.openstreetmap.josm.data.osm.Tag;
-import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItemGuiSupport;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetSelector;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresets;
 import org.openstreetmap.josm.gui.widgets.JosmListCellRenderer;
 import org.openstreetmap.josm.gui.widgets.OrientationAction;
+import org.openstreetmap.josm.tools.AlphanumComparator;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.Logging;
-import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -89 +85 @@
     public boolean values_searchable; // NOSONAR
 
-    protected final Set<PresetListEntry> presetListEntries = new CopyOnWriteArraySet<>();
-    private boolean initialized;
+    /**
+     * The standard entries in the combobox dropdown or multiselect list. These entries are defined
+     * in {@code defaultpresets.xml} (or in other custom preset files).
+     */
+    protected final List<PresetListEntry> presetListEntries = new ArrayList<>();
+    /** Helps avoid duplicate list entries */
+    protected Map<String, PresetListEntry> seenValues = new TreeMap<>();
     protected Usage usage;
-    protected Object originalValue;
+    /** Used to see if the user edited the value. May be null. */
+    protected String originalValue;
 
     /**
@@ -134 +136 @@
                 l.setText(value.getListDisplay(width));
             }
-            String tt = value.value;
-            if (tt != null && !tt.isEmpty()) {
-                l.setToolTipText(tr("Sets the key ''{0}'' to the value ''{1}''.", key, tt));
-            } else {
-                l.setToolTipText(tr("Clears the key ''{0}''.", key));
-            }
+            l.setToolTipText(value.getToolTipText(key));
             l.setIcon(value.getIcon());
             return l;
@@ -153 +150 @@
      * @return splitted items
      */
-    public static String[] splitEscaped(char delimiter, String s) {
+    public static List<String> splitEscaped(char delimiter, String s) {
         if (s == null)
-            return new String[0];
+            return null; // NOSONAR
+
         List<String> result = new ArrayList<>();
         boolean backslash = false;
@@ -176 +174 @@
             result.add(item.toString());
         }
-        return result.toArray(new String[0]);
-    }
-
-    protected abstract Object getSelectedItem();
-
-    protected abstract JComponent addToPanelAnchor(JPanel p, String def, TaggingPresetItemGuiSupport support);
+        return result;
+    }
+
+    /**
+     * Returns the value selected in the combobox or a synthetic value if a multiselect.
+     *
+     * @return the value
+     */
+    protected abstract PresetListEntry getSelectedItem();
 
     @Override
     public Collection<String> getValues() {
-        initListEntries(false);
+        initListEntries();
         return presetListEntries.stream().map(x -> x.value).collect(Collectors.toSet());
     }
@@ -194 +195 @@
      */
     public Collection<String> getDisplayValues() {
-        initListEntries(false);
+        initListEntries();
         return presetListEntries.stream().map(PresetListEntry::getDisplayValue).collect(Collectors.toList());
     }
 
-    @Override
-    public boolean addToPanel(JPanel p, TaggingPresetItemGuiSupport support) {
-        initListEntries(true);
-
-        // find out if our key is already used in the selection.
-        usage = determineTextUsage(support.getSelected(), key);
-        if (!usage.hasUniqueValue() && !usage.unused()) {
-            presetListEntries.add(new PresetListEntry(DIFFERENT));
-        }
-
+    /**
+     * Adds the label to the panel
+     *
+     * @param p the panel
+     * @return the label
+     */
+    protected JLabel addLabel(JPanel p) {
         final JLabel label = new JLabel(tr("{0}:", locale_text));
         addIcon(label);
@@ -214 +212 @@
         label.applyComponentOrientation(OrientationAction.getDefaultComponentOrientation());
         p.add(label, GBC.std().insets(0, 0, 10, 0));
-        JComponent component = addToPanelAnchor(p, default_, support);
-        label.setLabelFor(component);
-        component.setToolTipText(getKeyTooltipText());
-        component.applyComponentOrientation(OrientationAction.getValueOrientation(key));
-
-        return true;
-    }
-
-    private void initListEntries(boolean cleanup) {
-        if (initialized) {
-            if (cleanup) { // do not cleanup for #getDisplayValues used in Combo#addToPanelAnchor
-                presetListEntries.remove(new PresetListEntry(DIFFERENT)); // possibly added in #addToPanel
+        return label;
+    }
+
+    protected void initListEntries() {
+        if (presetListEntries.isEmpty()) {
+            initListEntriesFromAttributes();
+        }
+    }
+
+    private List<String> getValuesFromCode(String values_from) {
+        // get the values from a Java function
+        String[] classMethod = values_from.split("#", -1);
+        if (classMethod.length == 2) {
+            try {
+                Method method = Class.forName(classMethod[0]).getMethod(classMethod[1]);
+                // Check method is public static String[] methodName()
+                int mod = method.getModifiers();
+                if (Modifier.isPublic(mod) && Modifier.isStatic(mod)
+                        && method.getReturnType().equals(String[].class) && method.getParameterTypes().length == 0) {
+                    return Arrays.asList((String[]) method.invoke(null));
+                } else {
+                    Logging.error(tr("Broken tagging preset \"{0}-{1}\" - Java method given in ''values_from'' is not \"{2}\"", key, text,
+                            "public static String[] methodName()"));
+                }
+            } catch (ReflectiveOperationException e) {
+                Logging.error(tr("Broken tagging preset \"{0}-{1}\" - Java method given in ''values_from'' threw {2} ({3})", key, text,
+                        e.getClass().getName(), e.getMessage()));
+                Logging.debug(e);
             }
+        }
+        return null; // NOSONAR
+    }
+
+    /**
+     * Checks if list {@code a} is either null or the same length as list {@code b}.
+     *
+     * @param a The list to check
+     * @param b The other list
+     * @param name The name of the list for error reporting
+     * @return {@code a} if both lists have the same length or {@code null}
+     */
+    private List<String> checkListsSameLength(List<String> a, List<String> b, String name) {
+        if (a != null && a.size() != b.size()) {
+            Logging.error(tr("Broken tagging preset \"{0}-{1}\" - number of items in ''{2}List'' must be the same as in ''values''",
+                            key, text, name));
+            Logging.error(tr("Detailed information: {0} <> {1}", a, b));
+            return null; // NOSONAR
+        }
+        return a;
+    }
+
+    protected void initListEntriesFromAttributes() {
+        List<String> valueList = null;
+        List<String> displayList = null;
+        List<String> localeDisplayList = null;
+
+        if (values_from != null) {
+            valueList = getValuesFromCode(values_from);
+        }
+
+        if (valueList == null) {
+            // get from {@code values} attribute
+            valueList = splitEscaped(delimiter, values);
+        }
+        if (valueList == null) {
             return;
-        } else if (presetListEntries.isEmpty()) {
-            initListEntriesFromAttributes();
-        } else {
-            if (values != null) {
-                Logging.warn(tr("Warning in tagging preset \"{0}-{1}\": "
-                        + "Ignoring ''{2}'' attribute as ''{3}'' elements are given.",
-                        key, text, "values", "list_entry"));
-            }
-            if (display_values != null || locale_display_values != null) {
-                Logging.warn(tr("Warning in tagging preset \"{0}-{1}\": "
-                        + "Ignoring ''{2}'' attribute as ''{3}'' elements are given.",
-                        key, text, "display_values", "list_entry"));
-            }
-            if (short_descriptions != null || locale_short_descriptions != null) {
-                Logging.warn(tr("Warning in tagging preset \"{0}-{1}\": "
-                        + "Ignoring ''{2}'' attribute as ''{3}'' elements are given.",
-                        key, text, "short_descriptions", "list_entry"));
-            }
-            for (PresetListEntry e : presetListEntries) {
-                if (e.value_context == null) {
-                    e.value_context = values_context;
-                }
-            }
-        }
-        initializeLocaleText(null);
-        initialized = true;
-    }
-
-    private void initListEntriesFromAttributes() {
-
-        String[] valueArray = null;
-
-        if (values_from != null) {
-            String[] classMethod = values_from.split("#", -1);
-            if (classMethod.length == 2) {
-                try {
-                    Method method = Class.forName(classMethod[0]).getMethod(classMethod[1]);
-                    // Check method is public static String[] methodName()
-                    int mod = method.getModifiers();
-                    if (Modifier.isPublic(mod) && Modifier.isStatic(mod)
-                            && method.getReturnType().equals(String[].class) && method.getParameterTypes().length == 0) {
-                        valueArray = (String[]) method.invoke(null);
-                    } else {
-                        Logging.error(tr("Broken tagging preset \"{0}-{1}\" - Java method given in ''values_from'' is not \"{2}\"", key, text,
-                                "public static String[] methodName()"));
-                    }
-                } catch (ReflectiveOperationException e) {
-                    Logging.error(tr("Broken tagging preset \"{0}-{1}\" - Java method given in ''values_from'' threw {2} ({3})", key, text,
-                            e.getClass().getName(), e.getMessage()));
-                    Logging.debug(e);
-                }
-            }
-        }
-
-        if (valueArray == null) {
-            valueArray = splitEscaped(delimiter, values);
-            values = null;
-        }
-
-        String[] displayArray = valueArray;
+        }
+
         if (!values_no_i18n) {
-            final String displ = Utils.firstNonNull(locale_display_values, display_values);
-            displayArray = displ == null ? valueArray : splitEscaped(delimiter, displ);
-        }
-
-        final String descr = Utils.firstNonNull(locale_short_descriptions, short_descriptions);
-        String[] shortDescriptionsArray = descr == null ? null : splitEscaped(delimiter, descr);
-
-        if (displayArray.length != valueArray.length) {
-            Logging.error(tr("Broken tagging preset \"{0}-{1}\" - number of items in ''display_values'' must be the same as in ''values''",
-                            key, text));
-            Logging.error(tr("Detailed information: {0} <> {1}", Arrays.toString(displayArray), Arrays.toString(valueArray)));
-            displayArray = valueArray;
-        }
-
-        if (shortDescriptionsArray != null && shortDescriptionsArray.length != valueArray.length) {
-            Logging.error(tr("Broken tagging preset \"{0}-{1}\" - number of items in ''short_descriptions'' must be the same as in ''values''",
-                            key, text));
-            Logging.error(tr("Detailed information: {0} <> {1}", Arrays.toString(shortDescriptionsArray), Arrays.toString(valueArray)));
-            shortDescriptionsArray = null;
-        }
-
-        final List<PresetListEntry> entries = new ArrayList<>(valueArray.length);
-        for (int i = 0; i < valueArray.length; i++) {
-            final PresetListEntry e = new PresetListEntry(valueArray[i]);
-            final String value = locale_display_values != null || values_no_i18n
-                    ? displayArray[i]
-                    : trc(values_context, fixPresetString(displayArray[i]));
-            e.locale_display_value = value == null ? null : value.intern();
-            if (shortDescriptionsArray != null) {
-                final String description = locale_short_descriptions != null
-                        ? shortDescriptionsArray[i]
-                        : tr(fixPresetString(shortDescriptionsArray[i]));
-                e.locale_short_description = description == null ? null : description.intern();
-            }
-
-            entries.add(e);
+            localeDisplayList = splitEscaped(delimiter, locale_display_values);
+            displayList = splitEscaped(delimiter, display_values);
+        }
+        List<String> localeShortDescriptionsList = splitEscaped(delimiter, locale_short_descriptions);
+        List<String> shortDescriptionsList = splitEscaped(delimiter, short_descriptions);
+
+        displayList = checkListsSameLength(displayList, valueList, "display");
+        localeDisplayList = checkListsSameLength(localeDisplayList, valueList, "localeDisplay");
+        shortDescriptionsList = checkListsSameLength(shortDescriptionsList, valueList, "shortDescriptions");
+        localeShortDescriptionsList = checkListsSameLength(localeShortDescriptionsList, valueList, "localeShortDescriptions");
+
+        for (int i = 0; i < valueList.size(); i++) {
+            final PresetListEntry e = new PresetListEntry(valueList.get(i), this);
+            if (displayList != null)
+                e.display_value = displayList.get(i);
+            if (localeDisplayList != null)
+                e.locale_display_value = localeDisplayList.get(i);
+            if (shortDescriptionsList != null)
+                e.short_description = shortDescriptionsList.get(i);
+            if (localeShortDescriptionsList != null)
+                e.locale_short_description = localeShortDescriptionsList.get(i);
+            addListEntry(e);
         }
 
         if (values_sort && TaggingPresets.SORT_MENU.get()) {
-            Collections.sort(entries);
-        }
-
-        addListEntries(entries);
-    }
-
-    protected String getDisplayIfNull() {
-        return null;
-    }
-
-    protected Object getItemToSelect(String def, TaggingPresetItemGuiSupport support, boolean multi) {
-        final Object itemToSelect;
+            Collections.sort(presetListEntries, (a, b) -> AlphanumComparator.getInstance().compare(a.getDisplayValue(), b.getDisplayValue()));
+        }
+    }
+
+    /**
+     * Returns the initial value to use for this preset.
+     * <p>
+     * The initial value is the value shown in the control when the preset dialogs opens.
+     *
+     * @param def The default value
+     * @return The initial value to use.
+     */
+    protected String getInitialValue(String def) {
+        String initialValue = null;
+
         if (usage.hasUniqueValue()) {
-            // all items have the same value (and there were no unset items)
-            originalValue = multi ? usage.getFirst() : getListEntry(usage.getFirst());
-            itemToSelect = originalValue;
-        } else if (def != null && usage.unused()) {
-            // default is set and all items were unset
-            if (!usage.hadKeys() || PROP_FILL_DEFAULT.get() || isForceUseLastAsDefault()) {
-                // selected osm primitives are untagged or filling default feature is enabled
-                if (multi) {
-                    itemToSelect = def;
-                } else {
-                    PresetListEntry entry = getListEntry(def);
-                    itemToSelect = entry == null ? "" : entry.getDisplayValue();
-                }
-            } else {
-                // selected osm primitives are tagged and filling default feature is disabled
-                itemToSelect = "";
-            }
-            originalValue = multi ? DIFFERENT : getListEntry(DIFFERENT);
-        } else if (usage.unused()) {
-            // all items were unset (and so is default)
-            originalValue = multi ? null : getListEntry("");
-            if (LAST_VALUES.containsKey(key) && isUseLastAsDefault() && (!support.isPresetInitiallyMatches() || isForceUseLastAsDefault())) {
-                itemToSelect = getListEntry(LAST_VALUES.get(key));
-            } else {
-                itemToSelect = originalValue;
-            }
-        } else {
-            originalValue = multi ? DIFFERENT : getListEntry(DIFFERENT);
-            itemToSelect = originalValue;
-        }
-        return itemToSelect;
-    }
-
-    protected String getSelectedValue() {
-        Object obj = getSelectedItem();
-        String display = obj == null ? getDisplayIfNull() : obj.toString();
-
-        if (display == null) {
-            return "";
-        }
-        return presetListEntries.stream()
-                .filter(entry -> Objects.equals(entry.toString(), display))
-                .findFirst()
-                .map(entry -> entry.value)
-                .map(Utils::removeWhiteSpaces)
-                .orElse(display);
+            // all selected primitives have the same not empty value for this key
+            initialValue = usage.getFirst();
+        } else if (!usage.unused()) {
+            // at least one primitive has a value for this key (but not all have the same one)
+            initialValue = DIFFERENT;
+        } else if (PROP_FILL_DEFAULT.get() || isForceUseLastAsDefault()) {
+            // at this point no primitive had any value for this key
+            // use the last value no matter what
+            initialValue = LAST_VALUES.get(key);
+        } else if (!usage.hadKeys() && isUseLastAsDefault()) {
+            // use the last value only on objects with no keys at all
+            initialValue = LAST_VALUES.get(key);
+        } else if (!usage.hadKeys()) {
+            // use the default only on objects with no keys at all
+            initialValue = def;
+        }
+        originalValue = initialValue;
+        return initialValue;
     }
 
     @Override
     public void addCommands(List<Tag> changedTags) {
-        String value = getSelectedValue();
+        String value = getSelectedItem().value;
 
         // no change if same as before
-        if (originalValue == null) {
-            if (value.isEmpty())
-                return;
-        } else if (value.equals(originalValue.toString()))
+        if (value.isEmpty() && originalValue == null)
             return;
+        if (value.equals(originalValue))
+            return;
+        changedTags.add(new Tag(key, value));
 
         if (isUseLastAsDefault()) {
             LAST_VALUES.put(key, value);
         }
-        changedTags.add(new Tag(key, value));
     }
 
@@ -420 +372 @@
     }
 
+    /**
+     * Returns true if the last entered value should be used as default.
+     * <p>
+     * Note: never used in {@code defaultpresets.xml}.
+     *
+     * @return true if the last entered value should be used as default.
+     */
     protected boolean isUseLastAsDefault() {
         return use_last_as_default > 0;
     }
 
+    /**
+     * Returns true if the last entered value should be used as default also on primitives that
+     * already have tags.
+     * <p>
+     * Note: used for {@code addr:*} tags in {@code defaultpresets.xml}.
+     *
+     * @return true if see above
+     */
     protected boolean isForceUseLastAsDefault() {
         return use_last_as_default == 2;
@@ -434 +401 @@
     public void addListEntry(PresetListEntry e) {
         presetListEntries.add(e);
+        // we need to fix the entries because the XML Parser
+        // {@link org.openstreetmap.josm.tools.XmlObjectParser.Parser#startElement} has used the
+        // default standard constructor for {@link PresetListEntry} if the list entry was defined
+        // using XML {@code <list_entry>}.
+        e.cms = this;
     }
 
@@ -446 +418 @@
     }
 
-    protected PresetListEntry getListEntry(String value) {
-        return presetListEntries.stream().filter(e -> Objects.equals(e.value, value)).findFirst().orElse(null);
-    }
-
     @Override
     public MatchType getDefaultMatch() {

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/KeyedItem.java

@@ -8 +8 @@
 import java.util.HashMap;
 import java.util.Map;
+import java.util.SortedMap;
 import java.util.NoSuchElementException;
-import java.util.SortedSet;
-import java.util.TreeSet;
+import java.util.TreeMap;
 
 import javax.swing.JPopupMenu;
@@ -27 +27 @@
 public abstract class KeyedItem extends TextItem {
 
-    /** Translation of "<different>". Use in combo boxes to display an entry matching several different values. */
-    protected static final String DIFFERENT = tr("<different>");
-
+    /** The constant value {@code "<different>"}. */
+    protected static final String DIFFERENT = "<different>";
+    /** Translation of {@code "<different>"}. */
+    protected static final String DIFFERENT_I18N = tr(DIFFERENT);
+
+    /** True if the default value should also be set on primitives that already have tags.  */
     protected static final BooleanProperty PROP_FILL_DEFAULT = new BooleanProperty("taggingpreset.fill-default-for-tagged-primitives", false);
 
@@ -100 +103 @@
      */
     public static class Usage {
-        /**
-         * A set of values that were used for this key.
-         */
-        public final SortedSet<String> values = new TreeSet<>(); // NOSONAR
+        /** Usage count for all values used for this key */
+        public final SortedMap<String, Integer> map = new TreeMap<>();
         private boolean hadKeys;
         private boolean hadEmpty;
@@ -112 +113 @@
          */
         public boolean hasUniqueValue() {
-            return values.size() == 1 && !hadEmpty;
+            return map.size() == 1 && !hadEmpty;
         }
 
@@ -120 +121 @@
          */
         public boolean unused() {
-            return values.isEmpty();
+            return map.isEmpty();
         }
 
@@ -129 +130 @@
          */
         public String getFirst() {
-            return values.first();
+            return map.firstKey();
         }
 
@@ -152 +153 @@
             String v = s.get(key);
             if (v != null) {
-                returnValue.values.add(v);
+                returnValue.map.merge(v, 1, Integer::sum);
             } else {
                 returnValue.hadEmpty = true;
@@ -168 +169 @@
             String booleanValue = OsmUtils.getNamedOsmBoolean(s.get(key));
             if (booleanValue != null) {
-                returnValue.values.add(booleanValue);
+                returnValue.map.merge(booleanValue, 1, Integer::sum);
             }
         }
@@ -203 +204 @@
         switch (MatchType.ofString(match)) {
         case NONE:
-            return null;
+            return null; // NOSONAR
         case KEY:
             return tags.containsKey(key) ? Boolean.TRUE : null;

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/MultiSelect.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.tagging.presets.items;
 
-import java.awt.Dimension;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Set;
-import java.util.TreeSet;
+import java.util.TreeMap;
+import java.util.stream.Collectors;
 
-import javax.swing.JComponent;
+import javax.swing.DefaultListModel;
+import javax.swing.JLabel;
 import javax.swing.JList;
 import javax.swing.JPanel;
 import javax.swing.JScrollPane;
-import javax.swing.ListModel;
 
-import org.openstreetmap.josm.data.osm.Tag;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItemGuiSupport;
+import org.openstreetmap.josm.gui.widgets.OrientationAction;
 import org.openstreetmap.josm.tools.GBC;
 
@@ -28 +25 @@
     public short rows; // NOSONAR
 
-    protected ConcatenatingJList list;
+    /** The model for the JList */
+    protected final DefaultListModel<PresetListEntry> model = new DefaultListModel<>();
+    /** The swing component */
+    protected final JList<PresetListEntry> list = new JList<>(model);
 
-    @Override
-    protected JComponent addToPanelAnchor(JPanel p, String def, TaggingPresetItemGuiSupport support) {
-        list = new ConcatenatingJList(delimiter, presetListEntries.toArray(new PresetListEntry[0]));
-        ComboMultiSelectListCellRenderer renderer = new ComboMultiSelectListCellRenderer(list, list.getCellRenderer(), 200, key);
-        list.setCellRenderer(renderer);
-        Object itemToSelect = getItemToSelect(def, support, true);
-        list.setSelectedItem(itemToSelect == null ? null : new PresetListEntry(itemToSelect.toString()));
-        JScrollPane sp = new JScrollPane(list);
-        // if a number of rows has been specified in the preset,
-        // modify preferred height of scroll pane to match that row count.
-        if (rows > 0) {
-            double height = renderer.getListCellRendererComponent(list,
-                    new PresetListEntry("x"), 0, false, false).getPreferredSize().getHeight() * rows;
-            sp.setPreferredSize(new Dimension((int) sp.getPreferredSize().getWidth(), (int) height));
+    private void addEntry(PresetListEntry entry) {
+        if (!seenValues.containsKey(entry.value)) {
+            model.addElement(entry);
+            seenValues.put(entry.value, entry);
         }
-        list.addListSelectionListener(l -> support.fireItemValueModified(this, key, getSelectedValue()));
-        p.add(sp, GBC.eol().fill(GBC.HORIZONTAL));
-        return list;
     }
 
     @Override
-    protected Object getSelectedItem() {
-        return list.getSelectedItem();
+    protected boolean addToPanel(JPanel p, TaggingPresetItemGuiSupport support) {
+        initializeLocaleText(null);
+        usage = determineTextUsage(support.getSelected(), key);
+        seenValues = new TreeMap<>();
+        initListEntries();
+
+        model.clear();
+        if (!usage.hasUniqueValue() && !usage.unused()) {
+            addEntry(PresetListEntry.ENTRY_DIFFERENT);
+        }
+
+        String initialValue = getInitialValue(default_);
+        if (initialValue != null) {
+            // add all values already present to the list, otherwise we would remove all
+            // custom entries unknown to the preset
+            for (String value : initialValue.split(String.valueOf(delimiter), -1)) {
+                PresetListEntry e = new PresetListEntry(value, this);
+                addEntry(e);
+                int i = model.indexOf(e);
+                list.addSelectionInterval(i, i);
+            }
+        }
+
+        presetListEntries.forEach(this::addEntry);
+
+        ComboMultiSelectListCellRenderer renderer = new ComboMultiSelectListCellRenderer(list, list.getCellRenderer(), 200, key);
+        list.setCellRenderer(renderer);
+
+        if (rows > 0) {
+            list.setVisibleRowCount(rows);
+        }
+        JLabel label = addLabel(p);
+        p.add(new JScrollPane(list), GBC.eol().fill(GBC.HORIZONTAL)); // NOSONAR
+        label.setLabelFor(list);
+
+        list.addListSelectionListener(l -> support.fireItemValueModified(this, key, getSelectedItem().value));
+        list.setToolTipText(getKeyTooltipText());
+        list.applyComponentOrientation(OrientationAction.getValueOrientation(key));
+
+        return true;
     }
 
     @Override
-    public void addCommands(List<Tag> changedTags) {
-        // Do not create any commands if list has been disabled because of an unknown value (fix #8605)
-        if (list.isEnabled()) {
-            super.addCommands(changedTags);
-        }
-    }
-
-    /**
-     * Class that allows list values to be assigned and retrieved as a comma-delimited
-     * string (extracted from TaggingPreset)
-     */
-    private static class ConcatenatingJList extends JList<PresetListEntry> {
-        private final char delimiter;
-
-        protected ConcatenatingJList(char del, PresetListEntry... o) {
-            super(o);
-            delimiter = del;
-        }
-
-        public void setSelectedItem(Object o) {
-            if (o == null) {
-                clearSelection();
-            } else {
-                String s = o.toString();
-                Set<String> parts = new TreeSet<>(Arrays.asList(s.split(String.valueOf(delimiter), -1)));
-                ListModel<PresetListEntry> lm = getModel();
-                int[] intParts = new int[lm.getSize()];
-                int j = 0;
-                for (int i = 0; i < lm.getSize(); i++) {
-                    final String value = lm.getElementAt(i).value;
-                    if (parts.contains(value)) {
-                        intParts[j++] = i;
-                        parts.remove(value);
-                    }
-                }
-                setSelectedIndices(Arrays.copyOf(intParts, j));
-                // check if we have actually managed to represent the full
-                // value with our presets. if not, cop out; we will not offer
-                // a selection list that threatens to ruin the value.
-                setEnabled(parts.isEmpty());
-            }
-        }
-
-        public String getSelectedItem() {
-            ListModel<PresetListEntry> lm = getModel();
-            int[] si = getSelectedIndices();
-            StringBuilder builder = new StringBuilder();
-            for (int i = 0; i < si.length; i++) {
-                if (i > 0) {
-                    builder.append(delimiter);
-                }
-                builder.append(lm.getElementAt(si[i]).value);
-            }
-            return builder.toString();
-        }
+    protected PresetListEntry getSelectedItem() {
+        return new PresetListEntry(list.getSelectedValuesList()
+            .stream().map(e -> e.value).collect(Collectors.joining(String.valueOf(delimiter))), this);
     }
 }

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/PresetListEntry.java

@@ -17 +17 @@
 /**
  * Preset list entry.
+ * <p>
+ * Used for controls that offer a list of items to choose from like {@link Combo} and
+ * {@link MultiSelect}.
  */
 public class PresetListEntry implements Comparable<PresetListEntry> {
-    /** Entry value */
+    /** Used to display an entry matching several different values. */
+    protected static final PresetListEntry ENTRY_DIFFERENT = new PresetListEntry(KeyedItem.DIFFERENT, null);
+    /** Used to display an empty entry used to clear values. */
+    protected static final PresetListEntry ENTRY_EMPTY = new PresetListEntry("", null);
+
+    /**
+     * This is the value that is going to be written to the tag on the selected primitive(s). Except
+     * when the value is {@code "<different>"}, which is never written, or the value is empty, which
+     * deletes the tag.  {@code value} is never translated.
+     */
     public String value; // NOSONAR
-    /** The context used for translating {@link #value} */
-    public String value_context; // NOSONAR
-    /** Value displayed to the user */
+    /** The ComboMultiSelect that displays the list */
+    public ComboMultiSelect cms; // NOSONAR
+    /** Text displayed to the user instead of {@link #value}. */
     public String display_value; // NOSONAR
-    /** Text to be displayed below {@code display_value}. */
+    /** Text to be displayed below {@link #display_value} in the combobox list. */
     public String short_description; // NOSONAR
     /** The location of icon file to display */
@@ -36 +48 @@
     public String locale_short_description; // NOSONAR
 
+    private String cachedDisplayValue = null;
+    private String cachedShortDescription = null;
+    private ImageIcon cachedIcon = null;
+
     /**
      * Constructs a new {@code PresetListEntry}, uninitialized.
+     *
+     * Public default constructor is needed by {@link org.openstreetmap.josm.tools.XmlObjectParser.Parser#startElement}
      */
     public PresetListEntry() {
-        // Public default constructor is needed
     }
 
     /**
-     * Constructs a new {@code PresetListEntry}, initialized with a value.
+     * Constructs a new {@code PresetListEntry}, initialized with a value and
+     * {@link ComboMultiSelect} context.
+     *
      * @param value value
+     * @param cms the ComboMultiSelect
      */
-    public PresetListEntry(String value) {
+    public PresetListEntry(String value, ComboMultiSelect cms) {
         this.value = value;
+        this.cms = cms;
     }
 
@@ -61 +82 @@
      */
     public String getListDisplay(int width) {
-        if (value.equals(KeyedItem.DIFFERENT)) {
-            return "<b>" + KeyedItem.DIFFERENT + "</b>";
+        String displayValue = getDisplayValue();
+        Integer count = cms == null ? null : cms.usage.map.get(value);
+
+        if (count != null) {
+            displayValue = String.format("%s (%d)", displayValue, count);
         }
 
-        String shortDescription = getShortDescription(true);
-        String displayValue = getDisplayValue();
+        if (this.equals(ENTRY_DIFFERENT)) {
+            return "<html><b>" + Utils.escapeReservedCharactersHTML(displayValue) + "</b></html>";
+        }
+
+        String shortDescription = getShortDescription();
 
         if (shortDescription.isEmpty()) {
+            // avoids a collapsed list entry if value == ""
             if (displayValue.isEmpty()) {
                 return " ";
@@ -87 +115 @@
      */
     public ImageIcon getIcon() {
-        return icon == null ? null : TaggingPresetItem.loadImageIcon(icon, TaggingPresetReader.getZipIcons(), (int) icon_size);
+        if (icon != null && cachedIcon == null) {
+            cachedIcon = TaggingPresetItem.loadImageIcon(icon, TaggingPresetReader.getZipIcons(), (int) icon_size);
+        }
+        return cachedIcon;
     }
 
     /**
-     * Returns the contents of the current item view.
+     * Returns the contents displayed in the current item view.
      * @return the value to display
      */
     public String getDisplayValue() {
-        return Utils.firstNonNull(locale_display_value, tr(display_value), trc(value_context, value));
+        if (cachedDisplayValue == null) {
+            if (cms != null && cms.values_no_i18n) {
+                cachedDisplayValue = Utils.firstNonNull(value, " ");
+            } else {
+                cachedDisplayValue = Utils.firstNonNull(
+                    locale_display_value, tr(display_value), trc(cms == null ? null : cms.values_context, value), " ");
+            }
+        }
+        return cachedDisplayValue;
     }
 
     /**
      * Returns the short description to display.
-     * @param translated whether the text must be translated
      * @return the short description to display
      */
-    public String getShortDescription(boolean translated) {
-        String shortDesc = translated
-                ? Utils.firstNonNull(locale_short_description, tr(short_description))
-                        : short_description;
-        return shortDesc == null ? "" : shortDesc;
+    public String getShortDescription() {
+        if (cachedShortDescription == null) {
+            cachedShortDescription = Utils.firstNonNull(locale_short_description, tr(short_description), "");
+        }
+        return cachedShortDescription;
+    }
+
+    /**
+     * Returns the tooltip for this entry.
+     * @param key the tag key
+     * @return the tooltip
+     */
+    public String getToolTipText(String key) {
+        if (this.equals(ENTRY_DIFFERENT)) {
+            return tr("Keeps the original values of the selected objects unchanged.");
+        }
+        if (value != null && !value.isEmpty()) {
+            return tr("Sets the key ''{0}'' to the value ''{1}''.", key, value);
+        }
+        return tr("Clears the key ''{0}''.", key);
     }
 
@@ -113 +166 @@
     @Override
     public String toString() {
-        if (KeyedItem.DIFFERENT.equals(value))
-            return KeyedItem.DIFFERENT;
-        String displayValue = getDisplayValue();
-        return displayValue != null ? displayValue.replaceAll("\\s*<.*>\\s*", " ") : ""; // remove additional markup, e.g. <br>
+        if (this.equals(ENTRY_DIFFERENT))
+            return getDisplayValue();
+        return getDisplayValue().replaceAll("\\s*<.*>\\s*", " "); // remove additional markup, e.g. <br>
     }
 
@@ -134 +186 @@
     @Override
     public int compareTo(PresetListEntry o) {
-        return AlphanumComparator.getInstance().compare(this.getDisplayValue(), o.getDisplayValue());
+        return AlphanumComparator.getInstance().compare(this.value, o.value);
     }
 }

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/Text.java

@@ -138 +138 @@
             // cannot be an AutoCompComboBox because the values in the dropdown are different from
             // those we autocomplete on.
-            JosmComboBox<String> comboBox = new JosmComboBox<>(usage.values.toArray(new String[0]));
+            JosmComboBox<String> comboBox = new JosmComboBox<>();
+            comboBox.getModel().addAllElements(usage.map.keySet());
             comboBox.setEditable(true);
             comboBox.setEditor(editor);
-            comboBox.getEditor().setItem(DIFFERENT);
+            comboBox.getEditor().setItem(DIFFERENT_I18N);
             value = comboBox;
-            originalValue = DIFFERENT;
+            originalValue = DIFFERENT_I18N;
         }
         initializeLocaleText(null);