Code review comment for lp:~gary-lasker/software-center/recommended-installed-feedback

Revision history for this message
Michael Vogt (mvo) wrote :

Hi Gary, thanks for this updated branch.

I just reviewed it and it looks very nice now. Some comments below:

I like that the recommendations widgets do no longer get a catview, giving them the db and propertieshelper
looks much nicer now.

395 class FlowableGrid(Gtk.Fixed):
396
397 MIN_HEIGHT = 100
398
399 + __gsignals__ = {
400 + "application-activated": (GObject.SignalFlags.RUN_LAST,
401 + None,
402 + (GObject.TYPE_PYOBJECT, ),
403 + ),
404 + }
405 +

It seems that the FlowableGrid is pretty generic (i.e. can hold other items than application tiles too) so this signal probably is better put into the TileGrid class? Or am I missing something here? I can do this during the merge if you agree with moving it.

I think its great that you added the docstring:
416 + def add_tiles(self, properties_helper, docs, amount):
417 + '''Adds application tiles to an ApplicationTileGrid:
418 + properties_help = an instance of the PropertiesHelper object
419 + docs = xapian documents (apps)
420 + amount = number of tiles to add from start of doc range'''
here! It might be worth taking a quick look http://www.python.org/dev/peps/pep-0257/ but there seems to be no real convention that everyone in the python community is following, so to a good extend its a matter of taste :) I personally like using a "--" as the keyword seperator there, e.g. "doc -- xapian docuemnts" better than using the "=" as initially my brain read it as code until I noticed the surrounding '''. But thats just a tiny detail and its good to have the docstring.

I like the way the "more" button is now handled, much nicer than in the previous code.

review: Approve

« Back to merge proposal