mariusmuntean
commented
4 years ago This looks quite promising to me but also complex . At this moment I can't foresee all the implications this change will have (except the obvious breaking changes).
Do you have a branch were you're working? I'm interested to see how easy/complex it is to port some of the samples. I'd look into a simple one and a complex one. This has the advantage that it uncovers flaws or weaknesses pretty immediately. It also gives you the library user's perspective.
Another suggestion I have would be to support the library user as much as possible, but never restrict them. Inevitably we'll restrict the user so it'd be helpful to provide an "escape hatch", by that I mean a way of circumventing our abstractions to allow for unforeseen usages.
Joelius300
MindSwipe
Describe the feature request
Currently the datasets are all over the place. Some have base classes, some don't. Mixable charts use the covariant
IMixableDataset<T>which allows for different types of datasets in one chart but with many downsides. I once was really proud of my implementation of those mixable datasets (when I started learning about covariance) but let's face it, I did horribly. Having to use wrappers for all value types is really bad and extending this thing seems like a nightmare. Now with the recent interop-layer rework (not #70, that one 4 month ago), we need to have an id for each dataset so now we also have anIDatasetinterface that ensures that we have an accessable id.The datasets are stored in
config.data.dataset. This is usually a collection (eitherListorHashSet) containing objects of typeIMixableDataset<object>or some specific class likeBubbleDataset. For charts that only support one dataset-type with one data structure, using aListalong with a specific class, works fine. However, things get complicated once you're dealing with charts like the line chart. The line chart allows for different types of datasets (it's perfectly legal to have a bar dataset in a line chart) and the line chart also accepts data in different forms (array of numbers, array of number-points, array of time-points). These have to somehow be mixable without breaking typesafety and without allowing you to add other types of datasets.Which charts does this feature request apply to?
All charts
Describe the solution you'd like
I'd like the datasets and the collection of datasets to be enjoyable to use, performant and extendable.
Enjoyable to use
Dataset
ArrayListorList<object>or anything of the sorts. If you can modify it, it should be in a typesafe manner.IList<T>. Until recently (and maybe even now) some datasets don't really allow modification but I'd like to have all the methods ofIList<T>and as a bonus alsoAddRange.Wrap()extension methods are helpful but it's still not great.Dataset collection
LineDataset<string>to a line chart because it doesn't support string values. This needs to be a compilation error, not aNotSupportedException!IList<IDataset>but preventing the user from adding anyIDataset. Instead theAddmethods are exposed with only supported overloads. Anything else throws aNotSupportedException(or better, a compilation error, if possible).Performant
That's actually not that big of a deal considering the bottleneck in this library is likely the usage of reflection and dynamic features but I have not tested it. Still it would be nice if we weren't in boxing hell like we are currently with
IMixableDatasetand the wrappers.Extendable
We should split these datasets into interfaces and base classes so we can extend and modify them if needed. After all we are modeling a JavaScript library so it's definitely of value to be flexible. We don't want to get rid of typesafety (!) but being able to extend the model from outside without heavy reflection usage is great for something like this.
API proposal
I have already implemented the base system and this time I think I can actually be proud of it.
It's entirely typesafe but still flexible, raises compiler errors when you try to add a not supported dataset, has full support for structs, allows for object and collection initializers and implements
IListas best as possible.There is the base interface
IDataSet. This interface has no type associated with it. It's more a semantic restriction than anything else but it does contain the most important properties beingIdandType. It's also important when storing datasets since it's the base of any dataset.Implementing that base interface is the generic version
IDataSet<T>whereTcan be any type. Noinoroutmodifiers apply. This interface also assures that implementers need to expose a read-only list ofT.Then the base class for all the datasets is
DataSet<T>. This is a class that implementsIDataSet<T>andIList<T>for modification. It exposes the contents through a read-only property and thereby implementsIDataSet<T>.Data. It also contains theAddRangemethods we love.Now the dataset collections. In many cases we can just use
List<BubbleDataset>or something similar so we don't want too much abstraction.For charts that support more than that, we have the handy class
DatasetCollection. It implementsIList<IDataset>so you can do modifications as you please with the exception of addingIDataset. TheAddandInsertmethods are implemented explicitly (and therefore can't be called unless casted) and throw aNotSupportedExceptionif used. However, they expose theprotectedmethodAddDatasetwhich can add any dataset. This is the only way (disregarding reflection) to add datasets to this collection.Now we can derive from that collection and add our own
Addmethods. TheAddmethod is overloaded for every supported dataset. In the case of the line chart, this means every dataset that consists of eitherints,longs,doubles,Points orTimePoints. For each possibility there is anAddmethod. Not only is the name intuitive, it's also the key to collection initializers. They search for an overload ofAddwhen the base implementsIEnumerable(whichDatasetCollectionalready does). We can add some abstraction to those collections like theNumberDatasetCollectionand theNumberAndPointDatasetCollectionbut for now, there aren't interfaces likeINumberDatasetCollectionandIPointDatasetCollection. You'd still need to implement both so it's not really useful unless we start using composition but then things get even more complex.And here's the code
``` public interface IDataset { string Id { get; } // for interop string Type { get; } // for mixed charts } // this is actually implemented and the dataset collections // restrict to IDataset, IDataset through their Add methods
public interface IDataset : IDataset, IList
{
IReadOnlyList Data { get; }
}
```
This is the base implementation for every dataset.
```
[JsonObject]
public abstract class Dataset : Collection, IDataset
{
/// Data { get; }
/// ())
{
Data = new ReadOnlyCollection(Items);
Id = id ?? Guid.NewGuid().ToString();
Type = type;
}
public void AddRange(IEnumerable items) => ((List)Items).AddRange(items ?? throw new ArgumentNullException(nameof(items)));
public void AddRange(params T[] items) => AddRange(items as IEnumerable);
public override bool Equals(object obj) => obj is Dataset set &&
Id == set.Id &&
EqualityComparer>.Default.Equals(Items, set.Items);
public override int GetHashCode() => HashCode.Combine(Items, Id);
public static bool operator ==(Dataset left, Dataset right) =>
EqualityComparer>.Default.Equals(left, right);
public static bool operator !=(Dataset left, Dataset right) => !(left == right);
}
```
And here is how you can store those datasets.
```
// Supports every operation of IList except for adding and inserting.
// There are protected methods for implementors but only the supported IList
// members are implemented implicitly. Those don't show up in code complete unless
// you cast it to IList in which case they will throw a NotSupportedException.
// Also since the Add methods take precedence the more concrete they are and ILists Add
// is hidden, you can use the Collection Initializer with different types (check Program.cs it's AWESOME)
public abstract class DatasetCollection : IReadOnlyList, IList
{
private const string NotSupportedMessageModificationThroughInterface =
"This collection doesn't support adding datasets through the IList or ICollection interface.";
private readonly List _datasets;
[JsonIgnore]
public int Count => _datasets.Count;
[JsonIgnore]
public bool IsReadOnly => false;
IDataset IList.this[int index]
{
get => this[index];
set => ThrowNotSupported();
}
public IDataset this[int index] => _datasets[index];
protected DatasetCollection()
{
_datasets = new List();
}
public bool Contains(IDataset dataset) => _datasets.Contains(dataset ?? throw new ArgumentNullException(nameof(dataset)));
public void CopyTo(IDataset[] array, int index) => _datasets.CopyTo(array, index);
public IEnumerator GetEnumerator() => _datasets.GetEnumerator();
public int IndexOf(IDataset dataset) => _datasets.IndexOf(dataset ?? throw new ArgumentNullException(nameof(dataset)));
protected void AddDataset(IDataset dataset) => _datasets.Add(dataset ?? throw new ArgumentNullException(nameof(dataset)));
protected void InsertDataset(int index, IDataset dataset) => _datasets.Insert(index, dataset ?? throw new ArgumentNullException(nameof(dataset)));
public bool Remove(IDataset dataset) => _datasets.Remove(dataset ?? throw new ArgumentNullException(nameof(dataset)));
public void RemoveAt(int index) => _datasets.RemoveAt(index);
public void Clear() => _datasets.Clear();
IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_datasets).GetEnumerator();
void IList.Insert(int index, IDataset item) => ThrowNotSupported();
void ICollection.Add(IDataset item) => ThrowNotSupported();
private void ThrowNotSupported() => throw new NotSupportedException(NotSupportedMessageModificationThroughInterface);
}
```
/// Gets the id used on interop-level. ///
public string Id { get; } ////// Gets the data contained in this dataset. This property is read-only. ///
public IReadOnlyList/// Gets the type of this dataset. Important for mixed charts. ///
public string Type { get; } public Dataset(string type = null, string id = null) : base(new ListNow for some examples, shall we?
This is the simplified dataset for a line chart. Fully generic, supports value types and has the type already assigned. If you implement a new line-like chart, you can derive from this class and use the protected constructor to inject your own type. ``` public class LineDataset : Dataset
{
public LineDataset() : this("line")
{
}
protected LineDataset(string type) : base(type)
{
}
public int[] BorderDash { get; set; }
public int? PointBorderWidth { get; set; }
public int? PointHoverRadius { get; set; }
public bool? Fill { get; set; }
public double? LineTension { get; set; }
public bool? SpanGaps { get; set; }
}
```
In order to store all the datasets a line chart supports, we also need a `DatasetCollection` with the appropriate `Add` methods. We could just write those all in one but it makes more sense to get a certain degree of abstraction which helps implementing other charts such as the bar chart.
```
public class NumberDatasetCollection : DatasetCollection
{
public void Add(IDataset dataset) => AddDataset(dataset);
public void Add(IDataset dataset) => AddDataset(dataset);
public void Add(IDataset dataset) => AddDataset(dataset);
}
```
```
public class NumberPointDatasetCollection : NumberDatasetCollection
{
public void Add(IDataset dataset) => AddDataset(dataset);
public void Add(IDataset> dataset) => AddDataset(dataset);
public void Add(IDataset> dataset) => AddDataset(dataset);
public void Add(IDataset> dataset) => AddDataset(dataset);
}
```
What's missing is the `LineData` class. It just contains the correct dataset collection and the labels. The labels are only serialized when they contain data but they're still get-only.
```
public class LineData
{
public List Labels { get; } = new List();
[JsonProperty("xLabels")]
public List XLabels { get; } = new List();
[JsonProperty("yLabels")]
public List YLabels { get; } = new List();
// Supported: https://www.chartjs.org/docs/latest/charts/line.html#data-structure
public NumberPointDatasetCollection Datasets { get; } = new NumberPointDatasetCollection();
[Obsolete("json.net", true)]
public bool ShouldSerializeLabels() => Labels.Count > 0;
[Obsolete("json.net", true)]
public bool ShouldSerializeXLabels() => XLabels.Count > 0;
[Obsolete("json.net", true)]
public bool ShouldSerializeYLabels() => YLabels.Count > 0;
}
```
The bar example is similar but a bit more specific.
```
public class BarDataset : Dataset
{
public BarDataset(bool horizontal = false) : this(horizontal ? "horizontalBar" : "bar")
{
}
protected BarDataset(string type) : base(type)
{
}
public double? BarPercentage { get; set; }
public double? CategoryPercentage { get; set; }
}
```
```
public class BarDatasetCollection : NumberPointDatasetCollection
{
public void Add(IDataset dataset) => AddDataset(dataset);
}
```
```
[JsonConverter(typeof(FloatingBarPointConverter))]
public readonly struct FloatingBarPoint : IEquatable
{
public readonly double Start, End;
public FloatingBarPoint(double start, double end)
{
Start = start;
End = end;
}
public override bool Equals(object obj) => obj is FloatingBarPoint point && Equals(point);
public bool Equals(FloatingBarPoint other) => Start == other.Start && End == other.End;
public override int GetHashCode() => HashCode.Combine(Start, End);
public static bool operator ==(FloatingBarPoint left, FloatingBarPoint right) => left.Equals(right);
public static bool operator !=(FloatingBarPoint left, FloatingBarPoint right) => !(left == right);
}
internal class FloatingBarPointConverter : JsonConverter
{
public override FloatingBarPoint ReadJson(JsonReader reader, Type objectType, FloatingBarPoint existingValue, bool hasExistingValue, JsonSerializer serializer)
{
//todo
throw new NotImplementedException();
}
public override void WriteJson(JsonWriter writer, FloatingBarPoint value, JsonSerializer serializer)
{
writer.WriteStartArray();
writer.WriteValue(value.Start);
writer.WriteValue(value.End);
writer.WriteEndArray();
}
}
```
```
public class BarData
{
public List Labels { get; } = new List();
// Supported: https://www.chartjs.org/docs/latest/charts/bar.html#data-structure
public BarDatasetCollection Datasets { get; } = new BarDatasetCollection();
[Obsolete("json.net", true)]
public bool ShouldSerializeLabels() => Labels.Count > 0;
}
```
Another example is for the bubble chart. This one is less specific (both spectrums are well supported).
```
public class BubbleDataset : Dataset
{
public BubbleDataset() : this("bubble")
{
}
protected BubbleDataset(string type) : base(type)
{
}
public int? Rotation { get; set; }
public int? Radius { get; set; }
}
```
```
public readonly struct BubblePoint : IEquatable
{
public readonly double X, Y, R;
public BubblePoint(double x, double y, double r)
{
X = x;
Y = y;
R = r;
}
public override bool Equals(object obj) => obj is BubblePoint point && Equals(point);
public bool Equals(BubblePoint other) => X == other.X && Y == other.Y && R == other.R;
public override int GetHashCode() => HashCode.Combine(X, Y, R);
public static bool operator ==(BubblePoint left, BubblePoint right) => left.Equals(right);
public static bool operator !=(BubblePoint left, BubblePoint right) => !(left == right);
}
```
```
public class BubbleData
{
public IList Datasets { get; } = new List();
}
```
And here's all the rest I have/you need.
```
public readonly struct Point : IEquatable
{
public readonly double X, Y;
public Point(double x, double y)
{
X = x;
Y = y;
}
public override bool Equals(object obj) => obj is Point point && Equals(point);
public bool Equals(Point other) => X == other.X && Y == other.Y;
public override int GetHashCode() => HashCode.Combine(X, Y);
public static bool operator ==(Point left, Point right) => left.Equals(right);
public static bool operator !=(Point left, Point right) => !(left == right);
}
```
```
public readonly struct TimePoint : IEquatable>
{
[JsonProperty("t")]
public readonly DateTime Time;
public readonly T Y;
public TimePoint(DateTime time, T y)
{
Time = time;
Y = y;
}
public override bool Equals(object obj) => obj is TimePoint point && Equals(point);
public bool Equals(TimePoint other) => Time == other.Time && EqualityComparer.Default.Equals(Y, other.Y);
public override int GetHashCode() => HashCode.Combine(Time, Y);
public static bool operator ==(TimePoint left, TimePoint right) => left.Equals(right);
public static bool operator !=(TimePoint left, TimePoint right) => !(left == right);
}
```
```
// This contract resolver is necessary because `Collection` (the base of) `Dataset`, has a non-virtual `Count` property
// this property always gets serialized and can't be influenced by `JsonIgnoreAttribute` or `ShouldSerializeCount`.
// This contract resolver is currently the only way I konw to get rid of that `Count` property and also to keep the possibility
// of having a `Count` property in a dataset as an options (which should get serialized, but currently there's not even a use-case for that).
internal class IgnoreDatasetCountContractResolver : DefaultContractResolver
{
protected override IList CreateProperties(Type type, MemberSerialization memberSerialization)
{
IList baseProps = base.CreateProperties(type, memberSerialization);
if (typeof(IDataset).IsAssignableFrom(type))
{
string countName = nameof(ICollection.Count);
if (NamingStrategy != null)
{
countName = NamingStrategy.GetPropertyName(countName, false);
}
foreach (var prop in baseProps)
{
if (prop.PropertyName == countName &&
prop.DeclaringType.IsGenericType &&
prop.DeclaringType.GetGenericTypeDefinition() == typeof(Collection<>))
{
prop.Ignored = true;
break;
}
}
}
return baseProps;
}
}
```
```
class Program
{
static void Main(string[] args)
{
foreach (object sampleData in GetSampleData())
{
string serialized = JsonConvert.SerializeObject(sampleData, Formatting.Indented, JsonSerializerSettings);
Console.WriteLine(serialized + Environment.NewLine);
}
Console.ReadLine();
}
private static IEnumerable
Additional context
PointandTimeTupleand many other classes as structs as seen here. They might need some tweaking but are already improved a lot over the current classes.Nullable<T>(for the defaults, issue incoming). We'll have to see.Final words
Although I already thought this issue through quite a bit, I'm not sure on all the specifics yet. Please tell me anything you see wrong with this, potential pitfalls, missing features, etc.
I'd love to hear feedback on this!