Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add support for deprecated attribute and update the presets search dialog #123

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions src/org/openstreetmap/josm/data/validation/tests/TagChecker.java
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ public class TagChecker extends TagTest implements TaggingPresetListener {
* The preference key to check presets
*/
public static final String PREF_CHECK_PRESETS_TYPES = PREFIX + ".checkPresetsTypes";
public static final String PREF_CHECK_DEPRECATED = PREFIX + ".checkDeprecated";

/**
* The preference key for source files
Expand Down Expand Up @@ -159,6 +160,7 @@ public class TagChecker extends TagTest implements TaggingPresetListener {
* The preference key to search for presets - used before upload
*/
public static final String PREF_CHECK_PRESETS_TYPES_BEFORE_UPLOAD = PREF_CHECK_PRESETS_TYPES + BEFORE_UPLOAD;
public static final String PREF_CHECK_DEPRECATED_BEFORE_UPLOAD = PREF_CHECK_DEPRECATED + BEFORE_UPLOAD;

/**
* The preference key for the list of tag keys that are allowed to be the same on a multipolygon and an outer way
Expand All @@ -175,18 +177,21 @@ public class TagChecker extends TagTest implements TaggingPresetListener {
protected boolean checkComplex;
protected boolean checkFixmes;
protected boolean checkPresetsTypes;
protected boolean checkDeprecated;

protected JCheckBox prefCheckKeys;
protected JCheckBox prefCheckValues;
protected JCheckBox prefCheckComplex;
protected JCheckBox prefCheckFixmes;
protected JCheckBox prefCheckPresetsTypes;
protected JCheckBox prefCheckDeprecated;

protected JCheckBox prefCheckKeysBeforeUpload;
protected JCheckBox prefCheckValuesBeforeUpload;
protected JCheckBox prefCheckComplexBeforeUpload;
protected JCheckBox prefCheckFixmesBeforeUpload;
protected JCheckBox prefCheckPresetsTypesBeforeUpload;
protected JCheckBox prefCheckDeprecatedBeforeUpload;

// CHECKSTYLE.OFF: SingleSpaceSeparator
protected static final int EMPTY_VALUES = 1200;
Expand All @@ -210,6 +215,7 @@ public class TagChecker extends TagTest implements TaggingPresetListener {
protected static final int MULTIPOLYGON_INCOMPLETE = 1219;
protected static final int MULTIPOLYGON_MAYBE_NO_AREA = 1220;
protected static final int MULTIPOLYGON_SAME_TAG_ON_OUTER = 1221;
protected static final int DEPRECATED_TAG = 1223;
// CHECKSTYLE.ON: SingleSpaceSeparator

protected EditableList sourcesList;
Expand Down Expand Up @@ -654,15 +660,22 @@ public void check(OsmPrimitive p) {
checkMultipolygonTags(p);
}

final Collection<TaggingPreset> matchingPresets;
TagMap tags = p.getKeys();
if (checkPresetsTypes || checkDeprecated) {
matchingPresets = presetIndex.entrySet().stream()
.filter(e -> TaggingPresetItem.matches(e.getValue(), tags))
.map(Entry::getKey)
.collect(Collectors.toCollection(LinkedHashSet::new));
} else {
matchingPresets = null;
}
if (checkPresetsTypes) {
TagMap tags = p.getKeys();

TaggingPresetType presetType = TaggingPresetType.forPrimitive(p);
EnumSet<TaggingPresetType> presetTypes = EnumSet.of(presetType);

Collection<TaggingPreset> matchingPresets = presetIndex.entrySet().stream()
.filter(e -> TaggingPresetItem.matches(e.getValue(), tags))
.map(Entry::getKey)
.collect(Collectors.toCollection(LinkedHashSet::new));

Collection<TaggingPreset> matchingPresetsOK = matchingPresets.stream().filter(
tp -> tp.typeMatches(presetTypes)).collect(Collectors.toList());
Collection<TaggingPreset> matchingPresetsKO = matchingPresets.stream().filter(
Expand All @@ -687,6 +700,18 @@ public void check(OsmPrimitive p) {
}
}
}
if (checkDeprecated) {
for (TaggingPreset preset : matchingPresets) {
if (preset.deprecated()) {
errors.add(TestError.builder(this, Severity.ERROR, DEPRECATED_TAG)
.message(tr("Preset is deprecated"),
marktr("Preset {0} should not be used"),
tr(preset.getName()))
.primitives(p)
.build());
}
}
}
}

private static final Collection<String> NO_AREA_KEYS = Arrays.asList("name", "area", "ref", "access", "operator");
Expand Down Expand Up @@ -1099,6 +1124,11 @@ public void startTest(ProgressMonitor monitor) {
if (isBeforeUpload) {
checkPresetsTypes = checkPresetsTypes && Config.getPref().getBoolean(PREF_CHECK_PRESETS_TYPES_BEFORE_UPLOAD, true);
}

checkDeprecated = includeOtherSeverity && Config.getPref().getBoolean(PREF_CHECK_DEPRECATED, true);
if (isBeforeUpload) {
checkDeprecated = checkDeprecated && Config.getPref().getBoolean(PREF_CHECK_DEPRECATED_BEFORE_UPLOAD, true);
}
deprecatedChecker = OsmValidator.getTest(MapCSSTagChecker.class);
ignoreForOuterMPSameTagCheck.addAll(Config.getPref().getList(PREF_KEYS_IGNORE_OUTER_MP_SAME_TAG, Collections.emptyList()));
}
Expand All @@ -1111,7 +1141,7 @@ public void endTest() {

@Override
public void visit(Collection<OsmPrimitive> selection) {
if (checkKeys || checkValues || checkComplex || checkFixmes || checkPresetsTypes) {
if (checkKeys || checkValues || checkComplex || checkFixmes || checkPresetsTypes || checkDeprecated) {
super.visit(selection);
}
}
Expand Down Expand Up @@ -1176,6 +1206,14 @@ public void addGui(JPanel testPanel) {
prefCheckPresetsTypesBeforeUpload = new JCheckBox();
prefCheckPresetsTypesBeforeUpload.setSelected(Config.getPref().getBoolean(PREF_CHECK_PRESETS_TYPES_BEFORE_UPLOAD, true));
testPanel.add(prefCheckPresetsTypesBeforeUpload, a);

prefCheckDeprecated = new JCheckBox(tr("Check for deprecated tags"), Config.getPref().getBoolean(PREF_CHECK_DEPRECATED, true));
prefCheckDeprecated.setToolTipText(tr("Check whether the preset is deprecated"));
testPanel.add(prefCheckDeprecated, GBC.std().insets(20, 0, 0, 0));

prefCheckDeprecatedBeforeUpload = new JCheckBox();
prefCheckDeprecatedBeforeUpload.setSelected(Config.getPref().getBoolean(PREF_CHECK_DEPRECATED_BEFORE_UPLOAD, true));
testPanel.add(prefCheckDeprecatedBeforeUpload, a);
}

/**
Expand All @@ -1198,11 +1236,13 @@ public boolean ok() {
Config.getPref().putBoolean(PREF_CHECK_KEYS, prefCheckKeys.isSelected());
Config.getPref().putBoolean(PREF_CHECK_FIXMES, prefCheckFixmes.isSelected());
Config.getPref().putBoolean(PREF_CHECK_PRESETS_TYPES, prefCheckPresetsTypes.isSelected());
Config.getPref().putBoolean(PREF_CHECK_DEPRECATED, prefCheckDeprecated.isSelected());
Config.getPref().putBoolean(PREF_CHECK_VALUES_BEFORE_UPLOAD, prefCheckValuesBeforeUpload.isSelected());
Config.getPref().putBoolean(PREF_CHECK_COMPLEX_BEFORE_UPLOAD, prefCheckComplexBeforeUpload.isSelected());
Config.getPref().putBoolean(PREF_CHECK_KEYS_BEFORE_UPLOAD, prefCheckKeysBeforeUpload.isSelected());
Config.getPref().putBoolean(PREF_CHECK_FIXMES_BEFORE_UPLOAD, prefCheckFixmesBeforeUpload.isSelected());
Config.getPref().putBoolean(PREF_CHECK_PRESETS_TYPES_BEFORE_UPLOAD, prefCheckPresetsTypesBeforeUpload.isSelected());
Config.getPref().putBoolean(PREF_CHECK_DEPRECATED_BEFORE_UPLOAD, prefCheckDeprecatedBeforeUpload.isSelected());
return Config.getPref().putList(PREF_SOURCES, sourcesList.getItems());
}

Expand Down
3 changes: 2 additions & 1 deletion src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSException;
import org.openstreetmap.josm.gui.tagging.ac.AutoCompComboBox;
import org.openstreetmap.josm.gui.tagging.ac.AutoCompComboBoxModel;
import org.openstreetmap.josm.gui.tagging.presets.PresetSearchFilter;
import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetSelector;
import org.openstreetmap.josm.gui.widgets.AbstractTextComponentValidator;
Expand Down Expand Up @@ -231,7 +232,7 @@ public boolean isValid() {
* selected preset by the user. Every query is of the form ' group/sub-group/.../presetName'
* if the corresponding group of the preset exists, otherwise it is simply ' presetName'.
*/
selector = new TaggingPresetSelector(false, false, false);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

Choose a reason for hiding this comment

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

This would be the equivalent of new TaggingPresetSelector(true, true, true), which I don't think you wanted. You probably wanted to just do new TaggingPresetSelector().

selector.setBorder(BorderFactory.createTitledBorder(tr("Search by preset")));
selector.setDblClickListener(ev -> setPresetDblClickListener(selector, editorComponent));

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// License: GPL. For details, see LICENSE file.
package org.openstreetmap.josm.gui.tagging.presets;

import java.util.EnumMap;
import java.util.Map;

import static org.openstreetmap.josm.tools.I18n.marktr;

/**
* This enum defines different filters for searching presets.
*/
Copy link

Choose a reason for hiding this comment

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

Suggested change
*/
* @since xxx
*/

public enum PresetSearchFilter {
ONLY_APPLICABLE(marktr("Show only applicable to selection")),
SEARCH_IN_TAGS(marktr("Search in tags")),
DEPRECATED_TAGS(marktr("Show deprecated tags"));

/**
* Map containing the preferences for the filters
*/
private Map<PresetSearchFilter, Boolean> filtersPreference;

static {
for (PresetSearchFilter filter : values()) {
filter.filtersPreference = new EnumMap<>(PresetSearchFilter.class);
filter.filtersPreference.put(filter, true);
}
}

/**
* Sets the preference for the filter
* @param filter the filter to set the preference for
* @param pref true if the filter is enabled, false otherwise
* @since xxx
*/
public void setPref(PresetSearchFilter filter, Boolean pref) {
filtersPreference.put(filter, pref);
}

/**
* Gets the preference for the filter
* @param filter the filter to get the preference for
* @return true if the filter is enabled, false otherwise
* @since xxx
*/
public Boolean getPref(PresetSearchFilter filter) {
return filtersPreference.get(filter);
Copy link

Choose a reason for hiding this comment

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

This should be done via the standard Config interface or one of the Property classes.

}
Copy link

Choose a reason for hiding this comment

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

When I said, "we should use an EnumMap", this is not what I was thinking. I'll go find one of the methods using this enum and see if I can hack together something there as an example.

Generally speaking, enum classes should not have a state that can change. There are exceptions, but they are exceptions.


/**
* The translated text associated with the enum constant.
*/
private final String translatedText;

/**
* Constructor for the PresetSearchFilter enum.
* Initializes an enum constant with its corresponding translated text.
*
* @param translatedText The translated text associated with the enum constant.
*/
PresetSearchFilter(String translatedText) {
this.translatedText = translatedText;
Copy link

Choose a reason for hiding this comment

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

To avoid confusion, this should probably be textToBeTranslated or something; translatedText implies it has already been translated.

}

/**
* Returns the text associated with the filter
* @return the text marked for translation
*/
public String getText() {
return translatedText;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public class TaggingPreset extends AbstractAction implements ActiveLayerChangeLi
/**
* True if the preset is deprecated
*/
private boolean deprecated = false;
private boolean deprecated;

/**
* The types as preparsed collection.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ private TaggingPresetSearchDialog() {
super(MainApplication.getMainFrame(), tr("Search presets"), tr("Select"), tr("Cancel"));
setButtonIcons("dialogs/search", "cancel");
configureContextsensitiveHelp("/Action/TaggingPresetSearch", true /* show help button */);
selector = new TaggingPresetSelector(true, true, true);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
setContent(selector, false);
SelectionEventManager.getInstance().addSelectionListener(selector);
selector.setDblClickListener(e -> buttonAction(0, null));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static synchronized TaggingPresetSearchPrimitiveDialog getInstance() {
super(MainApplication.getMainFrame(), tr("Search for objects by preset"), tr("Search"), tr("Cancel"));
setButtonIcons("dialogs/search", "cancel");
configureContextsensitiveHelp("/Action/TaggingPresetSearchPrimitive", true /* show help button */);
selector = new TaggingPresetSelector(false, false, false);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

Choose a reason for hiding this comment

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

This would be the equivalent of new TaggingPresetSelector(true, true, true), which I don't think you wanted. You probably wanted to just do new TaggingPresetSelector().

setContent(selector, false);
selector.setDblClickListener(e -> buttonAction(0, null));
}
Expand Down
Loading