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

Sarabjeet108 · 2023-07-20T20:25:07Z

I discussed this with Simon about having a filter for the added attributes. We can also have this for region attributes, although I think it is unnecessary in the case of deprecated.

I am having the same issue as with the regions attribute, the validate method in TaggingPresetValidation class is not adding the errors from the check method in TagChecker class.

tsmock · 2023-07-25T11:55:26Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

     */
-    public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags) {
+    public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags, boolean displayDeprecated) {


At this point, I think we should start using an enum instead of a bunch of booleans.

Example:

Suggested change

public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags, boolean displayDeprecated) {

public enum DisplayCheckboxes {

ONLY_APPLICABLE,

SEARCH_IN_TAGS,

DEPRECATED

}

public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags) {

this(displayOnlyApplicable ? ONLY_APPLICABLE : null, displaySearchInTags ? SEARCH_IN_TAGS : null);

}

public TaggingPresetSelector(DisplayCheckboxes... options) {

// Many if statements

}

I'll note that my code isn't properly organized, so the enum would go better elsewhere, and I didn't add the class reference in the constructor, but that should (largely) work.

Should we add a checkbox for regions as well?

We could, yes. Realistically, the "primary" caller will probably just use DisplayCheckboxes.values() instead of hardcoding the current values.

Realistically, the "primary" caller will probably just use DisplayCheckboxes.values() instead of hardcoding the current values.

Does that mean we would use the preference for the checkbox associated with the filter like ONLY_APPLICABLE.getPref() instead of using bool value? (getPref will return boolean value)

tsmock · 2023-07-25T11:57:09Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+        if (displayDeprecated) {
+            ckDeprecated = new JCheckBox();
+            ckDeprecated.setText(tr("Show deprecated tags"));
+            pnChecks.add(ckDeprecated);
+            ckDeprecated.addItemListener(e -> filterItems());
+        } else {
+            ckDeprecated = null;
+        }
+


Going further on the enums, there are EnumMaps that we should start using, and the translated texts can be done via an enum field (e.g. DEPRECATED(marktr("Show deprecated tags")) + ck.setText(tr(DEPRECATED.getText())).

tsmock · 2023-07-25T11:58:49Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

@@ -254,11 +266,12 @@ protected synchronized void filterItems() {
        String text = edSearchText.getText().toLowerCase(Locale.ENGLISH);
        boolean onlyApplicable = ckOnlyApplicable != null && ckOnlyApplicable.isSelected();
        boolean inTags = ckSearchInTags != null && ckSearchInTags.isSelected();
+        boolean isDeprecated = ckDeprecated != null && ckDeprecated.isSelected();


Moving on with the EnumMap idea, this could be done via that. For that matter, the checkbox could have a change listener that puts a boolean in an enum map.

tsmock · 2023-07-25T12:00:38Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

@@ -279,7 +292,7 @@ public static class PresetClassifications implements Iterable<PresetClassificati

        private final List<PresetClassification> classifications = new ArrayList<>();

-        public List<PresetClassification> getMatchingPresets(String searchText, boolean onlyApplicable, boolean inTags,
+        public List<PresetClassification> getMatchingPresets(String searchText, boolean onlyApplicable, boolean inTags, boolean isDeprecated,


We should probably use an enum here. We could probably reuse the one I mentioned earlier (so getMatchingPresets(String searchText, DisplayCheckboxes..., so DisplayCheckboxes is probably the wrong name; see if you can find one yourself).

how about SearchStringFilters?

That would probably work. But we have other locations where we have search strings. It might be better to have some more identifying information. Although this really depends upon where we put the enum -- if it is in its own file, it will need a much better identifier, such as PresetSearchFilter. If it remains in the TaggingPresetSelector class, then that would be part of the identifier (TaggingPresetSelector.SearchStringFilters.DEPRECATED).

tsmock · 2023-07-25T12:02:53Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

-        public List<PresetClassification> getMatchingPresets(String[] groupWords, String[] nameWords, boolean onlyApplicable,
-                boolean inTags, Set<TaggingPresetType> presetTypes, final Collection<? extends OsmPrimitive> selectedPrimitives) {
+        public List<PresetClassification> getMatchingPresets(String[] groupWords, String[] nameWords, boolean onlyApplicable, boolean inTags,
+                 boolean isDeprecated, Set<TaggingPresetType> presetTypes, final Collection<? extends OsmPrimitive> selectedPrimitives) {


I should probably mention (at this point) why I think we should start moving these booleans to an enum: easier readability. There are some other advantages, but function(bool, bool, bool, bool, bool, bool) start to get really hard to wrap your mind around. Some IDEs will give hints as to what each bool is supposed to be, but not all IDEs will.

As a specific example, GitHub doesn't show what a boolean is supposed to be for, so the reviewer/editor has to know the function definition, or go find it.

Sarabjeet108 · 2023-08-07T22:14:57Z

I am facing a few problems

How will we initialize the values for the filters in the enum map? As a temporary fix, I assigned true to all values using a static block.
You mentioned that the primary caller would call using the .values() method. How will that work if we need the boolean values associated with the filters since .values() will return an array of the enum constants?

tsmock · 2023-08-08T18:21:52Z

src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java

@@ -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());


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().

tsmock · 2023-08-08T18:30:57Z

src/org/openstreetmap/josm/gui/tagging/presets/PresetSearchFilter.java

+    /**
+     * 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);
+    }


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.

tsmock · 2023-08-08T18:33:04Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+    PresetSearchFilter ONLY_APPLICABLE = PresetSearchFilter.ONLY_APPLICABLE;
+    PresetSearchFilter SEARCH_IN_TAGS = PresetSearchFilter.SEARCH_IN_TAGS;
+    PresetSearchFilter DEPRECATED_TAGS = PresetSearchFilter.DEPRECATED_TAGS;


You shouldn't need/want to do this. Importing them statically would work, or you can just use the qualified name (PresetSearchFilter.DEPREACATED_TAGS). I only saw these used in a switch statement, where they aren't needed.

tsmock · 2023-08-08T18:34:50Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+            switch(option) {
+                case ONLY_APPLICABLE: {
+                    ckOnlyApplicable = new JCheckBox();
+                    ckOnlyApplicable.setText(tr(PresetSearchFilter.ONLY_APPLICABLE.getText()));


You could use option.getText() here instead of calling the qualified name.

We want to have a map of option -> checkboxes, so EnumMap<PresetSearchFilter, JCheckBox> would work. Example:

final Map<PresetSearchFilter, JCheckBox> checkboxes = new EnumMap<PresetSearchFilter, JCheckBox>(PresetSearchFilter.class); for (PresetSearchFilterOption option : options) { final JCheckBox box = new JCheckBox(); box.setText(tr(option)); box.addItemListener(e -> filterItems()); pnChecks.add(box); switch (option) { case SEARCH_IN_TAGS: box.setSelected(PREF_SEARCH_IN_TAGS.get()); break; // Do other enum values } }

tsmock · 2023-08-08T18:37:33Z

src/org/openstreetmap/josm/gui/tagging/presets/PresetSearchFilter.java

+     * @param translatedText The translated text associated with the enum constant.
+     */
+    PresetSearchFilter(String translatedText) {
+        this.translatedText = translatedText;


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

tsmock · 2023-08-08T18:37:59Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSearchPrimitiveDialog.java

@@ -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());


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().

tsmock · 2023-08-08T18:43:34Z

src/org/openstreetmap/josm/gui/tagging/presets/PresetSearchFilter.java

+        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);


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

tsmock · 2023-08-08T18:54:29Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

        final List<PresetClassification> result = classifications.getMatchingPresets(
-                text, onlyApplicable, inTags, isDeprecated, getTypesInSelection(), selected);
+                text, ONLY_APPLICABLE.getPref(PresetSearchFilter.ONLY_APPLICABLE), SEARCH_IN_TAGS.getPref(PresetSearchFilter.SEARCH_IN_TAGS),
+                DEPRECATED_TAGS.getPref(PresetSearchFilter.DEPRECATED_TAGS), getTypesInSelection(), selected);


Ideally, you would just use the same EnumMap from one of my other comments (EnumMap<PresetSearchFilter, JCheckBox>) instead.

Example: map.entrySet().stream().filter(entry -> entry.getValue().isSelected().map(Map.Entry::getKey).toArray(PresetSearchFilter[]::new).

tsmock · 2023-08-29T12:17:04Z

resources/data/tagging-preset.xsd

@@ -121,7 +121,7 @@
    <complexType name="item">
        <annotation>
            <documentation>
-                Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, icon and name_template are optional attributes.
+                Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, name_template are optional attributes.


Suggested change

Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, name_template are optional attributes.

Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, and name_template are optional attributes.

tsmock · 2023-08-29T12:19:29Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPreset.java

+
+    /**
+     * Set if the preset is deprecated
+     * @param deprecated if true the preset is deprecated


Suggested change

* @param deprecated if true the preset is deprecated

* @param deprecated if true the preset is deprecated

* @since xxx

tsmock · 2023-08-29T12:24:03Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+    Map<PresetSearchFilter, Boolean> ckpreferences = new EnumMap<>(PresetSearchFilter.class);
+    {
+        //initial value of the checkboxes
+        ckpreferences.put(PresetSearchFilter.ONLY_APPLICABLE, true);
+        ckpreferences.put(PresetSearchFilter.SEARCH_IN_TAGS, true);
+        ckpreferences.put(PresetSearchFilter.DEPRECATED_TAGS, false);
+    }


We should probably keep the properties.

Realistically, the map can be populated with ckpreferences.computeIfAbsent(key, k -> PROPERTY.get()).

Using (and saving) the preferences lets us keep the checkbox state between application restarts.

tsmock · 2023-08-29T12:27:02Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+        if (checkboxes.get(PresetSearchFilter.SEARCH_IN_TAGS) != null) {
+            ckpreferences.put(PresetSearchFilter.SEARCH_IN_TAGS, checkboxes.get(PresetSearchFilter.SEARCH_IN_TAGS).isSelected());
+        }
+        if (checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE) != null && checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE).isEnabled()) {
+            ckpreferences.put(PresetSearchFilter.ONLY_APPLICABLE, checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE).isSelected());
        }
-        if (ckOnlyApplicable != null && ckOnlyApplicable.isEnabled()) {
-            ONLY_APPLICABLE.put(ckOnlyApplicable.isSelected());
+        if (checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS) != null && checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS).isEnabled()) {
+            ckpreferences.put(PresetSearchFilter.DEPRECATED_TAGS, checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS).isSelected());


I think we want to keep the PREFERENCE.put method calls.

tsmock · 2023-08-29T12:28:43Z

src/org/openstreetmap/josm/gui/tagging/presets/PresetSearchFilter.java

+
+/**
+ * This enum defines different filters for searching presets.
+ */


Suggested change

*/

* @since xxx

*/

tsmock · 2023-08-29T12:29:15Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSearchDialog.java

+        selector = new TaggingPresetSelector(PresetSearchFilter.ONLY_APPLICABLE, PresetSearchFilter.SEARCH_IN_TAGS,
+                PresetSearchFilter.DEPRECATED_TAGS);


Suggested change

selector = new TaggingPresetSelector(PresetSearchFilter.ONLY_APPLICABLE, PresetSearchFilter.SEARCH_IN_TAGS,

PresetSearchFilter.DEPRECATED_TAGS);

selector = new TaggingPresetSelector(PresetSearchFilter.values());

tsmock · 2023-08-29T12:29:41Z

src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java

+
+    /**
+     * Called from the {@code TaggingPresetConstructor}.
+     *


Suggested change

*

* @since xxx

Update search dialog

915ce22

tsmock requested changes Jul 25, 2023

View reviewed changes

Add enum for PresetSearchFilters

5cef4de

Sarabjeet108 requested a review from tsmock August 7, 2023 21:52

tsmock requested changes Aug 8, 2023

View reviewed changes

Add support for deprecated attribute in search dialog

3e6df3e

Sarabjeet108 changed the title ~~Update search dialog~~ Add support for deprecated attribute and update the presets search dialog Aug 27, 2023

tsmock requested changes Aug 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Sarabjeet108 commented Jul 20, 2023

tsmock Jul 25, 2023

Sarabjeet108 Jul 25, 2023

tsmock Jul 25, 2023

Sarabjeet108 Aug 7, 2023

tsmock Jul 25, 2023

tsmock Jul 25, 2023

tsmock Jul 25, 2023

Sarabjeet108 Jul 31, 2023

tsmock Aug 1, 2023

tsmock Jul 25, 2023

Sarabjeet108 commented Aug 7, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 8, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

tsmock Aug 29, 2023

	Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, name_template are optional attributes.
	Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, and name_template are optional attributes.

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

		selector = new TaggingPresetSelector(PresetSearchFilter.ONLY_APPLICABLE, PresetSearchFilter.SEARCH_IN_TAGS,
		PresetSearchFilter.DEPRECATED_TAGS);

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

Are you sure you want to change the base?

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

Conversation

Sarabjeet108 commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sarabjeet108 commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment