Merge lp:~rockstar/entertainer/entertainer-server into lp:entertainer

[Paul Hummer]

[Paul Hummer (rockstar)]

This branch adds the start of the new server code. There's a lot here, and
it's just a proving a working concept, but I think it's important that it gets
into trunk. I re-arranged some of the modules to make more sense, and to
avoid things like "from
entertainerlib.really.long.import.that.breaks.the.line import something"
I've created a few scripts. I know the server and temporary client are
working now because I can run the entertainer-server script in one terminal,
and then run the client script in another terminal, and get the list of dicts
passed across. This is a simple static implementation of a command that will
be used.

What I need to do now is create a new indexer that will give me data in the
new data format (using the Storm models). Once I have a full database
implementation, I can start writing good tests for the current server
implementation, and then move on to the full server implementation.

The plan after that is to make the frontend merely a network client, which
will be rather trivial compared to these other two steps.

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F

[Samuel Buffet (samuel-buffet)]

Hi Paul,

So this is the beginning of the new server. Great, I hope we'll do a lot of good things with it.

Here is my review :

goal of the branch : ok
make test : ok
make lint : ok

>=== modified file 'cfg/preferences.conf'
>--- cfg/preferences.conf 2009-03-08 21:29:32 +0000
>+++ cfg/preferences.conf 2009-04-05 18:22:45 +0000
>@@ -5,7 +5,7 @@
> stage_width = 1366
> stage_height = 768
> show_effects = True
>-start_in_fullscreen = True
>+start_in_fullscreen = False

Paul, I think you can let *True* here. Don't you prefer to start Entertainer in Fullscreen ?

=== modified file 'entertainerlib/frontend/__init__.py'
--- entertainerlib/frontend/__init__.py 2009-03-08 03:16:25 +0000
+++ entertainerlib/frontend/__init__.py 2009-04-05 18:22:45 +0000
@@ -1,15 +1,10 @@
 '''Frontend gui to entertainer'''
-# pylint: disable-msg=W0612

 def main(*args, **kwargs):
     '''Frontend runner'''

>- # Import statements are inside the function so that they aren't imported
>+ # Import statements are inside thu function so that they aren't imported

Hmm, I'm not sure because I'm far from being an English expert but I can't understand *thu*.

=== modified file 'entertainerlib/frontend/gui/screens/movie.py'
--- entertainerlib/frontend/gui/screens/movie.py 2009-03-21 22:43:10 +0000
+++ entertainerlib/frontend/gui/screens/movie.py 2009-04-05 18:22:45 +0000
@@ -214,8 +214,8 @@
         if self.menu.is_active():
             item = self.menu.get_current_menuitem().get_userdata()
             if item == "watch":
- self.media_player.set_media(self.movie)
- self.media_player.play()
+ self.mediaplayer.set_media(self.movie)
+ self.mediaplayer.play()
                 self.callback("video_osd")

Paul, this is really media_player and not mediaplayer otherwise we have an error. This was fixed by laymansterms recently so I'm wondering if this branch merged recently ?

review: Needs Fixing

[Samuel Buffet (samuel-buffet)]

Hi Paul,

I've merged your branch with latest trunk (no conflicts) to have a smaller diff :

=== added file 'entertainer-client'
--- entertainer-client 1970-01-01 00:00:00 +0000
+++ entertainer-client 2009-04-05 18:22:45 +0000
@@ -0,0 +1,10 @@
+#!/usr/bin/env python
+'''Test client for Entertainer's server.
+
+This code will go away when the client code is integrated into what is now the
+frontend.
+'''
+
+from entertainerlib.network import client_main
+
+client_main()

=== added file 'entertainer-server'
--- entertainer-server 1970-01-01 00:00:00 +0000
+++ entertainer-server 2009-04-05 18:22:45 +0000
@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+'''Server executable'''
+
+from entertainerlib.network import server_main
+
+server_main()
+

=== modified file 'entertainerlib/backend/core/db/models.py'
--- entertainerlib/backend/core/db/models.py 2009-01-31 21:47:47 +0000
+++ entertainerlib/backend/core/db/models.py 2009-04-05 18:22:45 +0000
@@ -5,6 +5,7 @@
 from storm.properties import Bool, DateTime, Int, Unicode
 from storm.references import Reference, ReferenceSet

+
 class BaseModel(Storm):
     '''Abstract class from which all Entertainer models inherit.'''

=== added directory 'entertainerlib/network'
=== added file 'entertainerlib/network/__init__.py'
--- entertainerlib/network/__init__.py 1970-01-01 00:00:00 +0000
+++ entertainerlib/network/__init__.py 2009-04-05 18:22:45 +0000
@@ -0,0 +1,38 @@
+'''Network functionality for Entertainer.'''
+import sys
+
+from twisted.internet import reactor
+from twisted.internet.protocol import ClientCreator
+from twisted.python.log import startLogging
+
+from entertainerlib.network.local.server import EntertainerLocalServer
+from entertainerlib.network.local.client import EntertainerLocalClientProtocol
+
+# Entertainer really needs a good absctractable way to handle command line
+# arguments.
+def server_main(*args, **kwargs):
+ '''Entertainer Server main function.'''
+ # When multiple server types are supported, the following variable will be
+ # more dynamic.
+ startLogging(sys.stdout)
+ server_type = EntertainerLocalServer
+
+ server = server_type()
+ # TODO: This port number could probably be a config var.
+ reactor.listenTCP(5545, server)
+ reactor.run()
+
+
+def client_main():
+ '''Entertainer test client code.
+
+ This code will go away when what is now the frontend is made into a true
+ client.
+ '''
+ startLogging(sys.stdout)
+ client = EntertainerLocalClientProtocol
+
+ ClientCreator(reactor, client).connectTCP(
+ 'localhost', 5545)
+ reactor.run()
+

=== added directory 'entertainerlib/network/local'
=== added file 'entertainerlib/network/local/__init__.py'
--- entertainerlib/network/local/__init__.py 1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/__init__.py 2009-04-05 18:22:45 +0000
@@ -0,0 +1,1 @@
+'''Local network storage and server package.'''

=== added file 'entertainerlib/network/local/client.py'
--- entertainerlib/network/local/client.py 1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/client.py 2009-04-05 18:22:45 +0000
@@ -0,0 +1,35 @@
+'''Classes to produce a local storage implementation.'''
+# pylint: d...

[Samuel Buffet (samuel-buffet)]

Hi Paul,

Forget my previous comments here are the good ones :

Here is my review :

goal of the branch : ok
make test : ok
make lint : ok

Ok, the code looks good to me. Entertainer and its backend seems to work.

Thanks to make our backend better.
Samuel-

[Samuel Buffet (samuel-buffet)]

review: Approve

[Jamie Bennett (jamiebennett)]

I'm not sure this should land in trunk just yet. As you say its concept
code that isn't used and hence my view would be to continue to mature it
in its own branch. Although it isn't a show stopper to merge it
(explaining my review decision).

Code wise, from my little twisted knowledge, looks OK as a dummy test of
the functionality. One comment, I would definitely make the port number
a config variable as your comments allude to. Port clashing should be
avoided and as this port could be anything (its non-standard) we can't
presume it isn't already in use on our users systems.

 review abstain

On Sat, 2009-04-04 at 17:05 +0000, Paul Hummer wrote:
> Paul Hummer has proposed merging lp:~rockstar/entertainer/entertainer-server into lp:entertainer.
>
> Requested reviews:
> Entertainer Release Team (entertainer-releases)
>
> This branch adds the start of the new server code. There's a lot here, and
> it's just a proving a working concept, but I think it's important that it gets
> into trunk. I re-arranged some of the modules to make more sense, and to
> avoid things like "from
> entertainerlib.really.long.import.that.breaks.the.line import something"
> I've created a few scripts. I know the server and temporary client are
> working now because I can run the entertainer-server script in one terminal,
> and then run the client script in another terminal, and get the list of dicts
> passed across. This is a simple static implementation of a command that will
> be used.
>
> What I need to do now is create a new indexer that will give me data in the
> new data format (using the Storm models). Once I have a full database
> implementation, I can start writing good tests for the current server
> implementation, and then move on to the full server implementation.
>
> The plan after that is to make the frontend merely a network client, which
> will be rather trivial compared to these other two steps.
>
> --
> Paul Hummer
> http://theironlion.net
> 1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F
>

review: Abstain

[Matt Layman (mblayman)]

I know you said you'll be merging this into some other branch, so I'm approving on those grounds (because there looks to be some things that you just wouldn't merge onto a production trunk), but here are my comments.

1) Port number stuff is already in the Configuration class. That could be used, but in the long run I don't know how we could "announce" that a server is available to connect to (is that something that would happen if we eventually add uPnP support?).

2) I'm assuming that pylint W0223 will be removed as soon as LocalStorage actually implements the abstract Storage methods. I think that could benefit from a XXX comment to help remember.

3) pylint R0922 could also benefit from a XXX comment in the pylintrc file. I don't think that the intention would be to remove it permanently as it seems to add value (especially if we start using more abstract classes).

review: Approve

=== added file 'entertainer-client'
--- entertainer-client	1970-01-01 00:00:00 +0000
+++ entertainer-client	2009-04-04 05:30:04 +0000
@@ -0,0 +1,10 @@
+#!/usr/bin/env python
+'''Test client for Entertainer's server.
+
+This code will go away when the client code is integrated into what is now the
+frontend.
+'''
+
+from entertainerlib.network import client_main
+
+client_main()

=== added file 'entertainer-server'
--- entertainer-server	1970-01-01 00:00:00 +0000
+++ entertainer-server	2009-04-04 04:24:46 +0000
@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+'''Server executable'''
+
+from entertainerlib.network import server_main
+
+server_main()
+

=== modified file 'entertainerlib/backend/core/db/models.py'
--- entertainerlib/backend/core/db/models.py	2009-01-31 21:47:47 +0000
+++ entertainerlib/backend/core/db/models.py	2009-04-04 04:27:46 +0000
@@ -5,6 +5,7 @@
 from storm.properties import Bool, DateTime, Int, Unicode
 from storm.references import Reference, ReferenceSet
 
+
 class BaseModel(Storm):
     '''Abstract class from which all Entertainer models inherit.'''
 

=== added directory 'entertainerlib/network'
=== added file 'entertainerlib/network/__init__.py'
--- entertainerlib/network/__init__.py	1970-01-01 00:00:00 +0000
+++ entertainerlib/network/__init__.py	2009-04-04 05:30:04 +0000
@@ -0,0 +1,38 @@
+'''Network functionality for Entertainer.'''
+import sys
+
+from twisted.internet import reactor
+from twisted.internet.protocol import ClientCreator
+from twisted.python.log import startLogging
+
+from entertainerlib.network.local.server import EntertainerLocalServer
+from entertainerlib.network.local.client import EntertainerLocalClientProtocol
+
+# Entertainer really needs a good absctractable way to handle command line
+# arguments.
+def server_main(*args, **kwargs):
+    '''Entertainer Server main function.'''
+    # When multiple server types are supported, the following variable will be
+    # more dynamic.
+    startLogging(sys.stdout)
+    server_type = EntertainerLocalServer
+
+    server = server_type()
+    # TODO: This port number could probably be a config var.
+    reactor.listenTCP(5545, server)
+    reactor.run()
+
+
+def client_main():
+    '''Entertainer test client code.
+
+    This code will go away when what is now the frontend is made into a true
+    client.
+    '''
+    startLogging(sys.stdout)
+    client = EntertainerLocalClientProtocol
+
+    ClientCreator(reactor, client).connectTCP(
+        'localhost', 5545)
+    reactor.run()
+

=== added directory 'entertainerlib/network/local'
=== added file 'entertainerlib/network/local/__init__.py'
--- entertainerlib/network/local/__init__.py	1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/__init__.py	2009-04-04 05:30:04 +0000
@@ -0,0 +1,1 @@
+'''Local network storage and server package.'''

=== added file 'entertainerlib/network/local/client.py'
--- entertainerlib/network/local/client.py	1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/client.py	2009-04-04 05:30:04 +0000
@@ -0,0 +1,35 @@
+'''Classes to produce a local storage implementation.'''
+# pylint: disable-msg=W0223
+
+from twisted.internet.defer import inlineCallbacks
+from twisted.protocols import amp
+
+from entertainerlib.network.local.commands import TenMusicTracks
+from entertainerlib.network.storage import Storage
+
+
+class EntertainerLocalClientProtocol(amp.AMP):
+    '''The client protocol to communicate with the local server.'''
+
+    def connectionMade(self):
+        '''See `twisted.protocols.Protocal.connectionMade`.'''
+        self.get_ten_tracks(1)
+
+    @inlineCallbacks
+    def get_ten_tracks(self, index):
+        '''Get ten tracks starting with the given index.'''
+        result = yield self.callRemote(TenMusicTracks, index=index)
+        print result
+
+
+class LocalStorage(Storage):
+    '''A local storage implementation.'''
+
+    def __init__(self):
+        Storage.__init__(self)
+        self.protocol = EntertainerLocalClientProtocol
+        self.host = 'localhost'
+        # TODO: This port number could probably be a config var.
+        self.port = 5545
+
+

=== added file 'entertainerlib/network/local/commands.py'
--- entertainerlib/network/local/commands.py	1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/commands.py	2009-04-04 05:30:04 +0000
@@ -0,0 +1,14 @@
+'''Local network protocol commands.'''
+from twisted.protocols import amp
+
+class TenMusicTracks(amp.Command):
+    arguments = [('index', amp.Integer())]
+    response = [
+        ('tracks', amp.AmpList([
+            ('id', amp.Integer()),
+            ('filename', amp.Unicode()),
+            ('title', amp.Unicode()),
+            ('artist', amp.Unicode())]
+        ))]
+
+

=== added file 'entertainerlib/network/local/server.py'
--- entertainerlib/network/local/server.py	1970-01-01 00:00:00 +0000
+++ entertainerlib/network/local/server.py	2009-04-04 05:30:04 +0000
@@ -0,0 +1,40 @@
+'''A LocalServer for Entertainer.'''
+
+from twisted.internet.protocol import ServerFactory
+from twisted.protocols import amp
+
+from entertainerlib.network.local import commands
+
+
+class EntertainerLocalProtocol(amp.AMP):
+    '''A local message passing protocol for Entertainer.
+
+    This protocol should be implemented when the client and the server are on
+    the same machine.
+    '''
+
+    def connectionLost(self, reason):
+        '''See `twisted.protocols.Protocol.connectionLost`.'''
+
+    def connectionMade(self):
+        '''See `twisted.protocols.Protocal.connectionMade`.'''
+
+    @commands.TenMusicTracks.responder
+    def get_ten_music_tracks(self, index):
+        '''Get the next ten music tracks from an index.'''
+        return {'tracks': [
+            {'id': 1,
+            'filename': u'/foo/bar/baz',
+            'title': u'Running with scissors',
+            'artist': u'Teh band'},
+            {'id': 1,
+            'filename': u'/foo/bar/bas',
+            'title': u'Fa la la',
+            'artist': u'Teh band'},
+        ]}
+
+
+class EntertainerLocalServer(ServerFactory):
+    '''A local server implementation for Entertainer.'''
+    protocol = EntertainerLocalProtocol
+

=== renamed file 'entertainerlib/storage/base.py' => 'entertainerlib/network/storage.py'
--- entertainerlib/storage/base.py	2009-02-06 06:34:20 +0000
+++ entertainerlib/network/storage.py	2009-04-04 05:30:04 +0000
@@ -1,4 +1,4 @@
-'''Base classes for Entertainer Storages.'''
+'''Base class for a Storage implementation.'''
 
 
 class Storage(object):

=== removed directory 'entertainerlib/storage'
=== removed file 'entertainerlib/storage/__init__.py'
--- entertainerlib/storage/__init__.py	2009-02-06 06:52:14 +0000
+++ entertainerlib/storage/__init__.py	1970-01-01 00:00:00 +0000
@@ -1,4 +0,0 @@
-'''Storage facilities for Entertainer (used by the client/frontend).'''
-# pylint: disable-msg=W0611
-from entertainerlib.storage.base import Storage
-

=== modified file 'entertainerlib/tests/test_storage.py'
--- entertainerlib/tests/test_storage.py	2009-02-10 04:54:36 +0000
+++ entertainerlib/tests/test_storage.py	2009-04-04 05:30:04 +0000
@@ -1,6 +1,6 @@
 '''Test the Storage classes.'''
 # pylint: disable-msg=W0704,W0612
-from entertainerlib.storage import Storage
+from entertainerlib.network.storage import Storage
 from entertainerlib.tests import EntertainerTest
 
 class TestStorage(EntertainerTest):

=== modified file 'pylintrc'
--- pylintrc	2009-02-10 02:34:38 +0000
+++ pylintrc	2009-04-04 05:30:04 +0000
@@ -61,7 +61,7 @@
 # W0201 checks for attributes defined outside init
 
 # XXX: rockstar - C0103 constrains method and function names to a regex
-disable-msg=I0011,R0201,R0801,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914,R0915,R0923,W0613,C0103,W0232,W0201,E1101,E1103,W0142
+disable-msg=R0922,I0011,R0201,R0801,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914,R0915,R0923,W0613,C0103,W0232,W0201,E1101,E1103,W0142
 
 
 [REPORTS]