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.

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 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 @@ -231,7 +231,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(false, false, false);
selector.setBorder(BorderFactory.createTitledBorder(tr("Search by preset")));
selector.setDblClickListener(ev -> setPresetDblClickListener(selector, editorComponent));

Expand Down
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 = false;

/**
* 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

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(true, true, true);
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(false, false, false);
setContent(selector, false);
selector.setDblClickListener(e -> buttonAction(0, null));
}
Expand Down
35 changes: 28 additions & 7 deletions src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
import org.openstreetmap.josm.tools.Utils;

/**
* GUI component to select tagging preset: the list with filter and two checkboxes
* GUI component to select tagging preset: the list with filter and three checkboxes
* @since 6068
*/
public class TaggingPresetSelector extends SearchTextResultListPanel<TaggingPreset>
Expand All @@ -66,9 +66,11 @@ public class TaggingPresetSelector extends SearchTextResultListPanel<TaggingPres

private static final BooleanProperty SEARCH_IN_TAGS = new BooleanProperty("taggingpreset.dialog.search-in-tags", true);
private static final BooleanProperty ONLY_APPLICABLE = new BooleanProperty("taggingpreset.dialog.only-applicable-to-selection", true);
private static final BooleanProperty DISPLAY_DEPRECATED = new BooleanProperty("taggingpreset.dialog.display-deprecated", true);

private final JCheckBox ckOnlyApplicable;
private final JCheckBox ckSearchInTags;
private final JCheckBox ckDeprecated;
private final Set<TaggingPresetType> typesInSelection = EnumSet.noneOf(TaggingPresetType.class);
private boolean typesInSelectionDirty = true;
private final transient PresetClassifications classifications = new PresetClassifications();
Expand Down Expand Up @@ -199,8 +201,9 @@ public String toString() {
* Constructs a new {@code TaggingPresetSelector}.
* @param displayOnlyApplicable if {@code true} display "Show only applicable to selection" checkbox
* @param displaySearchInTags if {@code true} display "Search in tags" checkbox
* @param displayDeprecated if {@code true} display "Applicable in region" checkbox
*/
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags) {
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags, boolean displayDeprecated) {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Should we add a checkbox for regions as well?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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)

super();
lsResult.setCellRenderer(new ResultListCellRenderer());
classifications.loadPresets(TaggingPresets.getTaggingPresets());
Expand Down Expand Up @@ -228,6 +231,15 @@ public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearc
ckSearchInTags = null;
}

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

Copy link

Choose a reason for hiding this comment

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

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

add(pnChecks, BorderLayout.SOUTH);

setPreferredSize(new Dimension(400, 300));
Expand All @@ -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();
Copy link

Choose a reason for hiding this comment

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

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.


DataSet ds = OsmDataManager.getInstance().getEditDataSet();
Collection<OsmPrimitive> selected = (ds == null) ? Collections.<OsmPrimitive>emptyList() : ds.getSelected();
final List<PresetClassification> result = classifications.getMatchingPresets(
text, onlyApplicable, inTags, getTypesInSelection(), selected);
text, onlyApplicable, inTags, isDeprecated, getTypesInSelection(), selected);

final TaggingPreset oldPreset = getSelectedPreset();
lsResultModel.setItems(Utils.transform(result, x -> x.preset));
Expand All @@ -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,
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

how about SearchStringFilters?

Copy link

Choose a reason for hiding this comment

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

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

Set<TaggingPresetType> presetTypes, final Collection<? extends OsmPrimitive> selectedPrimitives) {
final String[] groupWords;
final String[] nameWords;
Expand All @@ -292,11 +305,11 @@ public List<PresetClassification> getMatchingPresets(String searchText, boolean
nameWords = searchText.split("\\s", -1);
}

return getMatchingPresets(groupWords, nameWords, onlyApplicable, inTags, presetTypes, selectedPrimitives);
return getMatchingPresets(groupWords, nameWords, onlyApplicable, inTags, isDeprecated, presetTypes, selectedPrimitives);
}

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) {
Copy link

Choose a reason for hiding this comment

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

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.


final List<PresetClassification> result = new ArrayList<>();
for (PresetClassification presetClassification : classifications) {
Expand All @@ -317,6 +330,11 @@ public List<PresetClassification> getMatchingPresets(String[] groupWords, String
}
}

//do not show the preset in search dialog if isDeprecated is true and preset is deprecated
if (!isDeprecated && preset.deprecated()) {
continue;
}

if (groupWords != null && presetClassification.isMatchingGroup(groupWords) == 0) {
continue;
}
Expand Down Expand Up @@ -424,6 +442,9 @@ public void savePreferences() {
if (ckOnlyApplicable != null && ckOnlyApplicable.isEnabled()) {
ONLY_APPLICABLE.put(ckOnlyApplicable.isSelected());
}
if (ckDeprecated != null && ckDeprecated.isEnabled()) {
DISPLAY_DEPRECATED.put(ckDeprecated.isSelected());
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/unit/org/openstreetmap/josm/gui/tagging/presets/PresetClassificationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public static void setUp() throws IOException, SAXException {
}

private List<PresetClassification> getMatchingPresets(String searchText, OsmPrimitive w) {
return classifications.getMatchingPresets(searchText, true, true, EnumSet.of(TaggingPresetType.forPrimitive(w)),
return classifications.getMatchingPresets(searchText, true, true, true, EnumSet.of(TaggingPresetType.forPrimitive(w)),
Collections.singleton(w));
}

Expand Down