Custom animation #199
Custom animation #199
Conversation
Also started to use a unit for rgb rather than System.Drawing.Color as more efficent and better serialisation
Added missing English resources Added Loop checkbox and moved buttons
There was a problem hiding this comment.
Overall do like the PR and changes are pretty good. For sure some editor clashes in terms of spacing we have to figure out. Please do look over the file changes yourself for the issues.
I could look into creating a .editorconfig on this branch if that would help you?
What does your development environment look like?
I've not functionally tested the PR yet, mostly cause its midnight while I'm writing this and partly cause my Nanoleafs fell down a while ago and I've been unable to hang them up again. Should be able to do a functional test next week using both the triagles and the canvas.
What devices have you tested the PR on?
| /// <summary> | ||
| /// A series of frames each specifying the color for each panel | ||
| /// </summary> | ||
| public IList<Frame> Frames { get; set; } |
There was a problem hiding this comment.
Personally prefer using either IEnumerable<T> or just List<T>, don't see a point in using IList<T>
There was a problem hiding this comment.
So IMHO IList<T> is preferable to List<T> so we're not forcing the specific implementation of List. IEnumerable may be okay if I'm not using any IList methods. Sometimes you can end up with LINQ having to provide functionality that is already there, an example is Count. IList provides a property for Count, IEnumerable doesn't, so if you need to count the items you have to get LINQ to iterate over the IEnumerable for you using Count() rather than just using the Count property that would be there if it were an IList.
The same is true for Any(), rather than Count > 0. This isn't as bad as Count() as Any() just needs to read one item from the IEnumerable, but it is still less efficient than the Count property.
I'll see what I break if I change to IEnumerable and get back to you.
There was a problem hiding this comment.
There is a lot of use of indexing, so it has to be List or IList. Personally I prefer IList as this means if we ever wanted to swap the implementation from List to a similar collection that still implements IList we would know it would definitely not break any existing code.
| /// </summary> | ||
| public partial class LayoutDisplayUserControl : UserControl | ||
| { | ||
| private static readonly SolidColorBrush _lockedBorderColor = Brushes.Red; |
There was a problem hiding this comment.
Odd how the formatting changed. Maybe it would be good for us to get more professional on the repository as well and add an .editorconfig @StijnOostdam
|
Also apologies if some comments don't make a lot of sense or are placed weirdly. I'm still not used to the GitHub PR system. If you feel you'd rather explain something or talk about issues in a more personal matter, feel free to ask me for some contact details. I'm also within the Nanoleaf Discord under the name "Bort". |
|
@bartdebever, thanks for taking the time to review my PR - much appreciated! I have made another commit that makes the changes for all the comments I have resolved in the feedback. I have also sent yo a friend request on Nanoleaf Discord as Stellarat. |
Reworked the code to fit the new architecture.
I've added a window to create custom animations that is based on the existing PercentageProfile work that was already done.
I added a button the the MainWindow to spawn it.
@bartdebever - hope I'm following process - first GitHub PR!