Code review comment for lp:~samuel-buffet/entertainer/new-scrollmenu

Matt,

> motion_buffer.py:
>
> This is my understanding of MotionBuffer: A widget will have a private
> instance of MotionBuffer. When an event occurs, MotionBuffer will have some
> methods called to do some calculations. Once the calculations are done, the
> widget will access some properties of the buffer to determine what kind of
> motion happened and how to handle that motion. If this understanding is
> incorrect, please feel free to point out other details.

Yes this is precisely what this Class does. It's a tool Class added to widgets which need to computes cursor's motion characteristics (speeds / distances) in order to do a kinetic effect.

> Comments:
> * I think that the class should be a new style class (inherit from object)

Fixed.

> * I'm not a big fan of the class doc string. I know Paul has recommended to
> me in the past to stay away from "This class..." wording. Additionally, the
> description is pretty vague. What are "some calculations"? It's not a bad
> thing to have a longer docstring, especially if the class is more abstract
> like this one is.
> * init could use a comment describing what "ema" is.

Fixed. Yeah, that was a bit short. More detailed now.

> * compute methods docstrings are sentences so they each need a period. And it
> should be "deltas and speeds".

Fixed

> * scroll menu is using instance variables of motion buffer without using
> property methods. I know that there are a lot of instance variables here.
> Would it make sense to turn these into property methods?

Well, this is a point but I don't see the benefit.

I can understand:

@property
def foo(self):
    do_something
    return value

Because there we wrap a treatment before returning the value.

also I understand:

def _get_myproperty(self):
    return value

def _set_myproperty(self, value):
    self._value = value
    do_something

myproperty = property(_get_myproperty, _set_myproperty)

Because there we wrap a treatment setting the value.

But I don't see the added value of

@property
def foo(self):
    return value

In this case I think (I may be wrong) that it's cleaner, simpler and correct to access directly the variable itself.

> * Speaking of lots of instance variables, there seems to be a natural
> grouping of these variables. Would it help the class if those variable were
> grouped into their own class in a generic way. Maybe it would be a Moment
> class or something, and MotionBuffer would have three different instance of
> moments for start, last_motion_event, and ema. Then you could have a common
> compute method that would eliminate some duplicated code. I'm just trying to
> brainstorm here because I feel like I'm getting lost in a sea of instance
> variables. From the design perspective, this code seems to have a "smell"
> because of the long variable names. When you have a variable named
> "dy_from_last_motion_event," it makes me wonder if the design can be improved
> to create some important abstract concept.

Well, I don't see the need to add complexity there.

I've pushed the fixes mentioned *Fixed* above.

Thanks a lot Matt.
Samuel-