Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
915ce22
Update search dialog
Sarabjeet108 Jul 20, 2023
5cef4de
Add enum for PresetSearchFilters
Sarabjeet108 Aug 7, 2023
3e6df3e
Add support for deprecated attribute in search dialog
Sarabjeet108 Aug 27, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

@tsmock tsmock Aug 8, 2023

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
72 changes: 72 additions & 0 deletions src/org/openstreetmap/josm/gui/tagging/presets/PresetSearchFilter.java
View file
Open in desktop
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

@tsmock tsmock Aug 29, 2023

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

@tsmock tsmock Aug 8, 2023

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

@tsmock tsmock Aug 8, 2023

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

@tsmock tsmock Aug 8, 2023

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;
}

}
21 changes: 21 additions & 0 deletions src/org/openstreetmap/josm/gui/tagging/presets/TaggingPreset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ public class TaggingPreset extends AbstractAction implements ActiveLayerChangeLi
* Show the preset name if true
*/
public boolean preset_name_label;
/**
* True if the preset is deprecated
*/
private boolean deprecated;

/**
* The types as preparsed collection.
Expand Down Expand Up @@ -349,6 +353,23 @@ public void setMatch_expression(String filter) throws SAXException {
}
}

/**
* @return true if the preset is deprecated
* @apiNote this is not {@code isDeprecated} just in case we decide to make {@link TaggingPreset} a record class.
* @since xxx
*/
public final boolean deprecated() {
return this.deprecated;
}

/**
* Set if the preset is deprecated
* @param deprecated if true the preset is deprecated
Copy link

@tsmock tsmock Aug 29, 2023

Choose a reason for hiding this comment

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

Suggested change
* @param deprecated if true the preset is deprecated
* @param deprecated if true the preset is deprecated
* @since xxx

*/
public final void setDeprecated(boolean deprecated) {
this.deprecated = deprecated;
}

private static class PresetPanel extends JPanel {
private boolean hasElements;

Expand Down
2 changes: 1 addition & 1 deletion src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSearchDialog.java
View file
Open in desktop
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);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
setContent(selector, false);
SelectionEventManager.getInstance().addSelectionListener(selector);
selector.setDblClickListener(e -> buttonAction(0, null));
Expand Down
2 changes: 1 addition & 1 deletion src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSearchPrimitiveDialog.java
View file
Open in desktop
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);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

@tsmock tsmock Aug 8, 2023

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