Merge lp:~pitti/gtimelog/pygi into lp:~gtimelog-dev/gtimelog/trunk

Proposed by Martin Pitt
Status: Merged
Merge reported by: Marius Gedminas
Merged at revision: not available
Proposed branch: lp:~pitti/gtimelog/pygi
Merge into: lp:~gtimelog-dev/gtimelog/trunk
Diff against target: 203 lines (+72/-24)
3 files modified
HACKING.txt (+2/-1)
NEWS.txt (+10/-0)
src/gtimelog/main.py (+60/-23)
To merge this branch: bzr merge lp:~pitti/gtimelog/pygi
Reviewer Review Type Date Requested Status
Martin Pitt (community) Needs Resubmitting
Marius Gedminas Needs Fixing
Review via email: mp+47696@code.launchpad.net

Description of the change

This ports gtimelog from pygtk to gobject-introspection, so that this will also work with GTK3. (See http://live.gnome.org/GObjectIntrospection for details).

This mostly works fine, but the only thing that I can't get to work for the life of it is the detection whether to use the dark or bright icon. I played with get_style_context().lookup_color('base_color') and other methods, but I always get a bright background with a dark foreground, no matter which theme I set. Curiously the color values do change on theme changes, just not in the way that I need.

Do you know whether there is a cleaner way to ship icons for dark and bright themes?

To post a comment you must log in.
Revision history for this message
Marius Gedminas (mgedmin) wrote :

Awesome!

But something seems to be wrong: the diff is empty. Have you pushed your changes to the launchpad branch?

Is it possible to have one codebase that uses either PyGtk or PyGI? I'd like GTimeLog to continue working with older distributions like Lucid.

The icon color detection is an ugly hack. I think the proper solution is to install different versions of gtimelog.png into /usr/share/icons/ubutu-mono-{dark,light}, and perhaps a colourful one into /usr/share/icons/hicolor.

The hack is there mostly because development is easier if I can run gtimelog (and see its icon) directly from a source checkout.

Revision history for this message
Barry Warsaw (barry) wrote :

BTW, note that the dark background icon has *never* worked for me. For example, I'll attach a screenshot from Natty with stock gtimelog.

Revision history for this message
Barry Warsaw (barry) wrote :

Er, okay, I forgot you can't add attachments to merge proposals. :(

http://barry.warsaw.us/gtimelog.png

Revision history for this message
Martin Pitt (pitti) wrote :

> But something seems to be wrong: the diff is empty.

Argh, sorry. Pushed.

> Is it possible to have one codebase that uses either PyGtk or PyGI?

That's a bit tricky in general. I can change the code to work with either, but that will inevitably result in a few more "if using pygi; then ... else pygtk way" constructs.

Revision history for this message
Martin Pitt (pitti) wrote :

I updated the branch now to work with PyGTK, GTK2-GI, and GTK3-GI. For using GTK3, comment out "gtk.require_version('2.0')" for now.

BTW, the pygtk "get theme color" hack doesn't seem to work under unity either.

So while the code looks a bit ugly now in some places due to the four "if pygtk:" checks, it's by and large still ok, I think. The pygtk bits can be dropped in a year or so when GTK3 and GI become the ubiquitous standard.

lp:~pitti/gtimelog/pygi updated
177. By Marius Gedminas

Bugfix: always preserve the order of entries, even when they have the same
timestamp (LP: #708825).

178. By Marius Gedminas

Application Indicator menu now has a CheckMenuItem "Show GTimelog".

Previously it used the old tray icon menu with separate Show/Hide items.

179. By Marius Gedminas

Prepare to release 0.5.0

180. By Marius Gedminas

Post-release version bump

Revision history for this message
Marius Gedminas (mgedmin) wrote :

Looks good to me.

At the moment I'm having intermittent connection problems (connection refused on port 22) that prevent me from merging your branch, so I'll do that a bit later. I barely managed to push out a bugfix (unrelated to GUI code) and a small change to the app indicator menu earlier today. Speaking of which, I hope working GtkCheckMenuItems hasn't changed in any significant way between PyGtk and PyGI?

Also, we're hosting a 48-hour Game Jam in our office, so I might be a bit busy.

I think I'd also like try a few more refactorings, e.g. those constructor calls can also be handled the same way as constants:

    try:
        # PyGI
        ...
        new_pango_tab_array = pango.TabArray.new
        ...
    except ImportError:
        # PyGTK
        ...
        new_pango_tab_array = pango.TabArray
        ...
    ...
            tabs = new_pango_tab_array(2, False)
            tabs.set_tab(0, PANGO_TAB_LEFT, 9 * em)
            tabs.set_tab(1, PANGO_TAB_LEFT, 12 * em)
    ...

and constants such as PANGO_TAB_LEFT would look a bit more conventional in upper case. But those are minor details.

review: Approve
Revision history for this message
Martin Pitt (pitti) wrote :

Good idea, I made those two changes.

Revision history for this message
Marius Gedminas (mgedmin) wrote :

Unfortunately the code doesn't work on Maverick:

Traceback (most recent call last):
  File "./gtimelog", line 11, in <module>
    from gtimelog.main import main
  File "./src/gtimelog/main.py", line 24, in <module>
    gtk.require_version('2.0')
  File "/usr/lib/pymodules/python2.6/gtk-2.0/gi/module.py", line 212, in __getattr__
    attribute = getattr(self._dynamic_module, name)
  File "/usr/lib/pymodules/python2.6/gtk-2.0/gi/module.py", line 88, in __getattr__
    self.__class__.__name__, name))
AttributeError: 'DynamicModule' object has no attribute 'require_version'

Some kind of feature/version check may be necessary. Or ir may be enough to swap the try/except parts and try PyGTK by default, falling back to PyGI when PyGTK is not available.

Also, could you add "Contributed by Martin Pitt <email address hidden>" to the changelog entry?

review: Needs Fixing
Revision history for this message
Barry Warsaw (barry) wrote :

Minor comments about the code:

the 'pygtk = True' in the ImportError clause looks odd because you're clobbering the name binding given by the 'import pygtk'. It would probably read easier if you changed that to 'has_pygtk = True'.

OTOH, the way it's often done in other places is to set 'pygtk = None' in the try: clause. You're 'if pygtk' tests will still work that way because the import statement will bind pygtk to the module object, which is not false.

Can you tighten up the bare except on line 130? If not, *please* add a comment explaining why the bare except is necessary.

Other than that, it's encouraging that it wasn't really that much code to add support for the three different regimes. Very nice!

Revision history for this message
Martin Pitt (pitti) wrote :

Ah, if we get an AttributeError after the "Gtk" repository is already imported, we can't import pygtk afterwards, so due to that we actually have to test for pygtk first. That will cause pygtk to be preferred, and the GI code to get much less exposure, but we can still locally patch that in Ubuntu, so it's not a biggie.

I added the bare "except:" on the indicator because ImportError is not the only probable exception here. gi.RepositoryError also can happen if the installed AppIndicator gir wasn't built against the version of GTK that gtimelog uses. There might be other causes I haven't thought about as well, but I guess those two are the main ones, so I tightened up the except statement.

I addressed the other things as well.

review: Needs Resubmitting
Revision history for this message
Marius Gedminas (mgedmin) wrote :

Thanks, merged!

lp:~pitti/gtimelog/pygi updated
181. By Marius Gedminas

Support PyGI/PyGObject when PyGtk is not available.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'HACKING.txt'
2--- HACKING.txt 2011-01-08 14:38:47 +0000
3+++ HACKING.txt 2011-01-29 10:43:14 +0000
4@@ -26,7 +26,8 @@
5 Here's how you can create and send me a a patch. You'll need the `Bazaar
6 <http://bazaar.canonical.com/en/>`_ version control system installed on your
7 computer, as well as the usual stuff needed to run GTimeLog (`Python
8-<http://python.org/>`_, `PyGtk <http://pygtk.org/>`_).
9+<http://python.org/>`_, and `PyGtk <http://pygtk.org/>`_ or the GTK and Pango
10+gobject-introspection typelibs).
11
12 First, get a copy of the source code::
13
14
15=== modified file 'NEWS.txt'
16--- NEWS.txt 2011-01-28 13:58:01 +0000
17+++ NEWS.txt 2011-01-29 10:43:14 +0000
18@@ -12,6 +12,16 @@
19 src/gtimelog/gtimelog.ui. It needs to be installed into
20 /usr/share/gtimelog/.
21
22+* Ported from PyGTK to GI. This supports GTK 2 and GTK 3 with GI now, but still
23+ works with PyGTK.
24+
25+ Packager's note: If you want to use GI, you need to change the package's
26+ dependencies from pygtk to the package that provides the GTK and Pango
27+ typelibs (e. g. gir1.2-gtk-2.0 and gir1.2-pango-1.0 on Debian/Ubuntu). It
28+ also requires pygobject >= 2.27.1.
29+
30+ Contributed by Martin Pitt <martin.pitt@ubuntu.com>.
31+
32 * GTimeLog now supports Ubuntu's application indicators. There's a new
33 configuration option, ``prefer_app_indicator``, defaulting to true.
34 Fixes LP: #523461.
35
36=== modified file 'src/gtimelog/main.py'
37--- src/gtimelog/main.py 2011-01-28 13:53:34 +0000
38+++ src/gtimelog/main.py 2011-01-29 10:43:14 +0000
39@@ -15,11 +15,34 @@
40 import ConfigParser
41 from operator import itemgetter
42
43-import pygtk
44-pygtk.require('2.0')
45 import gobject
46-import gtk
47-import pango
48+
49+# we have to try pygtk first, then fall back to GI; if we have a too old GI
50+# (without require_version()), we can't import pygtk on top of gi.repo.Gtk.
51+try:
52+ import pygtk
53+ pygtk.require('2.0')
54+ import gtk
55+ from gtk import gdk as gdk
56+ import pango
57+
58+ PANGO_ALIGN_LEFT = pango.TAB_LEFT
59+ GTK_RESPONSE_OK = gtk.RESPONSE_OK
60+ gtk_status_icon_new = gtk.status_icon_new_from_file
61+ pango_tabarray_new = pango.TabArray
62+except ImportError:
63+ import gi
64+ from gi.repository import Gdk as gdk
65+ from gi.repository import Gtk as gtk
66+ gtk.require_version('2.0')
67+ from gi.repository import Pango as pango
68+ pygtk = None
69+
70+ # these are hacks until we fully switch to GI
71+ PANGO_ALIGN_LEFT = pango.TabAlign.LEFT
72+ GTK_RESPONSE_OK = gtk.ResponseType.OK
73+ gtk_status_icon_new = gtk.StatusIcon.new_from_file
74+ pango_tabarray_new = pango.TabArray.new
75
76 try:
77 import dbus
78@@ -995,6 +1018,10 @@
79 # XXX assumes the panel's color matches a menu bar's color, which is
80 # not necessarily the case! this logic works for, say,
81 # Ambiance/Radiance, but it gets New Wave and Dark Room wrong.
82+ if not pygtk:
83+ # XXX: need to figure out how to do that with gi
84+ return icon_file_bright
85+
86 menu_text_color = gtk.MenuBar().rc_get_style().text[gtk.STATE_NORMAL]
87 if menu_text_color.value >= 0.5:
88 return icon_file_bright
89@@ -1012,7 +1039,7 @@
90 if not hasattr(gtk, 'StatusIcon'):
91 # you must be using a very old PyGtk
92 return
93- self.icon = gtk.status_icon_new_from_file(self.icon_name)
94+ self.icon = gtk_status_icon_new(self.icon_name)
95 self.last_tick = False
96 self.tick()
97 self.icon.connect("activate", self.on_activate)
98@@ -1049,7 +1076,7 @@
99
100 def tick(self):
101 """Tick every second."""
102- self.icon.set_tooltip(self.tip())
103+ self.icon.set_tooltip_text(self.tip())
104 return True
105
106 def tip(self):
107@@ -1075,14 +1102,24 @@
108 self.gtimelog_window = gtimelog_window
109 self.timelog = gtimelog_window.timelog
110 self.indicator = None
111- try:
112- import appindicator
113- except ImportError:
114- return # nothing to do here, move along
115+ if pygtk:
116+ try:
117+ import appindicator
118+ self.indicator = appindicator.Indicator("gtimelog", self.icon_name,
119+ appindicator.CATEGORY_APPLICATION_STATUS)
120+ self.indicator.set_status(appindicator.STATUS_ACTIVE)
121+ except ImportError:
122+ return # nothing to do here, move along
123 # or install python-appindicator
124- self.indicator = appindicator.Indicator("gtimelog", self.icon_name,
125- appindicator.CATEGORY_APPLICATION_STATUS)
126- self.indicator.set_status(appindicator.STATUS_ACTIVE)
127+ else:
128+ try:
129+ from gi.repository import AppIndicator
130+ self.indicator = AppIndicator.Indicator.new("gtimelog", self.icon_name,
131+ AppIndicator.IndicatorCategory.APPLICATION_STATUS)
132+ self.indicator.set_status(AppIndicator.IndicatorStatus.ACTIVE)
133+ except (ImportError, gi._gi.RepositoryError):
134+ return
135+
136 self.indicator.set_menu(gtimelog_window.app_indicator_menu)
137 self.gtimelog_window.tray_icon = self
138 self.gtimelog_window.main_window.connect("style-set", self.on_style_set)
139@@ -1304,9 +1341,9 @@
140 """Set up tab stops in the log view."""
141 pango_context = self.log_view.get_pango_context()
142 em = pango_context.get_font_description().get_size()
143- tabs = pango.TabArray(2, False)
144- tabs.set_tab(0, pango.TAB_LEFT, 9 * em)
145- tabs.set_tab(1, pango.TAB_LEFT, 12 * em)
146+ tabs = pango_tabarray_new(2, False)
147+ tabs.set_tab(0, PANGO_ALIGN_LEFT, 9 * em)
148+ tabs.set_tab(1, PANGO_ALIGN_LEFT, 12 * em)
149 self.log_view.set_tabs(tabs)
150
151 def w(self, text, tag=None):
152@@ -1442,7 +1479,7 @@
153 def scroll_to_end(self):
154 buffer = self.log_view.get_buffer()
155 end_mark = buffer.create_mark('end', buffer.get_end_iter())
156- self.log_view.scroll_to_mark(end_mark, 0)
157+ self.log_view.scroll_to_mark(end_mark, 0, False, 0, 0)
158 buffer.delete_mark(end_mark)
159
160 def set_up_task_list(self):
161@@ -1593,7 +1630,7 @@
162
163 Returns either a datetime.date, or one.
164 """
165- if self.calendar_dialog.run() == gtk.RESPONSE_OK:
166+ if self.calendar_dialog.run() == GTK_RESPONSE_OK:
167 y, m1, d = self.calendar.get_date()
168 day = datetime.date(y, m1+1, d)
169 else:
170@@ -1603,7 +1640,7 @@
171
172 def on_calendar_day_selected_double_click(self, widget):
173 """Double-click on a calendar day: close the dialog."""
174- self.calendar_dialog.response(gtk.RESPONSE_OK)
175+ self.calendar_dialog.response(GTK_RESPONSE_OK)
176
177 def weekly_window(self, day=None):
178 if not day:
179@@ -1808,20 +1845,20 @@
180
181 def task_entry_key_press(self, widget, event):
182 """Handle key presses in task entry."""
183- if event.keyval == gtk.gdk.keyval_from_name('Prior'):
184+ if event.keyval == gdk.keyval_from_name('Prior'):
185 self._do_history(1)
186 return True
187- if event.keyval == gtk.gdk.keyval_from_name('Next'):
188+ if event.keyval == gdk.keyval_from_name('Next'):
189 self._do_history(-1)
190 return True
191 # XXX This interferes with the completion box. How do I determine
192 # whether the completion box is visible or not?
193 if self.have_completion:
194 return False
195- if event.keyval == gtk.gdk.keyval_from_name('Up'):
196+ if event.keyval == gdk.keyval_from_name('Up'):
197 self._do_history(1)
198 return True
199- if event.keyval == gtk.gdk.keyval_from_name('Down'):
200+ if event.keyval == gdk.keyval_from_name('Down'):
201 self._do_history(-1)
202 return True
203 return False

Subscribers

People subscribed via source and target branches