Merge lp:~samuel-buffet/entertainer/fixme-27 into lp:entertainer

[Samuel Buffet]

[Samuel Buffet (samuel-buffet)]

A proposal for FIXME 27, 28 according to http://wiki.entertainer-project.com/wiki/TechnicalDebt0-3

Samuel,

diff :

=== modified file 'entertainerlib/frontend/gui/screens/main_screen.py'
--- entertainerlib/frontend/gui/screens/main_screen.py 2008-11-17 20:51:41 +0000
+++ entertainerlib/frontend/gui/screens/main_screen.py 2008-11-24 21:35:11 +0000
@@ -67,7 +67,7 @@

     def create_main_menu(self):
         """Create main menu of the home screen"""
- self.menu = ScrollMenu(10, 60)
+ self.menu = ScrollMenu(10, 60, self.config.show_effects())
         self.menu.set_name("mainmenu")

         # Common values for all ScrollMenu items
@@ -248,6 +248,8 @@
         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):

=== modified file 'entertainerlib/frontend/gui/widgets/scroll_menu.py'
--- entertainerlib/frontend/gui/widgets/scroll_menu.py 2008-11-17 21:48:56 +0000
+++ entertainerlib/frontend/gui/widgets/scroll_menu.py 2008-11-24 21:36:12 +0000
@@ -29,22 +29,29 @@
         self.__item_height = item_height
         self.__animated = animated

+ self.scroll_down_timeline = clutter.Timeline(7, 26)
+ self.scroll_down_timeline.connect('new-frame', self.scroll_items_down)
+
+ self.scroll_up_timeline = clutter.Timeline(7, 26)
+ self.scroll_up_timeline.connect('new-frame', self.scroll_items_up)
+
     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)
- try:
- self.__update_opacity()
- except:
- pass

     def remove(self, actor):
         """
@@ -140,53 +147,54 @@
         """
         return self.__items[2]

+ def menu_is_moving(self):
+ """Return True if a menu animation is in progress"""
+ return self.scroll_down_timeline.is_playing() or \
+ self.scroll_up_timeline.is_playing()
+
     def scroll_down(self):
         """Scroll menu down by one menuitem."""
- if self.__animated:
- #FIXME: Animation doesn't work
- timeline = clutter.Timeline(7, 26)
- alpha = clutter.Alpha(timeline, clutter.ramp_inc_func)
- behaviour = clutter.BehaviourPath(
- alpha, ((0,0),(0,self.__gap + self.__item_height)))
- for item in self.__items:
- behaviour.apply(self)
- 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() - se...

[Matt Layman (mblayman)]

Samuel, first, let me add, this looks sweet! Okay, on to the comment (I think this is 95% complete):

For the methods that are private to scroll_menu can you please add a single underscore to them to give a visual indicator that they are private? For example, actor_notify_y to _actor_notify_y.

The methods I'm referring to are:
actor_notify_y
get_opacity_for_y
scroll_items_down
scroll_items_up

Just for my knowledge can you explain the concepts around connect "new-frame" and connect "notify::y". I'm assuming that these come directly from clutter and should be applied whenever we do animation, but since your code seems to work so well, I was hoping you might explain it so I could apply the concept to any frontend code that I happen to work on.

I'm going to go ahead and approve on the condition that my comment above is addressed.

review: Approve

[Matt Layman (mblayman)]

Oops, I forgot a comment. I noticed that this change reintroduced the bug, that a user can focus on the empty spot between Weather and Play CD. Can this be corrected too? Thanks.

[Samuel Buffet (samuel-buffet)]

>Oops, I forgot a comment. I noticed that this change reintroduced the bug, that a user can focus on
>the empty spot between Weather and Play CD. Can this be corrected too? Thanks.

Indeed. I'll rework that fix. One more proof that Reviews are efficient :)

>Just for my knowledge can you explain the concepts around connect "new-frame" and connect
>"notify::y". I'm assuming that these come directly from clutter and should be applied whenever we do
>animation, but since your code seems to work so well, I was hoping you might explain it so I could
>apply the concept to any frontend code that I happen to work on.

*new-frame* is a timeline signal wich is emited on every new-frame
*notify::y* is a signal emited when the y property of an Actor has been changed. This is not specific to clutter this is gobject property modification signal. I hook it to calculate the opacity of items.

Samuel,

[lp:~samuel-buffet/entertainer/fixme-27]

298. By Samuel Buffet on 2008-11-26

simplifications in the code.

299. By Samuel Buffet on 2008-11-26

remove the separator

[Samuel Buffet (samuel-buffet)]

Well, in fact it's not that easy to avoid the selection of the separator without a global rework of the widget.

So my suggestion is to remove the separator as it's not something really creating added value to that widget ???

[Joshua Scotton (joshuascotton)]

I think that's ok

[Joshua Scotton (joshuascotton)]

make check is ok, entertainer works better with these changes.
No problems with the code, thanks a lot samuel!

review: Approve