Closed sygmond closed 2 years ago
Thanks for the the PR. It looks good on first glance. I'll try it out tomorrow most likely and post back here.
Maybe a Test for ApplyDefaults() can be added, i.e. when 3 out of 5 Properties have a default value defined and not all.
ApplyDefaults() is using part of code from the Apply() method. It could be refactored so we don't duplicate code, if necessary.
@sygmond I'm curious about the MemoryStream issue. Would it not make sense to expose a byte array on the object that you're tracking? Conceptually, Jot takes a snapshot of static data from the object. A memory stream is a dynamic thing, it doesn't seem like something that belongs in a snapshot. I'm not saying it doesn't work, it's just that conceptually it seems a bit strange to save and restore a snapshot of a stream. On the one side, if this memory stream issue is common it might make practical sense to build the memory stream serialization into Jot. On the other hand, it seems like it might be too specific and easily worked around to warrant including in the library. Thoughts?
I changed it to a byte array, after doing some research that apply more to my case, but reading this Stackoverflow answer, I got to these conclusions, that one solution doesn't apply to all cases:
Stream
in memory often and not only on app start/exit, copying the Stream to byte array would result in duplicating data in memory, each time you do Stream -> byte array, instead of using the Stream itself.For example, Telerik for WPF is saving the state of the RadGridView via PersistenceManager that returns/accepts a Stream. Considering the above,
In my case, I need to save the state only on app start/exit, so I changed the property to byte array public byte[] GridViewState { get; set; }
and used a Stream extension for preserving the Telerik RadGridView to byte array instead of Stream:
public static class StreamExtensions
{
public static byte[] ToByteArray(this Stream stream)
{
using MemoryStream memoryStream = new();
stream.CopyTo(memoryStream);
return memoryStream.ToArray();
}
}
Just for reference, the GridViewExtensions class:
public static class GridViewExtensions
{
public static void FromStream(this RadGridView gridView, Stream gridViewStream)
{
if (gridViewStream?.Length > 0)
{
gridViewStream.Position = 0L;
PersistenceManager manager = new();
manager.Load(gridView, gridViewStream);
}
}
public static void FromByteArray(this RadGridView gridView, byte[] gridViewByteArray)
{
using MemoryStream stream = new(gridViewByteArray);
gridView.FromStream(stream);
}
public static Stream ToStream(this RadGridView gridView)
{
PersistenceManager manager = new();
manager.AllowCrossVersion = false;
return manager.Save(gridView);
}
public static byte[] ToByteArray(this RadGridView gridView)
{
Stream stream = gridView.ToStream();
return stream.ToByteArray();
}
}
Update: Removed the MemoryStream commit and added ILogger
Would be good to split up into separate MRs in the future but I like both changes. Thanks @sygmond !
I found myself needing the ability to restore the configured default values and also the ability to save a memory stream to Json (i.e. Telerik library is preserving the GridView settings in a stream).