Modify

Opened 2 months ago

Closed 6 days ago

#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 2 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 2 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 2 months ago by wiktorn

Cc: michael2402 added; wiktorn removed

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

Changed 2 months ago by GerdP

Attachment: 17327.patch added

comment:3 Changed 2 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 2 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) {
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.

comment:5 Changed 2 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 2 months ago by GerdP

comment:7 Changed 7 weeks ago by GerdP

In 14827/josm:

see #17327: work around to allow SortedMap in MapListSetting

comment:8 Changed 7 weeks ago by 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.

comment:9 Changed 7 weeks ago by Don-vip

Milestone: 19.03

comment:10 Changed 3 weeks ago by Don-vip

Milestone: 19.0319.04

comment:11 Changed 3 weeks ago by Don-vip

Owner: changed from team to michael2402

comment:12 Changed 3 weeks ago by michael2402

What is still to solve for this ticket?

comment:13 Changed 3 weeks 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 6 days 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
Action
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.