#17327 closed defect (fixed)

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

Reported by: GerdP Owned by: michael2402
Priority: normal Milestone: 19.04
Component: Core Version:
Keywords: preferences Cc: michael2402

Description (last modified by Don-vip)

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.

Attachments (1)

17327.patch (1.1 KB) - added by GerdP 17 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 17 months ago by Don-vip

Cc: wiktorn added
Description: modified (diff)
Keywords: preferences added

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

comment:2 Changed 17 months ago by wiktorn

Cc: michael2402 added; wiktorn removed

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

Changed 17 months ago by GerdP

Attachment: 17327.patch added

comment:3 Changed 17 months ago by GerdP

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

comment:4 Changed 17 months ago by 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) {
public MapListSetting(List<Map<String, String>> value) {

The copy method will be much simpler then.

comment:5 Changed 17 months ago by GerdP

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

comment:6 Changed 17 months ago by GerdP

comment:7 Changed 16 months ago by GerdP

In 14827/josm:

see #17327: work around to allow SortedMap in MapListSetting

comment:8 Changed 16 months ago by GerdP

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.

comment:9 Changed 16 months ago by Don-vip

Milestone: 19.03

comment:10 Changed 16 months ago by Don-vip

Milestone: 19.0319.04

comment:11 Changed 16 months ago by Don-vip

Owner: changed from team to michael2402

comment:12 Changed 16 months ago by michael2402

What is still to solve for this ticket?

comment:13 Changed 15 months ago by 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.

comment:14 Changed 15 months ago by GerdP

Resolution: fixed
Status: newclosed

Guess we can open a new ticket if wanted

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain michael2402.
as The resolution will be set.
The resolution will be deleted.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.