Merge lp:~mblayman/entertainer/preview-problem into lp:entertainer

[Matt Layman]

The main menu now keeps focus appropriately for left and right directions and all previews now fade in. (Matt Layman)

[Matt Layman (mblayman)]

I was reviewing Samuel's branch on the main menu focus issue (Bug 357056) and thought the problem was bigger than branch submitted. I decided to hack on it because my brain was having some trouble spelling out the problems with the branch (aside from how to break the patch).

I was left with two results:
1) A fix for the problem that doesn't break. Samuel, it turns out you just had the item.get_name() check in the wrong if statement.
2) The preview menu code on the main screen has been cleaned up quite a bit. Now the preview will fade in for either the RSS or the Now Playing preview.

A note on the second point. I changed some method names because they were fairly deceptive. I changed the "show_playing_preview" and "show_rss_preview" because they weren't really showing those previews. They actually recreate the menus and display them again. I changed "show" to "_create" so that it's clear what they are doing. Maybe someone will want to change them in the future to save calls to the feed library or prevent recreating a video texture, but it will at least be clear what they'll have to change.

In summary, I probably went way overboard to fix the bug in the branch that I was supposed to be reviewing for Samuel, but I think that the result is quite nice anyway.

[Samuel Buffet (samuel-buffet)]

Good Matt,

I approve those changes.

FYI, I've found the idea so good that I've merged those changes in *new-scrollmenu* to be able to continue modifications/improvements to take care of this issue in the context of the new signal based menu.

Thanks

Samuel-

review: Approve

[Jamie Bennett (jamiebennett)]

Good work Matt. Thorough and good code reuse.

 review approve

On Wed, 2009-04-08 at 02:55 +0000, Matt Layman wrote:
> Matt Layman has proposed merging lp:~laymansterms/entertainer/preview-problem into lp:entertainer.
>
> Requested reviews:
> Entertainer Release Team (entertainer-releases)
>
> I was reviewing Samuel's branch on the main menu focus issue (Bug 357056) and thought the problem was bigger than branch submitted. I decided to hack on it because my brain was having some trouble spelling out the problems with the branch (aside from how to break the patch).
>
> I was left with two results:
> 1) A fix for the problem that doesn't break. Samuel, it turns out you just had the item.get_name() check in the wrong if statement.
> 2) The preview menu code on the main screen has been cleaned up quite a bit. Now the preview will fade in for either the RSS or the Now Playing preview.
>
> A note on the second point. I changed some method names because they were fairly deceptive. I changed the "show_playing_preview" and "show_rss_preview" because they weren't really showing those previews. They actually recreate the menus and display them again. I changed "show" to "_create" so that it's clear what they are doing. Maybe someone will want to change them in the future to save calls to the feed library or prevent recreating a video texture, but it will at least be clear what they'll have to change.
>
> In summary, I probably went way overboard to fix the bug in the branch that I was supposed to be reviewing for Samuel, but I think that the result is quite nice anyway.

review: Approve

[Matt Layman (mblayman)]

Commit Message: The main menu now keeps focus appropriately for left and right directions and all previews now fade in. (Matt Layman)

=== 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 02:31:06 +0000
@@ -44,10 +44,12 @@
         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.rss_preview_menu = self._create_rss_preview_menu()
+        self.preview.add(self.rss_preview_menu)
 
         self.menu = None
         self.playing_item = None
@@ -56,6 +58,32 @@
         clock = ClockLabel(0.13, "screentitle", 0, 0.87)
         self.add(clock)
 
+    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_main_menu(self):
         """Create main menu of the home screen"""
         self.menu = ScrollMenu(10, 60, self.config.show_effects())
@@ -101,8 +129,10 @@
 
         self.add(self.menu)
 
-    def show_playing_preview(self):
-        """Create a group that displays information on current media."""
+    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,67 +171,40 @@
         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
-            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)
+            menu = self._create_rss_preview_menu()
+            preview.add(menu)
             self.rss_preview_menu = menu
-            self.preview.add(menu)
-
-            menu.set_opacity(0x00)
-            icon.set_opacity(0x00)
-            title.set_opacity(0x00)
-
-            fade_in.start()
-
         else:
             # No headlines available in the library
-            info = Label(0.07, "title", 0.1, 0.35, _("No headlines available"))
-            self.preview.add(info)
+            info = Label(0.05, "title", 0.1, 0.35, _("No headlines available"))
+            preview.add(info)
+
+        return preview
 
     def get_type(self):
         """Return screen type."""
@@ -227,20 +230,28 @@
 
         self.menu.update()
 
-        self.update_preview_area(self.menu.get_selected())
+        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)
-        """
+    def _update_preview_area(self, item):
+        '''Update the preview area to display the current menu item.'''
         self.preview.remove_all()
+        self.preview.set_opacity(0x00)
+
+        update = True
         if item.get_name() == "playing":
-            self.show_playing_preview()
-        #only show the rss menu if it has entries
+            self.preview.add(self._create_playing_preview())
         elif item.get_name() == "rss":
-            self.show_rss_preview()
+            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.behaviour = clutter.BehaviourOpacity(0x00, 0xff, alpha_in)
+            self.behaviour.apply(self.preview)
+            fade_in.start()
 
     def _move_menu(self, menu_direction):
         '''Move the menu in the given direction.'''
@@ -258,7 +269,7 @@
             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)
 
@@ -270,9 +281,17 @@
         '''Handle UserEvent.NAVIGATE_DOWN.'''
         self._move_menu(self.DOWN)
 
+    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:
+            return True
+
     def _handle_left(self):
         '''Handle UserEvent.NAVIGATE_LEFT.'''
-        if self.feed_library.is_empty():
+        if not self._can_move_horizontally():
             return
 
         if self.active_menu == "main":
@@ -282,7 +301,7 @@
 
     def _handle_right(self):
         '''Handle UserEvent.NAVIGATE_RIGHT.'''
-        if self.feed_library.is_empty():
+        if not self._can_move_horizontally():
             return
 
         if self.active_menu == "preview":