fix bug merging themes

[haakon-e]

deepcopy correctly handles copying of non-toplevel attributes like Theme(Axis = (; titlesize=10)), otherwise leftmost argument to merge is modified.

Description

Fixes the following bug in which theme1 is being modified by merge:

julia> theme1 = Theme(Axis = (; titlesize = 10))
Attributes with 1 entry:
  Axis => Attributes with 1 entry:
    titlesize => 10

julia> theme2 = Theme(Axis = (; xgridvisible = false))
Attributes with 1 entry:
  Axis => Attributes with 1 entry:
    xgridvisible => false

julia> merge(theme1, theme2)
Attributes with 1 entry:
  Axis => Attributes with 2 entries:
    titlesize => 10
    xgridvisible => false

julia> theme1
Attributes with 1 entry:
  Axis => Attributes with 2 entries:
    titlesize => 10
    xgridvisible => false

Type of change

Delete options that do not apply:

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Added an entry in CHANGELOG.md (for new features and breaking changes)
• Added or changed relevant sections in the documentation
• Added unit tests for new algorithms, conversion methods, etc.
• Added reference image tests for new plotting functions, recipes, visual options, etc.

[ffreyer]

Thanks, this is something I've been unhappy with too.

I think we'll need to review this rather carefully since it has the potential to subtly change which attributes get picked. I'm not sure if I trust CI to pick up on that, though maybe I'm underestimating it.

We may also want to consider this breaking since users might already use merge

[haakon-e]

We may also want to consider this breaking since users might already use merge

That's fair. Though merge (without !) should promise to not modify input arguments, so I feel this is somewhat more clearly "bugfix" than "breaking feature".

In any case, I'd enjoy seeing this fixed as it was causing some reproducibility issues in my code, and in the end, this (+#4419) made it hard to combine Themes in a reasonable way.

[SimonDanisch]

I don't expect many people to use merge and I also don't expect anyone to rely on this behavior.
So I'm happy to merge this for the next patch release.

[ffreyer]

I looked through all the merge functions we call. As far as I can tell we always rely on merge!() for Attributes/Themes, or cast to Dict beforehand. So I think this shouldn't cause any issues