Hi Paul,
 
> > === modified file 'entertainerlib/frontend/gui/screens/main.py'
> > --- entertainerlib/frontend/gui/screens/main.py       2009-04-17 21:02:35
> +0000
> > +++ entertainerlib/frontend/gui/screens/main.py       2009-04-25 23:30:15
> +0000
> > @@ -47,16 +47,55 @@ class Main(Screen):
> >          self.preview.set_opacity(0x00)
> >          self.add(self.preview)
> >
> > -        self.active_menu = "main"
> >          self.rss_preview_menu = self._create_rss_preview_menu()
> >          self.preview.add(self.rss_preview_menu)
> >
> > -        self.menu = None
> > -        self.playing_item = None
> > -        self.create_main_menu()
> > +        self.menu = self._create_main_menu()
> > +        self.add(self.menu)
> > +        self.menu.connect('selected', self._on_menu_selected)
> > +        self.menu.connect('moved', self._on_menu_moved)
> > +        self.menu.connect('activated', self._main_menu_activation)
> > +        self.menu.active = True
> > +
> > +        self.add(ClockLabel(0.13, "screentitle", 0, 0.87))
> > +
> 
> This makes me a bit queesy.  Why are you creating an instance variable, and
> then calling an instance method on that instance variable.  What does self.add
> do? Why are we calling it?  If it is required (and I'm concerned it isn't),
> self._create_main_menu should be calling self.add().  Also, _create_main_menu
> should be doing all the self.menu.connect type stuff.

Well, this was discussed with Matt in a previous comment. The idea is to add widgets only in the __init__ method of a screen. I really don't know at the moment the *right* place of the connect calls. It's more tricky than it seems on the first looks. Because every actor has to be created before. Why? simply because if we create a widget (let's say a menu) and connect its *activated* signal to a *menu_activate* callback. In this call back we will deactivate (in the future when reactive widgets will be all together) the other reactive widgets of the screen. So if there not created ... error

That is why I think the good place for connections is in the end of the __init__ method. I'll take care of that (including other optimizations and simplifications) in the *reactive_global* branch I'll do in the end.

 
> Also, I see the clock got moved here instead of the _create_main_menu code.
> Why?

Yep, see above.

> 
> > +    def get_type(self):
> > +        """Return screen type."""
> > +        return Screen.NORMAL
> > +
> > +    def get_name(self):
> > +        """Return screen name (human readble)"""
> > +        return "Main"
> > +
> 
> I've seen this convention a few times.  What are we using get_name for?  Is
> there a better way to handle this than using a string?  I'd like some answers
> about this, because I thought Joshua had eliminated the need for this code.
> 
> Also, it seems that type and name would be better as properties.  They never
> change dynamically, so making them instance properties is a good idea.  If
> type
> or name had the ability to change within the life of the instance, we would
> use
> accessors, but that's more for convention than anything.  In this case though,
> there isn't even a need for defining a function.  This code could be
> simplified
> to:
> 
>     type = Screen.NORMAL
>     name = "Main"

I agree on the principle, there's simplification to be done. But I think that's not the goal of this branch.


> >              self.preview.add(self._create_rss_preview())
> > +            # when we update the rss preview menu, we must set it active if
> > +            # main menu is not
> > +            if not self.menu.active:
> > +                self.rss_preview_menu.set_active(True)
> >          else:
> >              update = False
> >
> 
> The comment here needs to start with a capital letter and end with a full
> stop.

Fixed.


> > +
> > +    def update(self):
> > +        """
> > +        Update screen widgets. This is called always when screen is poped
> from
> > +        the screen history. Updates main menu widget.
> > +        """
> > +        if self.media_player.is_playing() and \
> > +                (self.menu.get_index("playing") == -1):
> > +            self.menu.add_item(_("Playing now..."), "playing")
> > +        elif not self.media_player.is_playing() and \
> > +                (self.menu.get_index("playing") != -1):
> > +            self.menu.remove_item("playing")
> > +
> > +        self.menu.refresh()
> > +        self._update_preview_area()
> >
> 
> This docstring needs to be consistent with the rest of our new docstrings.  It
> needs to read like this:

Yes, in fact that was already modified a few days ago (don't remember exactly when). I should have repost a new diff before you review. Sorry.


> > === added file 'entertainerlib/frontend/gui/widgets/motion_buffer.py'
> > --- entertainerlib/frontend/gui/widgets/motion_buffer.py      1970-01-01
> 00:00:00 +0000
> > +++ entertainerlib/frontend/gui/widgets/motion_buffer.py      2009-04-25
> 23:30:15 +0000
> > @@ -0,0 +1,137 @@
> > +"""Tools for pointer motion calculations"""
> > +
> > +__licence__ = "GPLv2"
> > +__copyright__ = "2009, Samuel Buffet"
> > +__author__ = "Samuel Buffet <email address hidden>"
> > +
> 
> Please remove this.  I'm going to go through and remove all of these vars from
> every file.  Claiming ownership of a file can lead to fear from other people
> to
> (especially newbies) step into "your file" and work on it.

Well, here I have to disagree as it's just common practice and also because this is not related to the goal of that branch. I mean I have to focus a bit on the goal of my last set of branches.


> > +        self._alpha = 2.0 / (self._rank + 1)
> > +        self._beta = 1 - self._alpha
> > +
> 
> The public and private comments are unneeded, since the var illustrates its
> contract.

Yeah, I've removed those comments.


> However, I would like to see documentation for what all these vars
> are used for. 

I've added better docstrings and a link to explain what is a exponential moving average.
Also, I think I've taken long and readable variable names.


> I'm suspicious of the mass amounts of instance variables here.
> I'm concerned this means that MotionBuffer is going to consume larger amounts
> of memory than it really needs.

We have only one instance of this small Class per widget using a kinetic effect that means that in a near future we will have max 2 or 3 per screen so I don't see where's the issue here.


> It looks like this class needs to retain state.  We can probably think of
> better ways to do that then all of these properties.

They are retain in variables I don't see what kind of optimization I can do here.


> > +            self.speed_x_from_start = float(self.dx_from_start) / \
> > +                float(self.dt_from_start)
> > +            self.speed_y_from_start = float(self.dy_from_start) / \
> > +                float(self.dt_from_start)
> > +            self.speed_from_start = float(self.distance_from_start) / \
> > +                float(self.dt_from_start)
> > +        else:
> > +            self.speed_x_from_start = 0
> > +            self.speed_y_from_start = 0
> > +            self.speed_from_start = 0
> > +
> > +    def compute_from_last_motion_event(self, event):
> > +        '''Compute deltas an speeds from the last motion-event'''
> > +        self.dt_from_last_motion_event = event.time - \
> > +            self._last_motion_event_time
> > +        self.dx_from_last_motion_event = event.x -
> self._last_motion_event_x
> > +        self.dy_from_last_motion_event = event.y -
> self._last_motion_event_y
> > +        self.distance_from_last_motion_event = math.sqrt( \
> > +            self.dx_from_last_motion_event * self.dx_from_last_motion_event
> + \
> > +            self.dy_from_last_motion_event *
> self.dy_from_last_motion_event)
> > +
> 
> I consider the trailing '\' character to be generally evil.  You can span
> lines
> in python by wrapping the statement in ().


Well, here also I have to disagree because I think it's much better to have a \. Problem with () is when you wraps math expressions already using () with a mathematical sens. 

> That's how the rest of Entertainer
> does it.

Paul, we have tons of \ in our code (5 only in the main screen).


> > +
> > +    def _get_active(self):
> > +        """Active property getter"""
> > +        return self._active
> > +
> > +    def _set_active(self, boolean):
> > +        """Active property setter"""
> > +        if self._active == boolean:
> > +            return
> 
> 
> Wait, why are we using a private accessor to get a private variable?  Why not
> just access the private attribute wherever _get_active is called.  The same
> with _set_active.  Accessors are really only needed when you're trying hide
> implementation details from the user.  If _active is never accessed outside of
> the class, an accessor is unneeded.

The active property is/will be used on every screen and many times. In fact I changed that into a regular property. It was menu.set_active(True) before. But on all widgets I'm refactoring I change that into a property.
Also the accessor is needed because we have treatment on the getter. You've cut/past only a fragment of the full code.

"
    def _get_active(self):
        """Active property getter"""
        return self._active

    def _set_active(self, boolean):
        """Active property setter"""
        if self._active == boolean:
            return

        self._active = boolean
        if boolean:
            self.set_opacity(255)
            self.emit('activated')
        else:
            self.set_opacity(128)

    active = property(_get_active, _set_active)"


> > +
> > +    def test_create(self):
> > +        """Test correct ScrollArea initialization"""
> > +        self.assertTrue(isinstance(self.menu, ScrollMenu))
> > +
> 
> I'd like to see more tests, especially for the new classes.  There's probably
> a
> bit that just can't be tested, but the state tracking itself probably could be
> tested pretty well, to make sure it retains state properly.

Okay, so I've fixed and pushed the comment issue.

For the test, I have to think about that more. Because it's quite tricky to add a usefull test. I'll do something more (but I don't know yet what). I'll post something here when done.

Thanks for taking time to review that branch Paul.
Cheers,

Samuel-