Code review comment for lp:~nbdarvin/awn-extras/pidgin-adium

== Disclaimer ==
Written around 5 am using a crappy internet connection.

== 1 ==
In setup_dialog_settings() you seem to use an old way to initialize the theme setting. I only took a quick look at the code, but if you have a .ui (GtkBuilder) file, then you can use:

binder = self.applet.settings.get_binder(prefs)
binder.bind("theme", "combobox-theme", key_callback=self.combobox_theme_changed_cb)
self.applet.settings.load_bindings(binder)

prefs is a gtk.Builder object here. You can then use self.applet.settings["theme"] or use the GProps object returned by load_bindings(); if you choose this, use it something like this:

self.settings = self.applet.settings.load_bindings(binder)

And then you should be able to use it like this: print self.settings.theme or self.settings.theme = "Tango" to read or write the setting.

In this case you need to create the combobox in the .ui file (not manually) with a model (GtkListStore) that needs a gchararray field/column. Then after having set-up the bindings, you need to fill the combobox with some text elements:

combobox_theme = prefs.get_object("combobox-theme")
awnlib.add_cell_renderer_text(combobox_theme)
for i in self.themes:
    combobox_theme.append_text(i)

self.themes is a list of strings here. Note that a call to add a cell renderer is needed, because the Glade tool doesn't seem to provide a way to set it in the .ui file.

Look in load_theme_pref() in volume-control.py how you could do this.

== 2 ==
I also noticed that you seem to set pixbufs as icons via self.applet.icon.set(). I think it's better to use Awn's themed icon API (moonbeam will agree :p) if possible. To do this, when initializing the icons (see setup_icons() in volume-control.py), first set the states:

self.applet.theme.set_states(states)

where states is a dict mapping the state names to the icon names. Second, you set the theme:

self.applet.theme.theme(theme)

theme must be None here if you want to use the user's current system theme. (In volume-control.py in setup_icons() I add "System theme" to the combobox, and then make theme None if the theme name matches "System theme")

Later, when you want to change the icon, set the icon to one of the states that you gave set_states():

self.applet.theme.icon(icon_state)

I call the setup_icons() method every time the combobox (theme) value changes. After that, you can change the icon (by changing the current state) whenever you want.

Btw, you need to change the folder adium-dock-themes/ to themes/ then. Also make sure Green/ and Purple/ both have each an index.theme file. And probably the images need to be in some directory with a famous/known name to get the bloody gtk icon code (used by the Awn themed icon API) to find the images. (I use themes/<theme name>/scalable/

If you use this themed icon API shizzle, then you can remove the call self.applet.connect_size_changed(self.size_changed_cb)

== 3 ==
Oh, and use if x is None (or if x) instead of if x == None
and if x is not None (or if not x) instead of if x != None

== 4 ==
Last thing: you seem to do

self.popup_menu = self.applet.create_default_menu()
self.popup_menu.show_all()

in setup_context_menu(). Don't do this, you can just do:

menu = self.applet.dialog.menu
menu_index = len(menu) - 1

And then do menu.insert(<your item>, menu_index + <some index>)

Then add a gtk.SeparatorMenuItme() and then call self.applet.dialog.new("preferences") so that you get some items, a separator, and then Preferences and About (last 2 are added automatically)

== 5 ==
Change name of procedure RefreshIcon to refresh_icon. Check code for PEP8 compliance.

http://wiki.awn-project.org/Applets:DevelopmentGuidelines#Python_PEP_8