[Bug]: Default lights get re-added to campaign properties

[kwvanderlinde]

Describe the Bug

After removing "D20" and "Generic" lights from the Campaign Properties dialog and pushing "OK", the lights are added back again and can be selected from the menu.

To Reproduce

1. Start a new campaign.
2. Open the Campaign Properties dialog and go the Light tab.
3. Delete everything and leave only these contents:

   Test
   ----
   test: circle 5
   

4. Click the "OK" button.
5. Open the Campaign Properties dialog again and go to the Light tab.
6. Observe these contents:

   D20
   ----
   Candle - 5: circle 5 10#000000
   Lamp - 15: circle 15 30#000000
   Torch - 20: circle 20 40#000000
   Everburning - 20: circle 20 40#000000
   Lantern, Hooded - 30: circle 30 60#000000
   Sunrod - 30: circle 30 60#000000
   
   Generic
   ----
   5: circle 5
   15: circle 15
   20: circle 20
   30: circle 30
   40: circle 40
   60: circle 60
   
   Test
   ----
   test: circle 5
   

7. Click "Cancel"
8. Drop a token on the map.
9. Right click the token, go to "Light Sources" and notices all the D20 and Generic lights are present.

Expected Behaviour

Deleted lights should not show up again unless I add them back myself.

Screenshots

No response

MapTool Info

1.12.0 alpha 1

Desktop

Linux Mint 20.3

Additional Context

Testing shows that 1.11.5 does not have this issue.

[kwvanderlinde]

A bit more testing shows that if I changes the contents to:

Generic
----
5: circle 5

... then only the D20 lights get added back in. So it seems dependent on which sections are present.

[kwvanderlinde]

Did some debugging, and what's happening is that, when clicking "OK" in the Campaign Properties dialog

1. Light sources are cleared from the CampaignProperties.
2. Then lights sources from the dialog are added to the CampaignProperties

The problem is that the accessor (CampaignProperties.getLightSourcesMap()) will repopulate the map if empty, which is the case when step (2) runs. Then once step (2) has that populated map, it adds the new ones, thus merging the two together.

The check the accessor used to be against null, but the original protobuf changes (#3348) changed that field so it is non-null and the check was updated to check emptiness instead.

[thelsing]

The old behavior was to only create them when creating a new campaign?

[Phergus]

Yup.

[thelsing]

I think that this is already fixed

[Phergus]

Looks like it. Can't reproduce with current develop branch.