Merge lp:~samuel-buffet/entertainer/new-scrollmenu into lp:entertainer

[Samuel Buffet]

The ScrollMenu is now reactive to pointer events.

[Samuel Buffet (samuel-buffet)]

Hi,

Finally, I think this branch is robust enough to be proposed.

The goal of this new scroll menu was to improve animations and to make it pointer reactive.

So, the scroll menu core mechanism has been created from scratch but I kept some old interface methods.

A new LoopedPath behaviour was created. It's more or less a 2 knots clutter path behaviour driven by an index. Index=0 place the actor on the starting knot and Index=1 place it on the end knot. Index can be whatever value we want but the actor will always be positioned on the 2 knots "infinite" path (ie if we apply the behaviour from idx=0 to idx=2 then the actor will move 2 times along the segment).

I've also added a MotionBuffer Class to make some calculations (distances, speeds) when we move the mouse. I've placed it its own file because this is something we can use later on other widgets (probably other menus). Right now it does more than needed by the ScrollMenu widget (only a vertical speed was needed).

I've also added a hide/show cursor mechanism (the one I discussed with Matt previously).

New interaction possible with the mouse are :

click on one menu item => moves it to central position
click on the menu item in central position => *enter* that menu
click + keeping the mouse button pressed and moving => we can slowly move the menu or give it an impulse to "turn" with a first try of kinetic effect based on the mouse speed motion.

The main screen has been modified to react to the new signals.

Cheers,
Samuel

[Samuel Buffet (samuel-buffet)]

As I've done some small changes since the proposal I post here the new diff.

=== modified file 'entertainerlib/frontend/gui/screens/main.py'
--- entertainerlib/frontend/gui/screens/main.py 2009-03-02 03:11:24 +0000
+++ entertainerlib/frontend/gui/screens/main.py 2009-04-09 19:29:11 +0000
@@ -44,65 +44,88 @@
         self.preview = clutter.Group() # Group that contains preview actors
         self.preview.set_position(self.get_abs_x(0.07), self.get_abs_y(0.1))
         self.preview.show()
+ self.preview.set_opacity(0x00)
         self.add(self.preview)

- self.active_menu = "main"
- self.rss_preview_menu = None # Pointer to the current preview menu
-
- self.menu = None
- self.playing_item = None
- self.create_main_menu()
-
- clock = ClockLabel(0.13, "screentitle", 0, 0.87)
- self.add(clock)
-
- def create_main_menu(self):
+ self.rss_preview_menu = self._create_rss_preview_menu()
+ self.preview.add(self.rss_preview_menu)
+
+ self._create_main_menu()
+ self.menu.active = True
+
+ self.add(ClockLabel(0.13, "screentitle", 0, 0.87))
+
+ def get_type(self):
+ """Return screen type."""
+ return Screen.NORMAL
+
+ def get_name(self):
+ """Return screen name (human readble)"""
+ return "Main"
+
+ def _create_main_menu(self):
         """Create main menu of the home screen"""
- self.menu = ScrollMenu(10, 60, self.config.show_effects())
+ self.menu = ScrollMenu(10, 60, 0.045, "menuitem_active")
         self.menu.set_name("mainmenu")
-
- # Common values for all ScrollMenu items
- size = 0.045
- color = "menuitem_active"
-
- item0 = Label(size, color, 0, 0, _("Play CD"), "disc")
- self.menu.add(item0)
-
- item2 = Label(size, color, 0, 0, _("Videos"), "videos")
- self.menu.add(item2)
-
- item3 = Label(size, color, 0, 0, _("Music"), "music")
- self.menu.add(item3)
-
- item4 = Label(size, color, 0, 0, _("Photographs"), "photo")
- self.menu.add(item4)
-
- item5 = Label(size, color, 0, 0, _("Headlines"), "rss")
- self.menu.add(item5)
+ self.menu.connect('selected', self._handle_select)
+ self.menu.connect('moved', self._on_menu_moved)
+ self.menu.connect('activated', self.main_menu_activation)
+
+ 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")

         if self.config.display_weather_in_frontend():
- item6 = Label(size, color, 0, 0, _("Weather"), "weather")
- self.menu.add(item6)
+ self.menu.add_item(_("Weather"), "weather")

         if self.config.display_cd_eject_in_frontend():
- item7 = Label(size, color, 0, 0, _("Eject CD"), "eject_cd")
- self.menu.add(item7)
+ self.menu.add_item(_("Eject CD"), "eject_cd")

         if self.media_player.has_media():
- playing = Label(size, color, 0, 0, _("Playing now..."), "playing")
- ...

[Matt Layman (mblayman)]

Samuel,

I'm finally getting a chance to look at some of this code. I'm going to do this review in a couple of stages because of some functional regressions that I found while doing user testing. First, I'll let you know what those problems are so that you'll get a chance to fix them, then I'll come back and review the new widget code in more detail.

But let me start by saying this is really cool stuff. Sometimes it is easy to be critical and just point out faults in code, but I just want to let you know in advance that I think this work is pretty awesome. I'm sure you put quite a bit of effort into this code, and it shows.

Okay, now for the comments:

1) The scroll menu no longer remembers its last item when you return to a screen. Every time I hit back, it started on Music again.

2) The major problem that I have so far is that the keyboard interface is mostly broken if you return to the main screen after being on a different screen. For some reason, the menu doesn't think that it is active so it won't react until the user presses the right arrow key, then it becomes active.

Now for the minor comments (keep in mind that in this stage of the review, I've only looked at the Main screen and UserInterface files):

3) I'm not a fan of variable abbreviations most of the time. I noticed that you have the menu property "visible_nb" and I understand what it is supposed to mean, but since ScrollMenu uses the term "item," I think that "visible_items" would be a much clearer property name.

4) You created some new methods in Main and UserInterface (which is fine, of course), but I don't think that they match the conventions that we use for "private" methods. Since the methods you made aren't meant to be used outside the class, I would recommend that you prepend an underscore character to make that clear (e.g., preview_activation becomes _preview_activation). This isn't directly stated in PEP8, but there is some discussion about having private portions start with _.

As soon as these regressions are addressed, I'll come back and look specifically at the underlying widget code.

Thanks.

review: Needs Fixing

[Samuel Buffet (samuel-buffet)]

Hi Matt,

Thanks a lot for your review. I've fixed the things you've already noticed.

> 1) The scroll menu no longer remembers its last item when you return to a
> screen. Every time I hit back, it started on Music again.

This is fixed. I've also replace "..has_media()" by "..is_playing" because I thought it was closer to what we want in the preview. This will allow us in a close future to connect "play" "stop" events of the media_player to the refresh method so we'll have a dynamic update of it. (those events are created in *reactive_progress_bar*)

> 2) The major problem that I have so far is that the keyboard interface is
> mostly broken if you return to the main screen after being on a different
> screen. For some reason, the menu doesn't think that it is active so it won't
> react until the user presses the right arrow key, then it becomes active.

Yep didn't notice that. It's fixed now.

> 3) I'm not a fan of variable abbreviations most of the time. I noticed that
> you have the menu property "visible_nb" and I understand what it is supposed
> to mean, but since ScrollMenu uses the term "item," I think that
> "visible_items" would be a much clearer property name.
>
> 4) You created some new methods in Main and UserInterface (which is fine, of
> course), but I don't think that they match the conventions that we use for
> "private" methods. Since the methods you made aren't meant to be used outside
> the class, I would recommend that you prepend an underscore character to make
> that clear (e.g., preview_activation becomes _preview_activation). This isn't
> directly stated in PEP8, but there is some discussion about having private
> portions start with _.

Yes, that's right. I've renamed those private methods.

Thanks again Matt
Samuel-

new diff :

=== modified file 'entertainerlib/frontend/gui/screens/main.py'
--- entertainerlib/frontend/gui/screens/main.py 2009-03-02 03:11:24 +0000
+++ entertainerlib/frontend/gui/screens/main.py 2009-04-14 19:43:51 +0000
@@ -44,65 +44,89 @@
         self.preview = clutter.Group() # Group that contains preview actors
         self.preview.set_position(self.get_abs_x(0.07), self.get_abs_y(0.1))
         self.preview.show()
+ self.preview.set_opacity(0x00)
         self.add(self.preview)

- self.active_menu = "main"
- self.rss_preview_menu = None # Pointer to the current preview menu
-
- self.menu = None
- self.playing_item = None
- self.create_main_menu()
-
- clock = ClockLabel(0.13, "screentitle", 0, 0.87)
- self.add(clock)
-
- def create_main_menu(self):
+ self.rss_preview_menu = self._create_rss_preview_menu()
+ self.preview.add(self.rss_preview_menu)
+
+ self._create_main_menu()
+ self.menu.active = True
+
+ self.add(ClockLabel(0.13, "screentitle", 0, 0.87))
+
+ def get_type(self):
+ """Return screen type."""
+ return Screen.NORMAL
+
+ def get_name(self):
+ """Return screen name (human readble)"""
+ return "Main"
+
+ def _create_main_menu(self):
         """Create main menu of the home screen"""
- self.menu = ScrollMenu(10, 60, self.config.show_effects())
+ ...

[Samuel Buffet (samuel-buffet)]

Some details about the LoopedPathBehaviour Class and the way it's used in the ScrollMenu.

First, you have to know that this class is inspired by ClutterBehaviourEllipse.

With this standard Clutter behaviour, we can move actors along an Ellipse. Actors's position on this Ellipse are defined by angles, and a motion can be described by a angle_start and a angle_end. The angle can be seen as a parameter describing an actor's position along the ellipse.

The LoopedPathBehaviour can be seen as an extension of the regular PathBehaviour but where we specify a path (here only a line in fact => only 2 knots allowed atm) AND a couple start_index/end_index. This can't be done with the PathBehaviour, with this standard behaviour we must start at the starting knot and stop at the ending knot.

LoopedPathBehaviour allows us to specify a start_index and a end_index corresponding to a starting point and a ending point on the segment defined by the 2 knots.

This ability to specify a start_index and a end_index is used in the ScrollMenu that way :

* we create a LoopedPathBehaviour for every menu item and apply it to this item
* in the _update_behaviours we set the start knot and the end knot. Those knots (so the path) are the same for every menu item. And then for each item we specify a start_index and a end_index which is a function of the position of the item in the item list AND the position of the *selected item* (meaning the item which is in the middle of the visible are, the clipped area of the widget).

Trust this helps,
Samuel-

Note: I know this one is not totally obvious. It's used in a much simpler way on the ScrollArea widget because there it's applied to only one widget, the moving clutter group including the clutter Label.

On the _update_behaviours method, we

[Matt Layman (mblayman)]

Samuel,

These are the comments that I had written down before you last push. I still haven't looked at MotionBuffer and I need to give all the files a second look through, but I think it would be a good start to address some of these things. I hope you find them helpful. Most of the comments are just an attempt to keep a unified style in the codebase. The comments might seem a little rough because they are basically just my notes. I hope that they're readable enough.

main.py:
 * _create_main_menu should return the menu. And the main screen should add that menu to itself. This will ensure that the init is the place that self.menu is created. It would also allow testing of _create_main_menu in the future (if we ever figure out what we would need to test). I did this for _create_rss_preview_menu and I think that it's a nice separation of responsibility. After all, shouldn't it only be the init that adds instance variables to itself?
 * docstrings need improvement for _on_menu_moved and _on_menu_selected. Don't use "We" and try to reduce to one line if you can.
 * hide_cursor_timeout_callback and handle_motion_event seem private. They should probably start with an underscore.

scroll_menu.py:
 * import order is wrong because math is a core library.
 * docstring doesn't need "This is" and might fit on one line
 * What is __gtype_name__ for?
 * Could clean up docstrings for get_active, set_active, get_selected (no need for @return and @param).
 * idx, xx, and yy aren't very intuitive variable names in _display_items_at_target
 * In _set_selected_index, I don't understand "#compute selected index to be between 0 and nb." What is nb? number? What number if so.
 * I'd suggest you remove the @author tag in get_index and just add Joshua Scotton as an author to the top of the file. I don't think we need to identify authors at the method level (that's what bzr annotate is for), but he still has the right to credit himself with something if that is what he wrote.

special_behaviours.py:
 * docstring is wrong at top of file.
 * import order is wrong. math is a core lib and clutter is not so math should be first and clutter should be below it separated by a newline.
 * No need for the space between class name and parenthetical superclass.
 * "custome" is actually spelled custom
 * do_alpha_notify has a space before its parameter list
 * Don't need "We" in the docstring for do_alpha_notify
 * what are idx and idxx?
 * Does path_length need to return an integer? (because it says length in pixels)

test_scrollmenu.py:
 * has an empty line with spaces.
 * testCreate should use a PEP8 method name. test_create (We use camel case for setUp and tearDown because that's what unittest does. Any existing test that we have that uses camel case is just because it's from a time before started being fully PEP8 compliant.)
 * test ScrollMenu instead of object

[Samuel Buffet (samuel-buffet)]

Hi Matt,

First, I'd like to thank you for all the efforts you put in this review. This really helps and I deeply appreciate.

In my last push, I've fixed all what you've already notice and more in the same spirit (mainly docstrings).

> a unified style in the codebase. The comments might seem a little rough
> because they are basically just my notes. I hope that they're readable enough.

No pb here, there are perfectly readable and not rough at all (I mean things have to be said)

> test_scrollmenu.py:
> * has an empty line with spaces.
> * testCreate should use a PEP8 method name. test_create (We use camel case
> for setUp and tearDown because that's what unittest does. Any existing test
> that we have that uses camel case is just because it's from a time before

Hmm, Matt I don't know what *camel* is about

Now, the good point is that after this review, everything will look more or less relax and easy ;)

Samuel-

[Matt Layman (mblayman)]

Samuel,

It seems like you have been very busy making modifications to this branch. Now that thing have stabilized a bit, let me throw another few comments at you. After this round of comments, I really only have to review the MotionBuffer class (I don't the time to do that tonight). Again, thanks for the good work.

main.py:
 * _move_menu is now a dead unused method that can be removed.

scroll_menu.py:
 * _on_release_event has more idx variables
 * line 301 could use some spacing around the less than character for clarity
 * _on_motion_event: why is it showing the cursor? That is done by the UserInterface.
 * _on_scroll_event: why is it showing the cursor? That is done by the UserInterface.

[Samuel Buffet (samuel-buffet)]

Hi Matt,

> Samuel,
>
> It seems like you have been very busy making modifications to this branch. Now
> that thing have stabilized a bit, let me throw another few comments at you.
> After this round of comments, I really only have to review the MotionBuffer
> class (I don't the time to do that tonight). Again, thanks for the good work.
>
> main.py:
> * _move_menu is now a dead unused method that can be removed.

Indeed, I've removed it.

> scroll_menu.py:
> * _on_release_event has more idx variables
> * line 301 could use some spacing around the less than character for clarity

Fixed.

> * _on_motion_event: why is it showing the cursor? That is done by the
> UserInterface.
> * _on_scroll_event: why is it showing the cursor? That is done by the
> UserInterface.

Yeah that's done by the UserInterface but if for instance you place your cursor on the menu and scroll it (no motion only scroll) then after 4 seconds the cursor will be hidden without the cursor.show in _on_scroll_event because there's no motion event grabbed by the stage. Just try to comment that line out, it looks strange from the user experience point of view (I think).

Same kind of thing with _on_motion_event if you want to click on the menu and, keeping the pressure on the button you play with it up and down. It will hide the cursor after 4 seconds but here the answer to "why?" is more subtle. Because here we have motion-events but they are grabbed only by the menu because of the clutter.grab_pointer(self) placed on _on_button_press_event. This redirection of all events to the menu is only released in _on_button_release_event by clutter.ungrab_pointer().
So until the button is released the stage is blind. It see no event at all so the timeout is not reset and hides the cursor after 4 seconds.

Now, after having explain that, maybe I should explain why I've done this grab/ungrab stuff. Simply it's because without clutter.grab_pointer(self) event are seen by the menu only if the cursor is over the menu. And when we move it to give it an impulse to scroll we can easily finish the cursor's motion (= release the mouse's button) anywhere with no guaranty to be over the menu. So in this case, the _on_button_release_event event will never be seen by the menu and it will be in a unstable weird state.

I've tried another way to detect the button's release using the ClutterModifierType field of a motion event which is supposed to provide the state of button (if I understand correctly). But unfortunately, it looks like it doesn't work (maybe after more tests I'll file a bug upstream).

Cheers,
Samuel-

[Matt Layman (mblayman)]

Okay, I finally had a good chance to sit down and look at MotionBuffer so without further adieu:

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.

Comments:
 * I think that the class should be a new style class (inherit from object)
 * 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.
 * compute methods docstrings are sentences so they each need a period. And it should be "deltas and speeds".
 * 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?
 * 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.

[Paul Hummer (rockstar)]

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()
...

[Samuel Buffet (samuel-buffet)]

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-

[Matt Layman (mblayman)]

Paul,

I'd like to add some comments of my own to what you've said. I think I can clear up some points about clutter since you haven't been doing much work in the frontend. I also have some general remarks based on what you've stated.

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

I hadn't really thought about it, but I think you're correct, the connect method calls and active setting could move into _create_main_menu.

self.add() is a clutter.Group method. What's it's doing is adding the self.menu clutter.Group to the Main screen (which itself is a clutter.Group). I had suggested to Samuel that self.add does not belong in create_main_menu because it should be clear that it is the init's responsibility to add new groups to itself. That's my opinion because I've often asked myself "where the hell is the widget being added?" and if it was all the init, we wouldn't have to hunt in other methods for when things show up on the screen. Basically, it was an experience judgment call from working with some of our clutter code.

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

main_menu is really just an initialized version of a ScrollMenu widget. The group doesn't represent everything that could show up on the Main screen. I think declaring the ClockLabel in Main's init makes sense.

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

[Samuel Buffet (samuel-buffet)]

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

[Matt Layman (mblayman)]

Samuel, the only thing keeping me from approving this branch is a lack of some tests. As Paul suggested, you could add some tests to test the state of some of the instance variables. It may require creating a mock event that you can feed to the ScrollMenu to probe things (rather than connecting to any handler methods).

I would suggest something like this:
1) You could add assertEqual statements to the create test so that you verify that your instance variables that get parameter values actually equal those values
2) There aren't any tests for MotionBuffer yet and I think it is important to add some for that class (because I think MotionBuffer is going to affect the behavior of most of our widgets so it needs some coverage).
 * You could do a create test and check that instance variable are the values you expect them to be.
 * You could pass in a mock event, compute the changes, and then make sure that those values are what you would expect. One way to do this would be to set up the test in a way that you know it will fail, then when you get the correct value from the failure, replace your faulty value with the value from the failure. You might ask "what is the value for this kind of test?" My answer is that it helps you capture the current behavior. If you think that MotionBuffer is acting the way you expect it to, then you want to preserve that. If you add the kind of test that I just described, you'll be able to find out when someone changes the behavior of MotionBuffer. In the future, you might decide that it is okay or you can decide that the change in behavior is wrong.

E.g.,

self.motion_buffer.compute_from_start(mock_event)
self.assertEqual(self.motion_buffer.distance_from_start, 'boogers')

I'm hoping this will fail :). Once it fails, replace 'boogers' with the value that you received. You may have to use assertAlmostEqual for some of the math instance variables.

Anyway, those are just my thoughts about what you could do to get some test coverage on this widgets. Trust me, I feel your pain when it comes to testing widgets. It's a big pain in the rear.

[Samuel Buffet (samuel-buffet)]

Matt, Paul,

Find bellow the diff of my last 3 commits :

I've made the diff between a local copy and the one pushed on lp:
So everything starting with + has been removed and everything starting with - has been added.

I've done a bit of clean up in the MotionBuffer Class and added more tests.

Samuel-

=== modified file 'entertainerlib/frontend/gui/widgets/motion_buffer.py'
--- entertainerlib/frontend/gui/widgets/motion_buffer.py 2009-04-28 09:49:40 +0000
+++ entertainerlib/frontend/gui/widgets/motion_buffer.py 2009-04-28 12:19:55 +0000
@@ -22,6 +22,9 @@
         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
@@ -84,6 +87,18 @@
         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 and speeds from the last motion-event.'''
         self.dt_from_last_motion_event = event.time - \

=== modified file 'entertainerlib/frontend/gui/widgets/scroll_menu.py'
--- entertainerlib/frontend/gui/widgets/scroll_menu.py 2009-04-28 12:18:16 +0000
+++ entertainerlib/frontend/gui/widgets/scroll_menu.py 2009-04-28 12:19:55 +0000
@@ -117,10 +117,6 @@

     active = property(_get_active, _set_active)

- def stop_animation(self):
- '''Stops the timeline driving menu animation.'''
- self._timeline.stop()
-
     def _update_behaviours(self, target):
         """Preparation of behaviours applied to menu items before animation"""
         items_len = len(self._items)

=== modified file 'entertainerlib/tests/mock.py'
--- entertainerlib/tests/mock.py 2009-04-28 11:49:43 +0000
+++ entertainerlib/tests/mock.py 2009-04-28 12:19:55 +0000
@@ -304,12 +304,3 @@
         '''See `VideoLibrary.get_number_of_video_clips`.'''
         return 0

-
-class MockPointerEvent(object):
- '''Mock a pointer event like those generated by Clutter'''
-
- def __init__(self):
- self.x = 0
- self.y = 0
- self.time = 0
-

=== removed file 'entertainerlib/tests/test_motionbuffer.py'
--- entertainerlib/tests/test_motionbuffer.py 2009-04-28 12:18:16 +0000
+++ entertainerlib/tests/test_motionbuffer.py 1970-01-01 00:00:00 +0000
@@ -1,83 +0,0 @@
-"""Tests MotionBuffer"""
-
-__license__ = "GPLv2"
-__copyright__ = "2009, Samuel Buffet"
-__author__ = "Samuel Buffet <email address hidden>"
-
-from entertainerlib.frontend.gui.widgets.motion_buf...

[Matt Layman (mblayman)]

Samuel, I'll be able to look over your latest changes and tests when I get home from work today. Barring any obvious problems, I'll likely approve the proposal today. If you could set the commit message (it's an option in the proposal details), then the approval and commit message should be indicators to tarmac to commit the work to the trunk.

I think this is how we will be committing stuff to the trunk from here on out, but I need to document that on the mailing list and work with Paul to make sure that Tarmac is ready to handle that.

[Matt Layman (mblayman)]

What a long review.... Samuel, I checked out your tests and they seem reasonable to me. Also, make test and lint passes (to the level I would expect, there are errors but they are unrelated to this code). There could probably be more tests, but I guess there could always be more tests. :)

I am satisfied enough to have this code committed to the trunk. This is a great start toward reactive widgets and I'm hoping that future ones will be easier to review now that we've covered the groundwork. I think that this branch is one step closer to making a lot of people happy who will eventually want touch screen support for Entertainer.

Nice work.

review: Approve

[Samuel Buffet (samuel-buffet)]

> What a long review....

And it will get a bit longer because it's important for me to have Paul's vote on it ;)
Et has commented it and wrote that I'll give his vote later so it means that he has an opinion, and I'd like to have it. Paul ?

> Nice work.

Thanks Matt,
Samuel-

[Matt Layman (mblayman)]

Sorry, I didn't realize Paul actually wanted to cast a vote on it for this branch.

[lp:~samuel-buffet/entertainer/new-scrollmenu]

364. By Samuel Buffet on 2009-05-05

merged with trunk 367

365. By Samuel Buffet on 2009-05-05

pylint: disable-msg=W0702 added to fix lint after last reintroduction of a try/except without particular exception catched.

[Paul Hummer (rockstar)]

You probably don't need to block on my comments. I can make code comments, but Matt really knows the UI code a lot better than I do. Even if this branch isn't ready for production, we have a little over a month to test it out and make sure that we can actually get things ironed out before release. Since this is a core part of the next release, I think even early code is better than no code.

review: Approve

=== modified file 'entertainerlib/frontend/gui/screens/main.py'
--- entertainerlib/frontend/gui/screens/main.py	2009-03-02 03:11:24 +0000
+++ entertainerlib/frontend/gui/screens/main.py	2009-04-08 11:23:26 +0000
@@ -44,65 +44,89 @@
         self.preview = clutter.Group() # Group that contains preview actors
         self.preview.set_position(self.get_abs_x(0.07), self.get_abs_y(0.1))
         self.preview.show()
+        self.preview.set_opacity(0x00)
         self.add(self.preview)
 
-        self.active_menu = "main"
-        self.rss_preview_menu = None # Pointer to the current preview menu
-
-        self.menu = None
-        self.playing_item = None
-        self.create_main_menu()
-
-        clock = ClockLabel(0.13, "screentitle", 0, 0.87)
-        self.add(clock)
-
-    def create_main_menu(self):
+
+        self.rss_preview_menu = self._create_rss_preview_menu()
+        self.preview.add(self.rss_preview_menu)
+
+        self._create_main_menu()
+        self.menu.active = True
+
+        self.add(ClockLabel(0.13, "screentitle", 0, 0.87))
+
+    def get_type(self):
+        """Return screen type."""
+        return Screen.NORMAL
+
+    def get_name(self):
+        """Return screen name (human readble)"""
+        return "Main"
+
+    def _create_main_menu(self):
         """Create main menu of the home screen"""
-        self.menu = ScrollMenu(10, 60, self.config.show_effects())
+        self.menu = ScrollMenu(10, 60, 0.045, "menuitem_active")
         self.menu.set_name("mainmenu")
-
-        # Common values for all ScrollMenu items
-        size = 0.045
-        color = "menuitem_active"
-
-        item0 = Label(size, color, 0, 0, _("Play CD"), "disc")
-        self.menu.add(item0)
-
-        item2 = Label(size, color, 0, 0, _("Videos"), "videos")
-        self.menu.add(item2)
-
-        item3 = Label(size, color, 0, 0, _("Music"), "music")
-        self.menu.add(item3)
-
-        item4 = Label(size, color, 0, 0, _("Photographs"), "photo")
-        self.menu.add(item4)
-
-        item5 = Label(size, color, 0, 0, _("Headlines"), "rss")
-        self.menu.add(item5)
+        self.menu.connect('selected', self._handle_select)
+        self.menu.connect('moved', self._on_menu_moved)
+        self.menu.connect('activated', self.main_menu_activation)
+
+        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")
 
         if self.config.display_weather_in_frontend():
-            item6 = Label(size, color, 0, 0, _("Weather"), "weather")
-            self.menu.add(item6)
+            self.menu.add_item(_("Weather"), "weather")
 
         if self.config.display_cd_eject_in_frontend():
-            item7 = Label(size, color, 0, 0, _("Eject CD"), "eject_cd")
-            self.menu.add(item7)
+            self.menu.add_item(_("Eject CD"), "eject_cd")
 
         if self.media_player.has_media():
-            playing = Label(size, color, 0, 0, _("Playing now..."), "playing")
-            self.menu.add(playing)
-
-        self.menu.set_number_of_visible_items(5)
-        menu_clip = self.menu.get_number_of_visible_items() * 70
-
+            self.menu.add_item(_("Playing now..."), "playing")
+
+        self.menu.visible_nb = 5
+        self.menu.selected_index = 2
+
+        menu_clip = self.menu.visible_nb * 70
         # Menu position
         menu_y = int((self.config.get_stage_height() - menu_clip + 10) / 2)
         self.menu.set_position(self.get_abs_x(0.75), menu_y)
 
         self.add(self.menu)
 
-    def show_playing_preview(self):
-        """Create a group that displays information on current media."""
+    def _create_rss_preview_menu(self):
+        '''Create the RSS preview menu that will show feed highlights. An
+        uninitialized menu will be returned to prevent move errors if there
+        is nothing in the feed library.'''
+        menu = TextMenu(self.theme, self.config.show_effects())
+
+        if self.feed_library.is_empty() is False:
+            menu.set_row_count(1)
+            menu.set_position(self.get_abs_x(0.035), self.get_abs_y(0.12))
+            menu.set_item_size(self.get_abs_x(0.549), self.get_abs_y(0.078))
+
+            # List of latest entries. pack = (Feed object, Entry object)
+            entries = self.feed_library.get_latest_entries(5)
+            for pack in entries:
+                text = pack[0].get_title() + " - " + pack[1].get_title()
+                item = TextMenuItem(0.549, 0.078,
+                    FeedEntryParser().strip_tags(text),
+                    pack[1].get_date())
+                kwargs = { 'feed' : pack[0], 'entry' : pack[1] }
+                item.set_userdata(kwargs)
+                menu.add_actor(item)
+
+            menu.set_active(False)
+
+        return menu
+
+    def _create_playing_preview(self):
+        '''Create the Now Playing preview sidebar.'''
+        preview = clutter.Group()
+
         # Video preview of current media
         video_texture = self.media_player.get_texture()
         if video_texture == None:
@@ -141,75 +165,72 @@
         rect.set_size(int(new_width + 6), int(new_height + 6))
         rect.set_position(rect_x, rect_y)
         rect.set_color((128, 128, 128, 192))
-        self.preview.add(rect)
+        preview.add(rect)
 
-        self.preview.add(video_texture)
+        preview.add(video_texture)
 
         title_text = _("Now Playing: %(title)s") % \
             {'title': self.media_player.get_media_title()}
         title = Label(0.03, "text", 0.03, 0.74, title_text)
-        self.preview.add(title)
-
-    def show_rss_preview(self):
-        """Rss preview"""
+        preview.add(title)
+
+        return preview
+
+    def _create_rss_preview(self):
+        '''Create the RSS preview sidebar.'''
+        preview = clutter.Group()
+
+        # RSS Icon
+        icon = Texture(self.theme.getImage("rss_icon"), 0, 0.04)
+        icon.set_scale(0.6, 0.6)
+        preview.add(icon)
+
+        # RSS Feed Title
+        title = Label(0.075, "title", 0.045, 0.03, _("Recent headlines"))
+        preview.add(title)
 
         if self.feed_library.is_empty() is False:
-            # RSS Icon
-            icon = Texture(self.theme.getImage("rss_icon"), 0, 0.04)
-            icon.set_scale(0.6, 0.6)
-
-            # RSS Feed Title
-            title = Label(0.075, "title", 0.045, 0.03, _("Recent headlines"))
-
-            menu = TextMenu(self.theme, self.config.show_effects())
-            menu.set_row_count(1)
-            menu.set_position(self.get_abs_x(0.035), self.get_abs_y(0.12))
-            menu.set_item_size(self.get_abs_x(0.549), self.get_abs_y(0.078))
-
-            # List of latest entries. pack = (Feed object, Entry object)
-            entries = self.feed_library.get_latest_entries(5)
-            for pack in entries:
-                text = pack[0].get_title() + " - " + pack[1].get_title()
-                item = TextMenuItem(0.549, 0.078,
-                    FeedEntryParser().strip_tags(text),
-                    pack[1].get_date())
-                kwargs = { 'feed' : pack[0], 'entry' : pack[1] }
-                item.set_userdata(kwargs)
-                menu.add_actor(item)
-
-            menu.set_active(False)
-
-            # Fade in timeline
+            menu = self._create_rss_preview_menu()
+            preview.add(menu)
+            self.rss_preview_menu = menu
+        else:
+            # No headlines available in the library
+            info = Label(0.05, "title", 0.1, 0.35, _("No headlines available"))
+            preview.add(info)
+
+        return preview
+
+    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())
+        else:
+            update = False
+
+        # If the preview was updated fade it in
+        if update:
             fade_in = clutter.Timeline(20, 60)
             alpha_in = clutter.Alpha(fade_in, clutter.smoothstep_inc_func)
-            self.in_behaviour = clutter.BehaviourOpacity(0x00, 0xff, alpha_in)
-            self.in_behaviour.apply(menu)
-            self.in_behaviour.apply(title)
-            self.in_behaviour.apply(icon)
-
-            self.preview.add(icon)
-            self.preview.add(title)
-            self.rss_preview_menu = menu
-            self.preview.add(menu)
-
-            menu.set_opacity(0x00)
-            icon.set_opacity(0x00)
-            title.set_opacity(0x00)
-
+            self.behaviour = clutter.BehaviourOpacity(0x00, 0xff, alpha_in)
+            self.behaviour.apply(self.preview)
             fade_in.start()
 
+    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:
-            # No headlines available in the library
-            info = Label(0.07, "title", 0.1, 0.35, _("No headlines available"))
-            self.preview.add(info)
-
-    def get_type(self):
-        """Return screen type."""
-        return Screen.NORMAL
-
-    def get_name(self):
-        """Return screen name (human readble)"""
-        return "Main"
+            return True
 
     def update(self):
         """
@@ -219,28 +240,13 @@
         selected_name = self.menu.get_selected().get_name()
 
         self.remove(self.menu)
-        self.create_main_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):
-        """
-        Update preview area. This area displayes information of currently
-        selected menuitem.
-        @param item: Menuitem (clutter.Actor)
-        """
-        self.preview.remove_all()
-        if item.get_name() == "playing":
-            self.show_playing_preview()
-        #only show the rss menu if it has entries
-        elif item.get_name() == "rss":
-            self.show_rss_preview()
+            self.menu.selected_index = index
+
+        self._update_preview_area()
 
     def _move_menu(self, menu_direction):
         '''Move the menu in the given direction.'''
@@ -258,39 +264,34 @@
             if selected.get_name() == None:
                 scroll_direction()
                 selected = self.menu.get_selected()
-            self.update_preview_area(selected)
+            self._update_preview_area(selected)
         else:
             self.rss_preview_menu.move(preview_direction)
 
     def _handle_up(self):
         '''Handle UserEvent.NAVIGATE_UP.'''
-        self._move_menu(self.UP)
+        if self.menu.active:
+            self.menu.scroll_up()
+        else:
+            self.rss_preview_menu.move(TextMenu.UP)
 
     def _handle_down(self):
         '''Handle UserEvent.NAVIGATE_DOWN.'''
-        self._move_menu(self.DOWN)
+        if self.menu.active:
+            self.menu.scroll_down()
+        else:
+            self.rss_preview_menu.move(TextMenu.DOWN)
 
     def _handle_left(self):
         '''Handle UserEvent.NAVIGATE_LEFT.'''
-        if self.feed_library.is_empty():
-            return
-
-        if self.active_menu == "main":
-            self.active_menu = "preview"
-            self.menu.set_active(False)
-            self.rss_preview_menu.set_active(True)
+        if self._can_move_horizontally():
+            self.preview_activation()
 
     def _handle_right(self):
         '''Handle UserEvent.NAVIGATE_RIGHT.'''
-        if self.feed_library.is_empty():
-            return
-
-        if self.active_menu == "preview":
-            self.active_menu = "main"
-            self.menu.set_active(True)
-            self.rss_preview_menu.set_active(False)
-
-    def _handle_select(self):
+        self.menu.active = True
+
+    def _handle_select(self, actor=None):
         '''Handle UserEvent.NAVIGATE_SELECT.'''
         item = self.menu.get_selected()
 
@@ -316,3 +317,22 @@
                 kwargs = menu_item.get_userdata()
                 self.callback("entry", kwargs)
 
+    def _on_menu_moved(self, actor):
+        '''
+        We update the preview area when the selected item changed on the
+        Main menu.
+        '''
+        self._update_preview_area()
+
+    def main_menu_activation(self, actor=None):
+        '''Handle the main menu activation.'''
+        self.menu.active = True
+        if self.rss_preview_menu:
+            self.rss_preview_menu.set_active(False)
+
+    def preview_activation(self, actor=None):
+        '''Handle the preview activation.'''
+        self.menu.active = False
+        if self.rss_preview_menu:
+            self.rss_preview_menu.set_active(True)
+

=== modified file 'entertainerlib/frontend/gui/user_interface.py'
--- entertainerlib/frontend/gui/user_interface.py	2009-03-22 21:00:43 +0000
+++ entertainerlib/frontend/gui/user_interface.py	2009-04-03 16:27:16 +0000
@@ -76,7 +76,10 @@
         #problem
         clutter.set_use_mipmapped_text(False)
 
+        self.hide_cursor_timeout_key = None
+
         self.stage.connect('key-press-event', self.handle_keyboard_event)
+        self.stage.connect('motion-event', self.handle_motion_event)
         self.stage.set_color(self.config.theme.get_color("background"))
         self.stage.set_size(
             self.config.get_stage_width(), self.config.get_stage_height())
@@ -177,7 +180,6 @@
 
     def _fullscreen(self):
         '''Set the window, stage, and config to fullscreen dimensions.'''
-        self.stage.hide_cursor()
         self.window.fullscreen()
         self.stage.fullscreen()
         width, height = self.stage.get_size()
@@ -204,6 +206,7 @@
     def start_up(self):
         '''Start the user interface and make it visible.'''
         self.show()
+        self.stage.hide_cursor()
         self.current = self.create_screen("main")
         self.transition.forward_effect(None, self.current)
         self.enable_menu_overlay()
@@ -217,7 +220,6 @@
         if self.is_fullscreen:
             self.stage.unfullscreen()
             self.window.unfullscreen()
-            self.stage.show_cursor()
             self.config.set_stage_width(self.old_width)
             self.config.set_stage_height(self.old_height)
             self.is_fullscreen = False
@@ -302,6 +304,22 @@
         elif direction == Transition.BACKWARD:
             self.transition.backward_effect(from_screen, screen)
 
+    def hide_cursor_timeout_callback(self):
+        '''Hide the cursor'''
+        self.stage.hide_cursor()
+        return True
+
+    def handle_motion_event(self, stage, clutter_event):
+        '''
+        Show the cursor if it has been moved and start a timeout to
+        hide it after a few seconds.
+        '''
+        self.stage.show_cursor()
+        if self.hide_cursor_timeout_key is not None:
+            gobject.source_remove(self.hide_cursor_timeout_key)
+        self.hide_cursor_timeout_key = gobject.timeout_add(3000,
+            self.hide_cursor_timeout_callback)
+
     def handle_keyboard_event(self, stage, clutter_event, event_handler=None):
         '''Translate all received keyboard events to UserEvents.'''
         if event_handler is None:

=== 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-03 20:23:10 +0000
@@ -0,0 +1,103 @@
+"""Tools for pointer motion calculations"""
+
+__licence__ = "GPLv2"
+__copyright__ = "2009, Samuel Buffet"
+__author__ = "Samuel Buffet <[e-mail]>"
+
+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
+
+        # 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
+
+    def start(self, event):
+        '''Has to be called when we start a pointer motion'''
+        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
+
+    def take_new_motion_event(self, event):
+        '''
+        Has to be called by a method connected to the *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)
+
+        if self.dt_from_last_motion_event != 0 :
+            self.speed_x_from_last_motion_event = \
+                float(self.dx_from_last_motion_event) / \
+                float(self.dt_from_last_motion_event)
+            self.speed_y_from_last_motion_event = \
+                float(self.dy_from_last_motion_event) / \
+                float(self.dt_from_last_motion_event)
+            self.speed_from_last_motion_event = \
+                float(self.distance_from_last_motion_event) / \
+                float(self.dt_from_last_motion_event)
+        else:
+            self.speed_x_from_last_motion_event = 0
+            self.speed_y_from_last_motion_event = 0
+            self.speed_from_last_motion_event = 0
+

=== 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-08 11:23:26 +0000
@@ -2,209 +2,271 @@
 
 __licence__ = "GPLv2"
 __copyright__ = "2007, Lauri Taimila"
-__author__ = "Lauri Taimila <[e-mail]>"
+__author__ = ("Lauri Taimila <[e-mail]>",
+              "Samuel Buffet <[e-mail]>")
 
 import clutter
-
-class ScrollMenu(clutter.Group):
+import math
+import gobject
+
+from entertainerlib.frontend.gui.widgets.label import Label
+from entertainerlib.frontend.gui.widgets.motion_buffer import MotionBuffer
+
+class LoopedPathBehaviour (clutter.Behaviour):
+    """A custome Path behaviour driven by an index [0, 1]"""
+    __gtype_name__ = 'LoopedPathBehaviour'
+
+    def __init__(self, alpha=None):
+        clutter.Behaviour.__init__(self)
+        self.set_alpha(alpha)
+        self.start_index = 0.0
+        self.end_index = 1.0
+        self.start_knot = (0, 0)
+        self.end_knot = (10, 10)
+
+    def do_alpha_notify (self, alpha_value):
+        """
+        We position the actor on the Line defined by the 2 knots according to
+        the alpha value.
+        alpha = 0         position = start_knot
+        alpha = MAX_ALPHA position = end_knot
+        """
+        idx = alpha_value * (self.end_index - self.start_index)
+        idx /= clutter.MAX_ALPHA
+        idx += self.start_index
+
+        if idx >= 0 :
+            idxx = math.modf(idx)[0]
+        else:
+            idxx = 1 + math.modf(idx)[0]
+
+        # Calculation of new coordinates
+        x = idxx * (self.end_knot[0] - self.start_knot[0]) + self.start_knot[0]
+        y = idxx * (self.end_knot[1] - self.start_knot[1]) + self.start_knot[1]
+
+        for actor in self.get_actors():
+            actor.set_position(int(x), int(y))
+
+    @ property
+    def path_length(self):
+        """Calculation of the path's length in pixels'"""
+        dx = self.end_knot[0] - self.start_knot[0]
+        dy = self.end_knot[1] - self.start_knot[1]
+        return math.sqrt(dx * dx + dy * dy)
+
+class ScrollMenuItem(clutter.Group):
+    """
+    This is a Group containing a Label to which a LoopedPathBehaviour is applied
+    """
+    __gtype_name__ = 'ScrollMenuItem'
+
+    def __init__(self, alpha, text, item_height, font_size, color_name):
+        clutter.Group.__init__(self)
+
+        self.label = Label(font_size, color_name, 0, 0)
+        self.label.set_text(text)
+
+        self.behaviour = LoopedPathBehaviour(alpha)
+        self.behaviour.apply(self)
+
+        clutter.Group.add(self, self.label)
+
+class ScrollMenu(clutter.Group, object):
     """
     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
-        """
+    __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.__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
-
-    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
-
-    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)
-
-            # 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))
-
-            # Set first item to last (menu scrolls)
-            first = self.__items[0]
-            first.set_position(
-                0, self.__items[-1].get_y() + self.__gap + self.__item_height)
-            self.__items.append(first)
-            del self.__items[0]
-
-    def scroll_items_cb(self, timeline, frame_num):
-        """Callback function of scroll_down_timeline"""
-        delta = timeline.get_delta()[0]
-        offset = int((self.__item_height + self.__gap) / 7 * delta)
-        if self.__scroll_direction == 1:
-            offset = - offset
-        for item in self.__items:
-            item.move_by(0, offset)
-
-    def is_active(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_nb = 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
+
+        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 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._on_item_notify_y)
+        clutter.Group.add(self, item)
+
+        self._items.append(item)
+
+    def _get_active(self):
         """
         Is this menu active.
         @return boolean, True if menu is currently active, otherwise False
         """
-        return self.__active
+        return self._active
 
-    def set_active(self, boolean):
+    def _set_active(self, boolean):
         """
         Set this widget active or inactive
         @param boolean: True to set widget active, False to set inactive
         """
-        self.__active = boolean
+        if self._active == boolean:
+            return
+
+        self._active = boolean
         if boolean:
-            #if self.__selector is not None:
-            #    self.__selector.show()
             self.set_opacity(255)
+            self.emit('activated')
         else:
-            #if self.__selector is not None:
-            #    self.__selector.hide()
             self.set_opacity(128)
 
-    def update(self):
-        """Update the whole menu"""
-        for item in self.__items:
-            opacity = self.get_opacity_for_y(item.get_y())
-            item.set_opacity(opacity)
+    active = property(_get_active, _set_active)
+
+    def _update_behaviours(self, target):
+        """
+        Preparation of looped behaviours applied to menu items before
+        we move them.
+        """
+        nbi = len(self._items)
+        step = 1.0 / nbi
+        step_pix = self._item_gap + self._item_height
+        a = int(self._visible_nb / 2) + 1
+        for x, item in enumerate(self._items):
+            item.behaviour.start_index = (x + a - self._selected_index) * step
+            item.behaviour.end_index = (x + a - target) * step
+
+            item.behaviour.start_knot = (0.0, -step_pix)
+            item.behaviour.end_knot = (0.0, (nbi - 1.0) * step_pix)
+
+    def _display_items_at_target(self, target):
+        """
+        This method draws the menu for a particular targeted value of the
+        driving index.
+        """
+        nbi = len(self._items)
+        step = 1.0 / nbi
+
+        a = int(self._visible_nb / 2) + 1
+        for x, item in enumerate(self._items):
+            index = (x + a - target) * step
+
+            if index >= 0 :
+                idxx = math.modf(index)[0]
+            else:
+                idxx = 1 + math.modf(index)[0]
+
+            # Calculation of new coordinates
+            xx = idxx * (item.behaviour.end_knot[0] - \
+                item.behaviour.start_knot[0]) + item.behaviour.start_knot[0]
+            yy = idxx * (item.behaviour.end_knot[1] - \
+                item.behaviour.start_knot[1]) + item.behaviour.start_knot[1]
+
+            item.set_position(int(xx), int(yy))
+
+    def _get_visible_nb(self):
+        """Return the visible_nb property"""
+        return self._visible_nb
+
+    def _set_visible_nb(self, visible_nb):
+        """Set the visible_nb property"""
+        self._visible_nb = visible_nb
+        h = visible_nb * self._item_height + (visible_nb - 1) * self._item_gap
+        self.set_clip(0, 0, self.get_width(), h)
+
+    visible_nb = property(_get_visible_nb, _set_visible_nb)
+
+    def get_selected(self):
+        """
+        Get currently selected menuitem
+        @return Clutter actor object
+        """
+        return self._items[int(self._selected_index)]
+
+    def _get_selected_index(self):
+        """Return the selected_index property"""
+        return self._selected_index
+
+    def _set_selected_index(self, selected_index, duration=300):
+        """Set the selected_index property"""
+        if not self._timeline.is_playing():
+            nb = len(self._items)
+            self._update_behaviours(selected_index)
+
+            # those 2 variables are used if we want to stop the timeline
+            # we use them + timeline progression to calculate the current index
+            # when (if) we stop
+            self._animation_start_index = self._selected_index
+            self._animation_end_index = selected_index
+
+            # compute selected_index to be between 0 and nb
+            if selected_index >= 0:
+                self._selected_index = selected_index - \
+                    math.modf(selected_index / nb)[1] * nb
+            else:
+                self._selected_index = selected_index + \
+                    (math.modf(-(selected_index + 1) / nb)[1] + 1) * nb
+
+            self._timeline.set_duration(duration)
+            self._timeline.start()
+
+            self.emit('moved')
+
+    selected_index = property(_get_selected_index, _set_selected_index)
+
+    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
+
+    def scroll_by(self, nb, duration=300):
+        """Set the selected_index property"""
+        self._set_selected_index(self._selected_index + nb, duration)
+
+    def scroll_up(self, duration=300):
+        """Set the selected_index property"""
+        self.scroll_by(-1, duration)
+
+    def scroll_down(self, duration=300):
+        """Set the selected_index property"""
+        self.scroll_by(1, duration)
 
     def get_opacity_for_y(self, y):
-        """Calculation of actor's opacity as a function of its y coordinate"""
-        opacity_first_item = 30
+        """Calculation of actor's opacity as a function of its y coordinates"""
+        opacity_first_item = 40
         opacity_selected_item = 255
+        o = int(self._visible_nb / 2)
 
-        y_medium_item = 2 * (self.__item_height + self.__gap)
+        y_medium_item = o * (self._item_height + self._item_gap)
         a  = float(opacity_selected_item - opacity_first_item)
         a /= float(y_medium_item)
         if y <= y_medium_item :
@@ -217,8 +279,116 @@
 
         return int(opacity)
 
-    def actor_notify_y(self, actor, stage):
+    def _on_item_notify_y(self, item, stage):
         """Set opacity to actors when they are moving. Opacity is f(y)"""
-        opacity = self.get_opacity_for_y(actor.get_y())
-        actor.set_opacity(opacity)
+        opacity = self.get_opacity_for_y(item.get_y())
+        item.set_opacity(opacity)
+
+    def _on_button_press_event(self, actor, event):
+        """button-press-event handler"""
+        clutter.grab_pointer(self)
+        if not self.handler_is_connected(self._motion_handler):
+            self._motion_handler = self.connect('motion-event',
+            self._on_motion_event)
+        self.active = True
+
+        if self._timeline.is_playing():
+            # before we stop the timeline, store its progression
+            self._animation_progression = self._timeline.get_progress()
+
+            # A click with an animation pending should stop the animation
+            self._timeline.stop()
+
+            # go to MODE_STOP to handle correctly next button-release event
+            self._event_mode = self.MODE_STOP
+        else:
+            # no animation pending so we're going to do either a menu_item
+            # selection or a menu motion. This will be decided later, right now
+            # we just take a snapshot of this button-press-event as a start.
+            self._motion_buffer.start(event)
+            self._event_mode = self.MODE_SELECTION
+
+        return False
+
+    def _on_button_release_event(self, actor, event):
+        """button-release-event handler"""
+        clutter.ungrab_pointer()
+        if self.handler_is_connected(self._motion_handler):
+            self.disconnect_by_func(self._on_motion_event)
+        self._motion_buffer.compute_from_last_motion_event(event)
+
+        y = event.y - self.get_y()
+
+        if self._event_mode == self.MODE_SELECTION:
+            # if we are in MODE_SELECTION it means that we want to select
+            # the menu item bellow the pointer
+
+            for idx, item in enumerate(self._items):
+                b = item.get_y()
+                h = item.get_height()
+                if (y >= b) and (y <= (b + h)):
+                    delta1 = idx - self._selected_index
+                    delta2 = idx - self._selected_index + len(self._items)
+                    delta3 = idx - self._selected_index - len(self._items)
+
+                    delta = 99999
+                    for i in [delta1, delta2, delta3]:
+                        if math.fabs(i)<math.fabs(delta):
+                            delta = i
+
+                    self.scroll_by(delta)
+
+                    # if delta = 0 it means we've clicked on the selected item
+                    if delta == 0:
+                        self.emit('selected')
+
+        elif self._event_mode == self.MODE_MOTION:
+            speed = self._motion_buffer.speed_y_from_last_motion_event
+            target = self._selected_index - \
+                self._motion_buffer.dy_from_start / \
+                self._items[0].behaviour.path_length * len(self._items)
+
+            new_index = int(target - 5 * speed)
+            self._selected_index = target
+            self._set_selected_index(new_index, 1000)
+
+        else:
+            # If we have stopped the pending animation. Now we have to do 
+            # a small other one to select the closest menu-item
+            current_index = self._animation_start_index + \
+                (self._animation_end_index - self._animation_start_index) * \
+                self._animation_progression
+            self._selected_index = current_index
+            target_index = int(current_index)
+            self._set_selected_index(target_index, 1000)
+
+        return False
+
+    def _on_motion_event(self, actor, event):
+        """motion-event handler"""
+        self._motion_buffer.compute_from_start(event)
+        if self._motion_buffer.distance_from_start > 10:
+            self._motion_buffer.take_new_motion_event(event)
+            self._event_mode = self.MODE_MOTION
+            target = self._selected_index - \
+                self._motion_buffer.dy_from_start / \
+                self._items[0].behaviour.path_length * len(self._items)
+            self._display_items_at_target(target)
+
+        self.get_stage().show_cursor()
+
+        return False
+
+    def _on_scroll_event(self, actor, event):
+        """scroll-event handler (mouse's wheel)"""
+        self.active = True
+
+        if event.direction == clutter.SCROLL_DOWN:
+            self.scroll_down(150)
+        else:
+            self.scroll_up(150)
+
+        self.get_stage().show_cursor()
+
+        return False
 

=== 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-07 11:31:48 +0000
@@ -0,0 +1,36 @@
+"""Tests ScrollMenu"""
+
+__license__ = "GPLv2"
+__copyright__ = "2009, Samuel Buffet"
+__author__ = "Samuel Buffet <[e-mail]>"
+
+import clutter
+
+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 testCreate(self):
+        """Test correct ScrollArea initialization"""
+        self.assertTrue(isinstance(self.menu, (clutter.Group, object)))
+