Tighten implementation of CssClassHolder - make any access to a zone's... (OSOE-590)

[0liver]

... class collection persistent in the private backing field.

See https://github.com/Lombiq/Orchard-Base-Theme/blob/dev/Lombiq.BaseTheme/Services/CssClassHolder.cs#L24 - here, an empty HashSet<string> is returned, which can then be modified by the caller, but won't be persisted for subsequent access to the same zone's class collection.

Jira issue

[DemeSzabolcs]

but won't be persisted for subsequent access to the same zone's class collection.

But if I understand correctly the detached empty HashSet<string> is only returned, when the given zone doesn't exist. Therefore, there is no zone where we could save it/ where it could persist.

If there are no classes on the zone it should still find the zone's key and return the empty HashSet<string> that is attached to the zone. Or am I missing something?

[sarahelsaig]

I think it would be the best to deprecate CssClassHolder.GetZoneClasses() and add a CssClassHolder.GetOrAddZoneClasses() instead. The name change would be important for clarity. Hastlayer had a similar method here: https://github.com/Lombiq/Hastlayer-SDK/blob/dev/Hast.Common/Extensions/DictionaryExtensions.cs#L5

[0liver]

I have the following ideas:

Indexer

I could imagine simply using an indexer with the zone name as the key:

public ISet<string> this[string zoneName] => _zones.GetMaybe(zoneName) ?? (_zones[zoneName] = new());

That can be used to .Add() and .Remove() classes, and also to just access them.

Fluent interface

Another option would be to make .AddClassToZone() fluent by returning the ICssClassHolder instance from it, and add an appropriate .Remove() method. Any .Get() method should simply return an IEnumerable<string>.