Samuel-

  This is a very large patch, which makes it difficult to truly review well.
I've asked lots of questions, and hope that through those questions, you might
be able to evaluate the implementation from another viewpoint, and discover
issues on your own.  I'm not going to vote just yet, as I'm curious to hear
your response to my review.


> === 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.

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


> +    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"


> @@ -206,42 +200,23 @@ class Main(Screen):
>  
>          return preview
>  
> -    def get_type(self):
> -        """Return screen type."""
> -        return Screen.NORMAL
> -
> -    def get_name(self):
> -        """Return screen name (human readble)"""
> -        return "Main"
> -
> -    def update(self):
> -        """
> -        Update screen widgets. This is called always when screen is poped from
> -        the screen history. Updates main menu widget.
> -        """
> -        selected_name = self.menu.get_selected().get_name()
> -
> -        self.remove(self.menu)
> -        self.create_main_menu()
> -
> -        index = self.menu.get_index(selected_name)
> -        if not index == -1:
> -            self.menu.select(index)
> -
> -        self.menu.update()
> -
> -        self._update_preview_area(self.menu.get_selected())
> -
> -    def _update_preview_area(self, item):
> +    def _update_preview_area(self):
>          '''Update the preview area to display the current menu item.'''
>          self.preview.remove_all()
> +        item = self.menu.get_selected()
> +
>          self.preview.set_opacity(0x00)
>  
>          update = True
> +
>          if item.get_name() == "playing":
>              self.preview.add(self._create_playing_preview())
>          elif item.get_name() == "rss":
>              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.


> @@ -253,61 +228,51 @@ class Main(Screen):
>              self.behaviour.apply(self.preview)
>              fade_in.start()
>  
> -    def _move_menu(self, menu_direction):
> -        '''Move the menu in the given direction.'''
> -
> -        if menu_direction == self.UP:
> -            scroll_direction = self.menu.scroll_down
> -            preview_direction = TextMenu.UP
> -        elif menu_direction == self.DOWN:
> -            scroll_direction = self.menu.scroll_up
> -            preview_direction = TextMenu.DOWN
> -
> -        if self.active_menu == "main":
> -            scroll_direction()
> -            selected = self.menu.get_selected()
> -            if selected.get_name() == None:
> -                scroll_direction()
> -                selected = self.menu.get_selected()
> -            self._update_preview_area(selected)
> +    def _can_move_horizontally(self):
> +        '''Return a boolean indicating if horizontal movement is allowed.'''
> +        item = self.menu.get_selected()
> +        if self.feed_library.is_empty() or item.get_name() != 'rss':
> +            return False
>          else:
> -            self.rss_preview_menu.move(preview_direction)
> +            return True
> +
> +    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:

    """Single line that describes the function.

    Optional details on how the method works.
    """


> === 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.

> +import math
> +
> +class MotionBuffer:
> +    """This Class achieves some calculations on the pointer motion"""
> +
> +    def __init__ (self):
> +        # public
> +        self.dt_from_start = 0
> +        self.dx_from_start = 0
> +        self.dy_from_start = 0
> +        self.distance_from_start = 0
> +        self.speed_x_from_start = 0
> +        self.speed_y_from_start = 0
> +        self.speed_from_start = 0
> +
> +        self.dt_from_last_motion_event = 0
> +        self.dx_from_last_motion_event = 0
> +        self.dy_from_last_motion_event = 0
> +        self.distance_from_last_motion_event = 0
> +        self.speed_x_from_last_motion_event = 0
> +        self.speed_y_from_last_motion_event = 0
> +        self.speed_from_last_motion_event = 0
> +
> +        self.dt_ema = 0
> +        self.dx_ema = 0
> +        self.dy_ema = 0
> +        self.distance_ema = 0
> +        self.speed_x_ema = 0
> +        self.speed_y_ema = 0
> +        self.speed_ema = 0
> +
> +        # private
> +        self._start_x = 0
> +        self._start_y = 0
> +        self._start_time = 0
> +
> +        self._last_motion_event_x = 0
> +        self._last_motion_event_y = 0
> +        self._last_motion_event_time = 0
> +
> +        # factors for exponential moving average calculation
> +        # the moving average is used as a filter
> +        self._rank = 5
> +        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.  However, I would like to see documentation for what all these vars
are used for.  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.

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


> +    def start(self, event):
> +        '''Buffer initialization'''
> +        self._start_x = event.x
> +        self._start_y = event.y
> +        self._start_time = event.time
> +
> +        self._last_motion_event_x = event.x
> +        self._last_motion_event_y = event.y
> +        self._last_motion_event_time = event.time
> +
> +        self.dt_ema = 0
> +        self.dx_ema = 0
> +        self.dy_ema = 0
> +        self.distance_ema = 0
> +        self.speed_x_ema = 0
> +        self.speed_y_ema = 0
> +        self.speed_ema = 0
> +
> +    def take_new_motion_event(self, event):
> +        '''To be called by on every *motion-event* of an actor'''
> +        self._last_motion_event_x = event.x
> +        self._last_motion_event_y = event.y
> +        self._last_motion_event_time = event.time
> +
> +    def compute_from_start(self, event):
> +        '''Compute deltas an speeds from the beginning of the motion'''
> +        self.dt_from_start = event.time - self._start_time
> +        self.dx_from_start = event.x - self._start_x
> +        self.dy_from_start = event.y - self._start_y
> +        self.distance_from_start = math.sqrt(self.dx_from_start * \
> +            self.dx_from_start + self.dy_from_start * self.dy_from_start)
> +
> +        if self.dt_from_start != 0 :
> +            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 ().  That's how the rest of Entertainer
does it.


> === modified file 'entertainerlib/frontend/gui/widgets/scroll_menu.py'
> --- entertainerlib/frontend/gui/widgets/scroll_menu.py	2009-03-02 02:27:27 +0000
> +++ entertainerlib/frontend/gui/widgets/scroll_menu.py	2009-04-25 23:30:15 +0000
> @@ -2,211 +2,236 @@
>  
>  __licence__ = "GPLv2"
>  __copyright__ = "2007, Lauri Taimila"
> -__author__ = "Lauri Taimila <email address hidden>"
> +__author__ = ("Lauri Taimila <email address hidden>",
> +              "Samuel Buffet <email address hidden>",
> +              "Joshua Scotton <email address hidden>")
> +
> +import math
>  
>  import clutter
> +import gobject
> +
> +from entertainerlib.frontend.gui.widgets.label import Label
> +from entertainerlib.frontend.gui.widgets.motion_buffer import MotionBuffer
> +from entertainerlib.frontend.gui.widgets.special_behaviours import \
> +    LoopedPathBehaviour
> +
> +class ScrollMenuItem(clutter.Group):
> +    """A Group containing a Label to which a LoopedPathBehaviour is applied."""
> +    __gtype_name__ = 'ScrollMenuItem'
>  
> -class ScrollMenu(clutter.Group):
> -    """
> -    Menu widget that contains text items. Menu can be scrolled up and down.
> -
> -    - Options: zoom, opacity, display N items, display arrows top+down
> -    - add Clutter labels
> -    - go around (how to implement this) (On state_change move items from
> -        start->back)
> -    """
> -
> -    DOWN = 0
> -    UP = 1
> -
> -    def __init__(self, item_gap, item_height, animated=False):
> -        """
> -        Initialize Menu
> -        @param item_gap: Gap between menuitems in pixels
> -        """
> +    def __init__(self, alpha, text, item_height, font_size, color_name):
>          clutter.Group.__init__(self)
> -        self.__active = True
> -        self.__clip_size = -1
> -        self.__items = []
> -        self.__gap = item_gap
> -        self.__item_height = item_height
> -        self.__animated = animated
> -
> -        self.scroll_timeline = clutter.Timeline(7, 26)
> -        self.scroll_timeline.connect('new-frame', self.scroll_items_cb)
> -
> -    def add(self, actor):
> -        """
> -        Add new menuitem to the menu.
> -        @param item: Label
> -        """
> -        clutter.Group.add(self, actor)
> -        actor.connect('notify::y', self.actor_notify_y)
> -
> -        y = ((len(self.__items) * self.__item_height) +
> -            (self.__gap * len(self.__items)))
> -
> -        if y == 0 :
> -            # need to do that to update opacity of the first item added
> -            self.actor_notify_y(actor, None)
> -        actor.set_position(0, y)
> -
> -        self.__items.append(actor)
> -
> -    def remove(self, actor):
> -        """
> -        Remove actor from the menu.
> -        """
> -        clutter.Group.add(self, actor)
> -        for item in self.__items:
> -            if item is actor:
> -                del item
> -
> -    def remove_all(self):
> -        """
> -        Remove all actors from the menu.
> -        """
> -        del self.__items
> -        self.__items = []
>  
> -    def get_index(self, text):
> -        """
> -        Returns index of label with the text as passed, returns -1 if not found
> -        @author Joshua Scotton
> -        """
> -        for item in self.__items:
> -            if item.get_name() == text:
> -                return self.__items.index(item)
> -        return -1
> +        self.label = Label(font_size, color_name, 0, 0)
> +        self.label.set_text(text)
>  
> -    def select(self, index):
> -        """
> -        Selects the item at the passed index, does not support animations
> -        @author Joshua Scotton
> -        """
> -        #split list so that item at the index will now be at position 2
> -        #which is the selected index in a scroll menu
> -        items = self.__items[index-2:] + self.__items[0:index-2]
> -
> -        #Basically remove all then add in the correct order
> -        self.remove_all()
> -        for item in items:
> -            #Remove all doesn't remove the parent object. So in order to avoid
> -            #adding two parent clutter items to the item we need to remove the
> -            #old one first
> -            clutter.Group.remove(self, item)
> -            self.add(item)
> -
> -    def get_number_of_items(self):
> -        """Get number of menuitems in this Menu"""
> -        return len(self.__items)
> -
> -    def set_number_of_visible_items(self, number):
> -        """
> -        Set how many menuitems are displayed on the screen at the same time.
> -        @param number: Number of items
> -        """
> -        self.__clip_size = number
> -        self.set_clip(0,
> -                      0,
> -                      self.get_width(),
> -                      (self.__item_height + self.__gap) * number)
> -
> -    def get_number_of_visible_items(self):
> -        """
> -        Get number of menuitems that are displayed on the screen at the same
> -        time.
> -
> -        @return: Ingeter
> -        """
> -        return self.__clip_size
> +        self.behaviour = LoopedPathBehaviour(alpha)
> +        self.behaviour.apply(self)
>  
> -    def get_selected(self):
> -        """
> -        Get currently selected menuitem
> -        @return Clutter actor object
> -        """
> -        return self.__items[2]
> -
> -    def scroll_down(self):
> -        """Scroll menu down by one menuitem."""
> -        if not self.scroll_timeline.is_playing():
> -            if self.__animated:
> -                self.__scroll_direction = self.DOWN
> -                self.scroll_timeline.start()
> -            else:
> -                for item in self.__items:
> -                    item.move_by(0, self.__item_height + self.__gap)
> +        self.add(self.label)
>  
> -            # Set last item to first (menu scrolls)
> -            last = self.__items[-1]
> -            last.set_position(
> -                0, self.__items[0].get_y() - self.__gap - self.__item_height)
> -            self.__items = [last] + self.__items
> -            del self.__items[-1]
> -
> -    def scroll_up(self):
> -        """Scroll menu up by one menuitem."""
> -        if not self.scroll_timeline.is_playing():
> -            if self.__animated:
> -                self.__scroll_direction = self.UP
> -                self.scroll_timeline.start()
> -            else:
> -                for item in self.__items:
> -                    item.move_by(0, -(self.__item_height + self.__gap))
> +class ScrollMenu(clutter.Group, object):
> +    """Menu widget that contains text items."""
> +    __gtype_name__ = 'ScrollMenu'
> +    __gsignals__ = {
> +        'activated' : ( gobject.SIGNAL_RUN_LAST, gobject.TYPE_NONE, () ),
> +        'selected' : ( gobject.SIGNAL_RUN_LAST, gobject.TYPE_NONE, () ),
> +        'moved' : ( gobject.SIGNAL_RUN_LAST, gobject.TYPE_NONE, () ),
> +        }
> +
> +    MODE_SELECTION = 0
> +    MODE_MOTION = 1
> +    MODE_STOP = 2
> +
> +    def __init__(self, item_gap, item_height, font_size, color_name):
> +        clutter.Group.__init__(self)
> +        self._motion_buffer = MotionBuffer()
> +        self._items = []
> +        self._item_gap = item_gap
> +        self._item_height = item_height
> +        self._item_font_size = font_size
> +        self._item_color_name = color_name
> +        self._selected_index = 1
> +        self._visible_items = 5
> +        self._event_mode = -1
> +        self._animation_progression = 0
> +        self._animation_start_index = 1
> +        self._animation_end_index = 1
> +        self._active = False
> +        self._motion_handler = 0
> +

Once again, we're retaining state for an object.  Maybe we can find a way to
retain state in 

> +        self._timeline = clutter.Timeline(fps=200, duration=300)
> +        self._alpha = clutter.Alpha(self._timeline, clutter.sine_inc_func)
> +
> +        # preparation to pointer events handling
> +        self.set_reactive(True)
> +        self.connect('scroll-event', self._on_scroll_event)
> +        self.connect('button-press-event', self._on_button_press_event)
> +        self.connect('button-release-event', self._on_button_release_event)
> +
> +    def refresh(self):
> +        """Refresh the menu: clip area dimensions and items positions"""
> +        self._set_selected_index(self._selected_index, 1)
> +        self._set_visible_items(self._visible_items)
> +
> +    def add_item(self, text, name):
> +        """Creation of a new MenuItem and addition to the ScrollMenu"""
> +        item = ScrollMenuItem(self._alpha, text, self._item_height,
> +            self._item_font_size, self._item_color_name)
> +
> +        item.set_name(name)
> +        item.connect('notify::y', self._update_item_opacity)
> +        self.add(item)
> +
> +        self._items.append(item)
> +        self._update_item_opacity(item)
> +
> +    def remove_item(self, name):
> +        """Remove an item from the menu"""
> +        index = self.get_index(name)
> +
> +        if index != -1:
> +            # if item was found, we remove it from the item list, from the
> +            # group and finally we delete it.
> +            item = self._items[index]
> +            self._items.remove(item)
> +            self.remove(item)
> +            del item
> +
> +    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.


> === added file 'entertainerlib/tests/test_scrollmenu.py'
> --- entertainerlib/tests/test_scrollmenu.py	1970-01-01 00:00:00 +0000
> +++ entertainerlib/tests/test_scrollmenu.py	2009-04-25 23:30:15 +0000
> @@ -0,0 +1,33 @@
> +"""Tests ScrollMenu"""
> +
> +__license__ = "GPLv2"
> +__copyright__ = "2009, Samuel Buffet"
> +__author__ = "Samuel Buffet <email address hidden>"
> +
> +from entertainerlib.frontend.gui.widgets.scroll_menu import ScrollMenu
> +from entertainerlib.tests import EntertainerTest
> +
> +class ScrollMenuTest(EntertainerTest):
> +    """Test for entertainerlib.frontend.gui.widgets.scroll_menu"""
> +
> +    def setUp(self):
> +        """Set up the test"""
> +        EntertainerTest.setUp(self)
> +
> +        self.menu = ScrollMenu(10, 60, 0.045, "menuitem_active")
> +        self.menu.set_name("mainmenu")
> +
> +        self.menu.add_item(_("Play CD"), "disc")
> +        self.menu.add_item(_("Videos"), "videos")
> +        self.menu.add_item(_("Music"), "music")
> +        self.menu.add_item(_("Photographs"), "photo")
> +        self.menu.add_item(_("Headlines"), "rss")
> +
> +    def tearDown(self):
> +        """Clean up after the test"""
> +        EntertainerTest.tearDown(self)
> +
> +    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.