Class MapListSetting doesn't allow to store a TreeMap in preferences

[GerdP]

Is that intended? The method consistencyTest() uses

            if (map.containsKey(null))
                throw new IllegalArgumentException("Error: Null as map key in preference setting");

which causes a NPE when used with a TreeMap no matter what the map contains.

[Don-vip]

I believe Wiktor plans to work on the preferences system :)

[wiktorn]

It was Micheal. I only planned to change imagery stuff in preferences. See #16869

[GerdP]

The patch fixes this particular problem. I think other map implementations also don't allow null values in keys?

[michael2402]

We cannot store null values in keys, this is the reason why we check if it is in the map.

You are right, containsKey() throws a NPE for some map implementations. Excluding only SortedMap instances won't be enough since we cannot tell if there is any other map. map.keySet().contains() has the same problem.

There are two options I see:
(1) Catch the NPE and handle that case as if null was not included (bad design)
(2) Loop through the map and check for every element, if it's key is null
(3) Imo best: Use a copy constructor (using Map.copyOf once we switch to java9+, to make this a Nop if the map is immutable). For now replace:

public MapListSetting(List<Map<String, String>> value) {
super(value);

public MapListSetting(List<Map<String, String>> value) {
super(value.stream().map(LinkedHashMap::new).map(Collections::immutableMap).collect(Collectors.toImmutableList());

The copy method will be much simpler then.

[GerdP]

I don't care much, performance is probably never a problem here. Change it to whatever suits best.

[GerdP]

See also ticket:17268#comment:55

[GerdP]

In 14827/josm:

see #17327: work around to allow SortedMap in MapListSetting

[GerdP]

@michael2402:
Your proposed changes don't compile with Java 1.8, so to get things going with #17268 I've committed my work around for now. Please improve once you find the time.

[michael2402]

What is still to solve for this ticket?

[GerdP]

With the current subject the ticket is fixed. If we change it to something like ".. doesn't allow to store all Map implementations" we have to keep it open.

[GerdP]

Guess we can open a new ticket if wanted