[WIP PATCH] Rewrite of the presets system

[marcello@…]

This patch contains following changes:

The XML reader was completely rewritten to not use java introspection any more.

All classes of the XML reader have been placed into one package, considerably reducing the number of public fields and methods.

The XML reader now preserves element nesting. Element groups like "Optional Attributes" now can have a surrounding border in the dialog. (This is a first step towards conditionally shown preset entries, eg. the 'width' entry could be hidden for highway areas.)

The concepts of preset template and preset instance are cleanly separated. The XML file gets parsed into a tree of immutable preset templates. The templates are used to create panels, menu items, and support instances.

Preset patch files have been added as experimental feature to allow further customization of existing preset files. A preset patch file's main function is to override chunks in the preset file. A preset patch file has the same structure as the defaultpresets.xml file. All items in the root of the preset patch file will be appended to the root of the respective presets file, ie. the root elements of both files will be merged while chunks in the patch file will override chunks with the same id in the presets file. The patch file must be placed in the josmdir:// and have the same filename and extension with an added extension of .local eg. <josmdir>/defaultpresets.xml.local.

The preset system now uses a pluggable handler for all data access so any preset can operate on the dataset or any other key/value store like the tag table in the relation editor. Fixes #21221

Autocomplete suggestions can be filtered in the relation editor. Comboboxes have been added that provide suggestions. Fixes #21227

This is a huge patch, it should be reviewed and tested before applying. Suggestions are welcome.

[stoecker]

You don't explain the why. A complete rewrite will introduce dozens of bugs. Why is this necessary?

I don't like the "preset patch" idea. It's already possible to extend existing sections. Rewriting stuff will cause support trouble without much benefit.

[marcello@…]

Because the old code is ugly and badly maintainable. All fields in the preset dialogs are initialized through java object introspection, and as a consequence all those fields are public. This is bad practice, breaks encapsulation and makes the preset system code unmaintainable because it lacks a clearly defined API.

There is at present no way to modify the stock presets, short of editing defaultpresets.xml itself. Every update of that file will then erase your modifications. My proposal allows to keep your personal modifications in a separate file where it will be safe from updates. The code is very simple, I don't see much potential for bugs there. This is clearly a feature for power users, I don't think you'll have to support them. Most users will never need this. Maybe I should post an example of why some users might want this.

[marcello@…]

This is an example of why people would want to patch the defaultpresets.xml file.

I'm mapping in a bilingual (at places trilingual) area, so every name has to be entered 2 or 3 times, plus a calculated name that is a concatenation of all 3 languages. See below:

What I did here is: I wrote a chunk with the logic for 3 languages and then patched it into defaultpresets.xml. Now all stock dialogs in defaultpresets.xml display 3 name fields instead of just one. Without this feature I would have to change every single preset that contains a name.

Full disclosure: actually defaultpresets.xml needs to be changed for this to work out of the box, but the changes are minimal.

[stoecker]

Replying to marcello@…:

Because the old code is ugly and badly maintainable. All fields in the preset dialogs are initialized through java object introspection, and as a consequence all those fields are public. This is bad practice, breaks encapsulation and makes the preset system code unmaintainable because it lacks a clearly defined API.

Sorry, but that's no reason for me. If the new code has no clear benefit over the old code, but only "the looks ugly", then the patch has no chance to ever be applied.

There is at present no way to modify the stock presets, short of editing defaultpresets.xml itself.

Yes. And that's also how it should be.

[marcello@…]

Well ... its wasn't the 'ugly' that bothered me that much, but the 'unmaintainable'. Sometimes you need to refactor even if it introduces bugs in the short term. Then the bugs will be fixed and there'll be an easier road ahead.

If the defaultpresets.xml.local bothers you, I can remove it. It's only 40 lines of code with comments. I still believe it's a very useful feature for people that really need it, but I can always compile my own jar anyway.

[gaben]

Oh my... that's quite a patch!

I haven't looked it yet. Does it make improvements to something mentioned in #21228?

[GerdP]

I only looked at a small part. Are the changes to OpeningHourTest intended?

[marcello@…]

Replying to GerdP:

I only looked at a small part. Are the changes to OpeningHourTest intended?

Yes. All data access goes through a handler instead of going directly to the dataset. This way you can easily run checks on your edits before committing them to the dataset.

[taylor.smock]

Replying to marcello@…:

Yes. All data access goes through a handler instead of going directly to the dataset. This way you can easily run checks on your edits before committing them to the dataset.

I haven't done a deep dive into the patch yet, but when I was looking at #15217, I ended up trying to shoehorn the NSI into our current tagging system. I haven't applied that patch yet (I haven't been particularly happy about it). Will this make it easier to include different sources for pre-filling the preset, and possibly adding additional fields from the pre-fill to the preset window?

From skimming the patch:

• DataSetTaggingPresetHandler#createCommand: ChangePropertyCommand has a constructor that takes a collection of primitives and a map of tags. I recommend using that instead of creating a separate command for each tag pair. new TagMap will make the creation of a Map<String, String> easier.
• Please look for @since tags in files that you moved (e.g. Item from TaggingPresetItem -- use @since xxx (moved from TaggingPresetItem where it existed since xxx) or something like that). Also, since you are breaking binary and source compatibility, please look into making that hierarchy Java 14 record compatible (use interfaces instead of abstract classes, final class, final fields, and getters for the fields are the field name, e.g. final int id; public int id() {returns this.id;}). There is a JEP for value objects which may allow us to trim some memory during runtime (almost all record classes can be value classes, but not vice-versa -- in the current JEP, value classes can extend other abstract value classes, while record classes cannot extend anything).
• I'd strongly prefer the moving of files to be its own patch. It will make it easier to review actual changes, instead of just code moving around. This would be easier with git, but we use svn. :(
• There are some files in the patch that only have modified grammar. Please don't do this. I don't mind that in files that already have extensive modifications.
• You need to update the patch -- I got a lot of rejects when applying it to r18614
• If we end up accepting this patch, you will likely have to work with some plugins to fix breakage. For example, this will almost certainly break ​https://github.com/maripo/JOSM_easypresets (last commit > 1 year ago). It will also break some functionality in the Mapillary plugin (which I can fix, so don't worry about that).

Now, with all that said, I do kind of agree with stoecker that change for the sake of change isn't a good idea. If you have a long-term goal that this change makes possible/easier/more maintainable, it makes it easier to accept a rather large patch. If there are tickets that could be fixed by this change and cannot be fixed any other way, please add those to the description with why they cannot be fixed any other way.

I also think defaultpresets.xml.local is not a great idea. It would be better to detect the locality of the edit and add name:lang fields as necessary. We'd probably want to extend the preset XSD to allow us to specify localizable tags (possibly a pattern?) so we aren't hard-coding the localizable tags. For that, we'd probably want to coordinate with Vespucci. We probably have a ticket for that somewhere.

[Woazboat]

I'd love if presets were able to dynamically show/hide sections based on specified conditions (tag matches, ...). Presets being extensible would also be nice. That would solve the current mess where there are five different presets for one feature that all handle different subsets. I don't think these things would be easy to implement with the current preset system.

[Woazboat]

Matching presets being combined into one window (with collapsible subsections per preset) would be a nice start instead of having N separate preset windows.