[PATCH] Make tagging presets more flexible

[anonymous]

I've been mapping traffic signs lately and it's really time consuming to do it for multiple signs at the same point. It's almost always necessary to manually add the signs in the editor because the tagging presets only allow to replace an existing value but not append it.

So I've decided to make the tagging presets more flexible with the option to allow appending of values with the <key> tag. That made it easier but with time a lot more changes made it even better. So I've also added options for the <multiselect> tag and sort options to <group> tag.

Of course the default behavior is unchanged, so old presets will keep working as they had been until now.

Changes to tag <group>:

• new optional attribute sort: Boolean value. If sort is set to false and alphabetically sorting is enabled for preset menu all items in this group will not be sorted.

Changes to tag <key>:

• new optional attribute append: Can contain values default or always, with default value is set for non existing key or appended with value if current value doesn't contain value, with always value is set for non existing key or appended with value regardless if it is already present.
• new optional attribute append_value: When appending current value this value is used instead of value, if append_value is present append can be omitted it will then default to default.
• new optional attribute prefix: When key is empty, the prefix will be added before value, when appending prefix will not be used.
• new optional attribute delimiter: When appending value use delimiter to separate values from each other. Defaults to ; when omitted.

Changes to tag <multiselect>:

• new optional attribute quick_select: Contains a boolean value. If it is true the entries in the list can be selected by single left click without the need to hold down a key on the keyboard.
• new optional attribute prefix: Is added to the begin of the new value.
• added option user to attribute values_sort: if user is set the preset dialog contains arrow buttons to sort the entries of the mulitselect.

Changes to tag <combo>:

• new optional attribute if_needed_only: Contains boolean value. If value is true the preset dialog is only shown if the key doesn't exists or other tags cause it to be shown.

Changes to list_entry:

• new optional attribute delimiter: When value is selected use delimiter to separate value of this list entry from preceding value.

New tag selectgroup:

• Allows tags of list_entry to be grouped together for multiselect. Only one list entry of each selectgroup can be selected at the same time. Also usable to make multiselect behave like a single select list.

For instance with this changes it's now possible to set traffic_sign to DE:101,1012-51;274-30,1042-31 with a single multiselect preset item without the risk of accidentally add contradictory signs.

[SimonPoole]

Couple of comments

• I implemented item sorting in groups way back see ​http://vespucci.io/tutorials/presets/#extensions (I didn't use booleans at the time to potentially allow specific sorting values going forward), it would be nice if this wasn't gratuitously done differently.
• if_needed_only would seem to make a tag uneditable via the preset dialog once it is present, that doesn't seem like a good idea at all and very confusing for users.
• overall: isn't this very specific to your use case? Are there other tagging schemes that could use this?

[anonymous]

• Change sorting of groups to another attribute seems easy enough, so that wouldn't be a problem.
• if_needed_only has exactly that purpose, when the key already exists, it should not be shown, for this use case. In case of adding traffic signs to existing signs the direction key cannot be different than the already existing value. So when adding additional signs the dialog will show even though it isn't needed and causes additional clicks. That seems not to be much, but when adding additional signs it will add up to a lot of unnecessary clicks. Still this part is not so important for this patch.
• yes it is specific to my use case but I thought maybe others would like to have those options too. Especially select groups prevent entering of conflicting values. Currently a multiselect will always allow selecting every list value at the same time even if they are impossible to exist together. So users can enter values that are not valid. A multiselect has the purpose to select multiple values from that list, so having to hold a key on the keyboard just to select values seems rather unnecessary. Just having to click on the value one wants to select is a lot more sensible to me.

I've been using this changes for a while now and they help to save a lot of time.

[anonymous]

I've changed the patch to now use items_sort for sorting of preset menu items. It can be set to yes (default value), no and always, when yes is set the items are sorted if sorting is enabled in preferences, when always is set the items will be sorted regardless if sorting is enabled in preferences and when no is set the items will not be sorted.

[SimonPoole]

One further point that needs to be considered. You only allow list_entry elements in selectgroup, from a consistency pov, I believe this should allow references to chunks that contain (only) a sequence of list_entry. This isn't actually documented anywhere except in the xsd file but is very useful in preset maintenance. See also ​https://github.com/simonpoole/beautified-JOSM-preset/pull/355

[anonymous]

Very good point. I've changed it to support chunks. I've also found another problem when selecting values for the access key. The value no cannot be combined with other values, so even selectgroup won't help here, I've added the attribute exclusive (boolean value) to selectgroup}} which if true prevents values in that {{{selectgroup from being selected when other values are selected. So the value Kein Zugang in the screenshot below cannot be selected when any other value is, while the other values can be selected all at the same time.

I've forgot to mention that multiselect can also have the attribute if_needed_only. Like in the first screenshot I've been using a multiselect with one selectgroup turning it into a quasi single select instead of using combo for selecting direction values, which needs one click less than using a combo. It also seems to be not much, but saving one click for a single action saves a lot of time overall.

[SimonPoole]

WRT if_needed_only if I understand the code correctly this makes not only the tag essentially write only, but it seems as if you don't parse the tag value taking the per list_entry delimiter in to account, which again would be rather confusing for a user, or am I misunderstanding something here?

[anonymous]

I don't know if I understand your question correctly. The if_needed_only attribute is intended for easy tasks like setting the direction key, it's definitely not intended to be used for multiselects with list entries that set delimiter.

I understand that it's possible to be set on such multiselects but I don't understand why that would be done by anyone for complex selections since I cannot come up with such a selection that would be write only.

In the first draft I've added a separate <singleselect> tag but with <selectgroup> that seemed to be just overhead, so I've removed it and added the attribute if_needed_only to <multiselect>.

[anonymous]

Still I've found an issue with if_needed_only when a value for the key is not in the list. If that happens the dialog should definitely be shown.

[SimonPoole]

My concern is while preset based editing is not the focus of JOSM, it still can and is done (and that is likely to increase in the future). As I understand your code now, initial tagging is fine, however if the user wants to change the tagging the preset form will not work as expected and the user will have to manually edit the tag value.

[anonymous]

I do understand your concern but if_needed_only is only an option and would typically not be set. Maybe an option in the settings of the presets would be a possibility. This option is typically false but can be set to true to support if_needed_only. So only users who know what to expect would enabled this and all other users would get the dialog even if if_needed_only is set.

[anonymous]

But you are not parsing the delimiter element either, so you won't be able to modify tags that have the multiple level of values (which this essentially allows) either.

[anonymous]

That seems to be an overly constructed case. So again if_needed_only is an optional value, that is intended to be used for simple tasks, not complicated selections. I would understand that concern if if_needed_only would be a new default for all presets, but it isn't. If it isn't set all stays the same like it currently is and if an option is added to the settings of presets overall it wouldn't even apply if the user doesn't activate it's usage.

In which case will it happen, that a delimiter for a value would be different from the one that the preset sets?

For traffic_sign for example the delimiter ; is used for main signs like DE:205 and , for signs that only occur together with main signs like DE:1000-32. There is no case for a delimiter that could be any other way and if the value has the wrong delimiter it would just be replaced with the correct delimiter that the list_entry sets, if if_needed_only would be true, which it wouldn't be for a selection like in the screenshot in the first post.

If you think that the delimiter would cause problems for if_needed_only you must have an example in mind where it really could happen. For which key would that apply?

And I'd like to mention that currently it isn't even possible to create a multiselect that even allows to create a selection with correct delimiters for all signs and it is possible to select signs that contradict each other at the same time. I think that is an actual and far greater problem than some constructed "might be possible to be misused or confuse users".

The attribute if_needed_only is intended to speed up adding values to existing keys not to confuse the user by setting it for complex selections.

[anonymous]

To add some data. I've checked the time I need to handle a dialog that just needs to be accepted without selecting anything in it. On my small notebook screen when I know the exact position the dialog will pop up and the button to click I need about 2 seconds to handle it. That amounts to about 30 minutes for each 1000 signs and that's a low estimate because one need to be on high alert to be able to handle a dialog in such a short time span. A lot of useless time that would just be needed so that no user might be confused by an option that needs to be enabled in the settings. That's a lot of time to trade for an application like JOSM that already is really complex and will often confuse new users anyhow. With an option that needs to be enabled to use if_needed_only that wouldn't even apply to new users but only those who should know what to expect.

[anonymous]

So I've added the option to the settings and came up with an idea to show the dialog when the user needs it to change the value. Now when the option is enabled and if_needed_only is set the dialog will still be shown if the user presses the CTRL key while selecting the preset menu item.

[anonymous]

Additionally, how about highlighting preset menu entries with if_needed_only enabled?

For example text in blue and italic:

[taylor.smock]

From a quick read through:

• I'd strongly prefer to avoid new public fields in Java classes that are not final. And even then, I'd still prefer to avoid it. This is largely due to wanting to move the preset system into Java records and eventually (hopefully) make them into value classes, but the former is blocked by #17858 and the latter isn't yet available in the JDK (see ​JEP draft 8277163). public static final fields are ok for now.
• New @since in comments should be @since xxx -- we have a script that will replace the xxx with the appropriate JOSM revision
• Be careful with ctrl -- you probably ought to use PlatformManager.getPlatform().getMenuShortcutKeyMaskEx(). Of specific note, Mac uses the meta key instead of ctrl.
• Boolean.TRUE.toString() instead of String.valueOf(true) -- these are functionally equivalent, but I think that the former is more readable
• Instead of values_sort.equals(String.valueOf(true)), use Boolean.valueOf(values_sort). This avoids a potential NPE and an additional if statement (that may or may not be optimized out).
• Please write tests. As an example, in most cases, an NPE will be thrown if the Key.toString() method is called. This is because the compiler will initialize the append_value to null.
• This also means that the public String append = null; could just be pubic String append;. There are a couple of other places in your code where you initialize variables to the default values.
• The delimiter should just be a char, unless you know of a delimiter that is 2 or more characters in OSM. In which case we need to change another delimiter value. Unless we really need a null value -- keep in mind that you set a default in your code.
• Please avoid // NOSONAR wherever possible. If you do need to use it, explain why -- possible exception: the public fields in the TaggingPresetItem class hierarchy, since those are written to by the XML parser.
• Please don't add new TODO items that are just because something was autogenerated. Either it is "right" or it is not. So the SelectGroup class is kind of problematic.
• For TaggingPreset#createPanel, create an overload and with the current method call the new method with a default (e.g. createPanel(selected, false)). Please keep in mind that plugins may be affected by your changes. Specifically EasyPreset. There are probably others.
• Please run ant pmd checkstyle and fix the warnings

With all that said, please profile using the Name Suggestion Index (NSI). The NSI is (almost certainly) the largest preset we have general access to. It probably has more than 10k entries. Note: if you are on Mac, don't try to open the preset menu -- the F3 shortcut will work fine.

Other tickets that may be of interest:

• #15007: Check and set key/value for more than one key/value (more of a multicheckbox request)
• #15217 (NSI in JOSM, just due to scope, the patch there was trying to be as noninvasive as possible)
• #19190: Reuse strings throughout presets (specifically value strings). This can be worked around with chunk and list_entry values.
• #21228: Allow overriding some parts of the preset dialog