[FEATURE]-Additional improvements to Feedbacks

[ngenovese11]

Is your feature request related to a problem? Please describe. The current feedback paradigm requires you to maintain state externally of the Feedback and pass in a valueFunc at construction to notify listeners of a value change. This process is tedious, especially since you have access to the last state already baked into the Feedback. I'd propose passing in a function to the FireUpdate method. This way you wouldn't have to manage state externally, and you could update state based on context. This is partly inspired by #743

Describe the solution you'd like

pseudo code for a bool feedback class:

        public void FireUpdate(Func<bool> valueFunc)
        {
            bool newValue = valueFunc();
            if (newValue != _BoolValue)
            {
                _BoolValue = newValue;
                LinkedInputSigs.ForEach(s => UpdateSig(s));
                LinkedComplementInputSigs.ForEach(s => UpdateComplementSig(s));
                OnOutputChange(newValue);
            }
        }

        public void FireUpdate(Func<bool, bool> valueFunc)
        {
            bool newValue = valueFunc(_BoolValue);
            if (newValue != _BoolValue)
            {
                _BoolValue = newValue;
                LinkedInputSigs.ForEach(s => UpdateSig(s));
                LinkedComplementInputSigs.ForEach(s => UpdateComplementSig(s));
                OnOutputChange(newValue);
            }
        }

Describe alternatives you've considered An entirely new class and update the LinkInputSig extensions

[andrew-welker]

Do you have a use case for something like this? I'm not sure I'm a fan of state NOT being managed by a device class. There are also other things that access state outside of feedbacks on occasion.

[ngenovese11]

pseudo code:

OnStringReceivedFromAGather(args) { PowerFeedback.FireUpdate(ProcessPowerFeedback(args.Text)) }

static bool ProcessPowerFeedback(string) { return a new power state }

[jtalborough]

I have to agree with @ngenovese11 it offers a cleaner way to update feedbacks. Its also backwards compatible enhancing feedbacks but still allows for the current methods as well.

[TrevorPayne]

I find it exceptionally hard to argue with an enhancement that is fully backwards compatible while offering additional flexibility.

[ngenovese11]

Also a method to update the ValueFunc was just added, this seems like the next logical step to that. How is the below any different:

OnStringReceivedFromAGather(args) { var newPower = ProcessPowerFeedback(args.Text); PowerFeedback.SetValueFunc(() => newPower); PowerFeedback.FireUpdate() }

static bool ProcessPowerFeedback(string) { return a new power state }

[TrevorPayne]

Yeah - I hadn't seen that one - that seems far more egregious if we're worried about the device being the source of truth because it allows you to utterly decouple the feedback from the value you're already tracking without context.

[ngenovese11]

#743

[andrew-welker]

I believe (@ndorin can correct me if I'm wrong) we added the SetValueFunc to allow for changing the source of the state value that's provided when FireUpdate is called because we had a need for that with the EssentialsPartitionManageer that was added to enable room combining in Essentials. The state still isn't being maintained in the Feedback class, just the source of the state, either directly from a partition sensor or from a variable set manually from a UI. And, as I'm thinking about it, we probably could have achieved the same thing using a getter instead of changing the feedback func on the fly.

While I think we can go ahead and add this as it just extends on the SetValueFunc added previously, I'm worried we're opening Pandora's box that might make things harder to debug and troubleshoot later on.

Conceptually, the feedbacks are like the selectors in the Redux pattern I've come to know and love while working with NgRx in Angular. I think they should be pure functions, in the sense that if you feed in the same value, you get the same value out every time without any side effects. Little different in practice, because of the nature of the feedbacks and trilist stuff.

One other thing to consider: as we move more towards standalone Essentials with HTML5 UI's, there is less and less of a need for trilist-based communications. It becomes easier to serialize a class and its properties entirely, without relying on our existing feedback classes to provide state to HTML5 UI's.

[ngenovese11]

Our goals are aligned @andrew-welker. Obviously FireUpdate is not pure as it doesn't return anything and there are tons of side effects linked to it. The idea would be that you pass in a pure function (the reducer), so that you can work with existing state in a pure manner.

[ngenovese11]

Also don't discount the value of the feedback class even outside of the context of trilists. At its heart its something that maintains a primitive value and then fires an update when it changes. This functionality on its own is still very useful, and it probably wouldn't be too much of an effort to extend it to non primitive values provided that the state being managed implements IEquateable<T>

[andrew-welker]

Well...selectors also have side effects linked to them when they update. The selector takes in the immutable state object and gets some piece of it out. The FireUpdate is more akin to Observable.next that gets called when the state changes. The selector analog for Feedbacks is the function being passed in to determine the value that goes out when FireUpdate is called. May be taking the analogy too far here...

Thinking about it a little more...I'm wondering if it would be better to add an overload that just takes in a value instead of a func. You could accomplish the same thing , but still maintain the state in the device class.

[ngenovese11]

I have thought about this as well, but I think that passing in Func's gives you more flexibility (especially with the Func<bool, bool> parameter). I also think that working with funcs as parameters is cleaner and more expressive but no need to go down that rabbit hole.

[jtalborough]

I would say both...have the ability to fireUpdate(value) and fireUpdate(func)

[ndorin]

I'm going to drag in @hvolmer as he put a lot of time into the original concept of the Feedback class.

These are good suggestions for improvements and should certainly be considered. We just need to make sure that as we think of ways to expand the feature set of Feedback we don't fundamentally change it's intended operation or use.

The suggestion made of adding an overload to FireUpdate() that could take in a value as the new value needs to be very carefully considered. @hvolmer can add more historical context, but I believe that this class was designed specifically to ensure that when FireUpdate() was called, it was FORCED to always run its internal Func to return the absolute latest value. This matters when values might be rapidly changing at runtime.

Allowing the Func to be changed after construction of a Feedback doesn't break this, but overriding FireUpdate() to allow a new value to be passed in might. Let's make sure we fully consider the potential tradeoffs of this proposed enhancement.

[jtalborough]

If fireUpdate(func) or fireUpdate(var) is called its still FORCED to run with the absolute latest value. The developer called it to do so. The ability to set a func reference in the feedback still remains as well.

There's nothing stopping a dev from calling SetValueFunc(newFunc) immediately followed by FireUpdate(), what's the difference with FireUpdate(newFunc)?

This is a helper method to throw around primitives. It should be as flexible and accessible as possible IMHO.

[ngenovese11]

Not to derail this, but @ndorin 's concerns about thread safety are valid. We'd need something like the below to deal with it.

public class ThreadsafeBoolFeedback
{
  public readonly bool Value;
  public readonly BoolFeedback Feedback;

  public ThreadsafeBoolFeedback()
  {
    Feedback = new Feedback(() => Value);
  }

 public ThreadsafeBoolFeedback(value, feedback)
  {
    Feedback = feedback;
   Value = value;
   feedback.SetValueFunc(() => Value);
   Feedback.FireUpdate()
  }

   public static ThreadsafeBoolFeedback Reduce(this BoolFeedback state, func<bool> reduce)
   {
     var newValue = reduce();
    return new BoolFeedbackState(newValue, state.Feedback);
   }
}

[TrevorPayne]

Is the threadsafe argument valid for Feedbacks at large, or only in the context of this proposed enhancement?

[ndorin]

I suggest we have a quick call about this. After reviewing with @hvolmer I was able to confirm my concern about the original intended function of the Feedback class.

There are definitely quality of life improvements that can be made, but we want to make sure everyone understands why the Feedback is designed as it currently is.

[andrew-welker]

If fireUpdate(func) or fireUpdate(var) is called its still FORCED to run with the absolute latest value. The developer called it to do so. The ability to set a func reference in the feedback still remains as well.

There's nothing stopping a dev from calling SetValueFunc(newFunc) immediately followed by FireUpdate(), what's the difference with FireUpdate(newFunc)?

This is a helper method to throw around primitives. It should be as flexible and accessible as possible IMHO.

I think the difference as Nick proposed it is that passing a func into the FireUpdate call is a one-time thing. That func is used for that specific FireUpdate call and never used again, and a standard FireUpdate call would then result in possibly a different value being sent out.

Using the SetValueFunc -> FireUpdate sequence results in all subsequent FireUpdate calls using the func set through SetVauleFunc until SetValueFunc is used to change it again.

[ngenovese11]

so?

[jtalborough]

It could be possible that the only time FireUpdate is called its called as FireUpdate(func)...its the developers choice.

[andrew-welker]

so?

I was more answering Jason's question of what the difference between using the SetValueFunc -> FireUpdate vs FireUpdate(someFunc) was.

The more I'm thinking about it, the more I'm thinking that the better route is adding a FireUpdate(T value) method. In this scenario, there's no real reason that the FireUpdate method has to go run a Func. If/when something throws an exception in the func that's passed in, figuring out where in the chain, especially for more inexperienced developers, is going to be painful. You get the same benefit of bypassing the func passed in through the constructor, with more readability and easier troubleshooting.

[andrew-welker]

Also, for any interested parties, you can join our Zoom call at 2 PM EDT at the link below:

https://pepperdash.zoom.us/j/83548462062?pwd=M2FrdFZXY3BpRkRIek9LbG1zb3Vhdz09

[TrevorPayne]

That's 4 EDT, I think.

[ngenovese11]

so?

I was more answering Jason's question of what the difference between using the SetValueFunc -> FireUpdate vs FireUpdate(someFunc) was.

The more I'm thinking about it, the more I'm thinking that the better route is adding a FireUpdate(T value) method. In this scenario, there's no real reason that the FireUpdate method has to go run a Func. If/when something throws an exception in the func that's passed in, figuring out where in the chain, especially for more inexperienced developers, is going to be painful. You get the same benefit of bypassing the func passed in through the constructor, with more readability and easier troubleshooting.

In the original paradigm, If/when the func passed into the constructor throws how would it be any different?

[jtalborough]

Just to wrap this up and document our discussions:

1. We will proceed implementing a variation of feedbacks by either wrapping the class or recreating the class but not manipulating the base feedbacks classes.
2. We will create an interface that describes feedbacks both as they exist today and future iterations.

Also we discussed some criteria for features worth documenting:

1. New features should be functional
2. New features should not manipulate core functionality. Its better to wrap or rewrite.
3. Backwards compatibility must be maintained unless implemented on the new dev breaking changes branches.