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

This code is essentially a relic of some of Lauri's stuff that I just never got around to cleaning up. I think that the only place name is used is when making decisions about whether to quit of not. type is used for some of the overlay logic for different screen types (normal vs osd).

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

I think this needs some clarification, because if you are referring to all three variables, I think I disagree. I wouldn't mind seeing the author line go away (as long as we create an AUTHORS file in docs) because I can understand how other people would be intimidated by messing with someone's apparent "turf." I do think, however, that the license and copyright variables are important to be in line with the spirit of the GPL.

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

active is used by the Main screen so it needs the accessor methods

> > === added file 'entertainerlib/tests/test_scrollmenu.py'
> > --- entertainerlib/tests/test_scrollmenu.py   1970-01-01 00:00:00 +0000
> > +++ entertainerlib/tests/test_scrollmenu.py   2009-04-25 23:30:15 +0000
> > @@ -0,0 +1,33 @@
> > +"""Tests ScrollMenu"""
> > +
> > +__license__ = "GPLv2"
> > +__copyright__ = "2009, Samuel Buffet"
> > +__author__ = "Samuel Buffet <email address hidden>"
> > +
> > +from entertainerlib.frontend.gui.widgets.scroll_menu import ScrollMenu
> > +from entertainerlib.tests import EntertainerTest
> > +
> > +class ScrollMenuTest(EntertainerTest):
> > +    """Test for entertainerlib.frontend.gui.widgets.scroll_menu"""
> > +
> > +    def setUp(self):
> > +        """Set up the test"""
> > +        EntertainerTest.setUp(self)
> > +
> > +        self.menu = ScrollMenu(10, 60, 0.045, "menuitem_active")
> > +        self.menu.set_name("mainmenu")
> > +
> > +        self.menu.add_item(_("Play CD"), "disc")
> > +        self.menu.add_item(_("Videos"), "videos")
> > +        self.menu.add_item(_("Music"), "music")
> > +        self.menu.add_item(_("Photographs"), "photo")
> > +        self.menu.add_item(_("Headlines"), "rss")
> > +
> > +    def tearDown(self):
> > +        """Clean up after the test"""
> > +        EntertainerTest.tearDown(self)
> > +
> > +    def test_create(self):
> > +        """Test correct ScrollArea initialization"""
> > +        self.assertTrue(isinstance(self.menu, ScrollMenu))
> > +
> 
> I'd like to see more tests, especially for the new classes.  There's probably
> a
> bit that just can't be tested, but the state tracking itself probably could be
> tested pretty well, to make sure it retains state properly.

ScrollMenu is a hard thing to test, but checking the state that's retained would be a good idea like you suggest.