Merge lp:~rockstar/entertainer/add-distutils into lp:entertainer

[Paul Hummer]

Distutils support has been added, so that users can python setup.py install and get entertainer installed, and we can facilitate easier packaging.

[Matt Layman (mblayman)]

Here are my comments:

1) In the Makefile, TEST_DIR is no longer used, please remove it.

2) gettext.install is only available in the system_tray_icon and run_tests. To me this doesn't make any sense. How is the functionality of translation installation part of the system tray? Doesn't it belong in the frontend main runner?

3) What is the reason for removing the try/except block around some imports? Is the assumption that the package will ensure that we have those imports?

4) Why are we back to use the abs path for the test runner? Didn't we go through the trouble of using __dirname__ for a reason?

5) I think setup.py should be executable. Either that or you should get rid of the #! line.

6) I saw these lines when I ran `python setup.py build`:

package init file 'entertainerlib/frontend/glade/__init__.py' not found (or not a regular file)
package init file 'entertainerlib/utils/glade/__init__.py' not found (or not a regular file)
package init file 'entertainerlib/frontend/glade/__init__.py' not found (or not a regular file)
package init file 'entertainerlib/utils/glade/__init__.py' not found (or not a regular file)

This seems like something incorrect that needs to be refined/fixed.

I'll approve after the couple of corrections (and hopefully some answers to my inquiries). I'll also recommend we fast track this branch after that so that you can get working on the package branch.

review: Disapprove

[Paul Hummer (rockstar)]

> 1) In the Makefile, TEST_DIR is no longer used, please remove it.
>

Sounds good.

> 2) gettext.install is only available in the system_tray_icon and run_tests. To me this doesn't make any sense. How is the functionality of translation installation part of the system tray? Doesn't it belong in the frontend main runner?
>

I think for the system tray notification messages. I don't know, I
didn't really design this (joshua did), and it's really beyond the scope
of this branch if it doesn't need to be there.

> 3) What is the reason for removing the try/except block around some imports? Is the assumption that the package will ensure that we have those imports?
>

You and I chatted about this a bit. Basically, if we're missing
imports, I'd rather it error out quickly, and provide a standard python
traceback that someone can instantly see the need for another library.
It made the code more straightforward.

> 4) Why are we back to use the abs path for the test runner? Didn't we go through the trouble of using __dirname__ for a reason?
>

For some reason, __file__ wasn't being set, and so calling dirname on
__file__ failed. I think it has somethin to do with calling it through
the Makefile.

> 5) I think setup.py should be executable. Either that or you should get rid of the #! line.
>

It's been pretty standard with distutils I've seen to allow it to be set
executable by the user, but not actually BE executable. I was just
going with the convention.

> 6) I saw these lines when I ran `python setup.py build`:
>
> package init file 'entertainerlib/frontend/glade/__init__.py' not found (or not a regular file)
> package init file 'entertainerlib/utils/glade/__init__.py' not found (or not a regular file)
> package init file 'entertainerlib/frontend/glade/__init__.py' not found (or not a regular file)
> package init file 'entertainerlib/utils/glade/__init__.py' not found (or not a regular file)
>

Oh, good catch. I'm going to have to figure out how to get the glade
files figured out. Damnit.

--
Paul Hummer
Launchpad Code Team

[Joshua Scotton (joshuascotton)]

>> 2) gettext.install is only available in the system_tray_icon and run_tests. To me this doesn't make any sense. How is the functionality of translation installation part of the system tray? Doesn't it belong in the frontend main runner?

The System Tray Icon text in the drop down menu needs to be translated, however when I set the main part of the frontend up for translation, the tray icon menu was still not being translated, hence I put the gettext.install in there.

As to run_tests, translation setup for Entertainer is run in the ./entertainer script line 15. However run_tests bypasses this in the trunk atm. Hence the need for a seperate translation setup for run_tests.

It would be great if this all can be merged into one place for translation setup such as the frontend main runner. However that's only if the frontend main runner is run as part of run_tests, which I don't think is the case.

[Joshua Scotton (joshuascotton)]

I'm getting the following when running ./entertainer in this branch

Traceback (most recent call last):
  File "./entertainer", line 15, in <module>
    main()
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/__init__.py", line 9, in main
    from entertainerlib.frontend.frontend_client import FrontendClient
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/frontend_client.py", line 24, in <module>
    from entertainerlib.frontend.gui.user_interface import UserInterface
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/gui/user_interface.py", line 58, in <module>
    from entertainerlib.frontend.gui.screens.music_screen import MusicScreen
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/gui/screens/music_screen.py", line 25, in <module>
    from entertainerlib.frontend.gui.tabs.tab import Tab
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/gui/tabs/tab.py", line 13, in <module>
    class Tab(clutter.Group):
  File "/home/jozzy/Projects/devel/Entertainer/add-distutils/entertainerlib/frontend/gui/tabs/tab.py", line 59, in Tab
    def show_empty_tab_notice(self, title=_("Empty tab"),
NameError: name '_' is not defined

This is because TranslationSetup() must be called before any of the frontend imports are called.

This can be solved by moving
    from entertainerlib.frontend.translation_setup import TranslationSetup
    TranslationSetup()
to just before
    from entertainerlib.frontend.frontend_client import FrontendClient
in frontend/__init__.py main()

[lp:~rockstar/entertainer/add-distutils]

298. By Paul Hummer on 2008-10-13

Worked out lint issues

299. By Paul Hummer on 2008-10-13

Fixed a bug with translations (that oddly wasn't manifesting on trunk)

[Joshua Scotton (joshuascotton)]

=== modified file 'entertainerlib/frontend/__init__.py'
--- entertainerlib/frontend/__init__.py 2008-08-16 04:28:47 +0000
+++ entertainerlib/frontend/__init__.py 2008-10-11 20:59:42 +0000
@@ -1,1 +1,26 @@
 '''Frontend gui to entertainer'''
+
+def main(*args, **kwargs):
+ '''Frontend runner'''
+
+ # Import statements are inside thu function so that they aren't imported
+ # every time something from the frontend is imported
+ from entertainerlib.backend.backend_server import BackendServer
+ from entertainerlib.frontend.frontend_client import FrontendClient
+ from entertainerlib.utils.configuration import Configuration
+ from entertainerlib.utils.system_tray_icon import init_systray
+ from entertainerlib.frontend.translation_setup import TranslationSetup
+ TranslationSetup()

=== modified file 'entertainerlib/frontend/gui/tabs/tab.py'
--- entertainerlib/frontend/gui/tabs/tab.py 2008-10-11 13:49:43 +0000
+++ entertainerlib/frontend/gui/tabs/tab.py 2008-10-13 15:58:02 +0000
@@ -8,8 +8,11 @@

 from entertainerlib.frontend.gui.widgets.label import Label
 from entertainerlib.frontend.gui.widgets.texture import Texture
+from entertainerlib.frontend.translation_setup import TranslationSetup
 from entertainerlib.utils.configuration import Configuration

+TranslationSetup()
+
 class Tab(clutter.Group):
     """
     Tab can be used as part of the TabGroup

If you reorder the imports in entertainerlib/frontend/__init__.py main() so that the TranslationSetup() call is made before any of the frontend imports, then you won't need the changes in tab.py

[Paul Hummer (rockstar)]

> If you reorder the imports in entertainerlib/frontend/__init__.py main() so that the TranslationSetup() call is made before any of the frontend imports, then you won't need the changes in tab.py

Fixed, thanks!

--
Paul Hummer
Launchpad Code Team

[lp:~rockstar/entertainer/add-distutils]

300. By Paul Hummer on 2008-10-13

Fixed import order in the frontend in response to the code review.

[Joshua Scotton (joshuascotton)]

Great

review: Approve

[Matt Layman (mblayman)]

Cool, I'm glad to see this is all merged and figured out. So now what do we do? Are all the recently landed branches going to be merged into 0.2, then a debian branch will be made from that? Paul, I also noticed that you put Entertainer into your PPA. How do we notify users that that is where our package is? Just use an announcement?

Do you think you could draft up an email about how/when/what code will go into a package? Can we figure out a process for consistently pulling code into a branch and then building that branch? I think it will make sense to have stable packages, but it would also be cool to have periodic dev packages for users to test the latest and greatest without running from a source branch. Since this is a major breakthrough for the availability of Entertainer, I'm hoping we can put some serious thought into how we deliver stuff to end users and testers.