Re: Patch to show more results

[Joe Shaw <joeshaw novell com>]

Hi Ryan,

On Fri, 2006-11-10 at 15:21 -0700, Ryan Probasco wrote:
> I worked with Joe a bit on an alternative solution for this same 
> issue a few weeks ago, but it seemed like the best solution needed 
> to be a bit more comprehensive in the end.  So, here's a little 
> patch that makes the GUI display as many results per category as the 
> window allows (rather than just a couple).

I really like the effects of this patch, nicely done.

> It seems to work pretty well for me, but there are still a few 
> problems with the strategy it uses.  For instance it doesn't have a 
> way to determine which categories should receive more rows; it tries 
> to spread the additional rows evenly starting with the categories 
> that have the fewest number of rows.  

I think this is a pretty good approach, actually, because it means that
one category doesn't dominate over the others.

The only problem I see with it is that as you resize the window
vertically, a row can disappear from one category as a row in another
category is created.  I think we need to fix that before the patch goes
in.

> On the technical side, I did have some trouble finding a reliable way
> to determine widget dimensions at run time (I'm fairly new to GTK) so 
> I hope I haven't made any mistakes there (the final solution seems to 
> work reliably for me).

There are two steps to widget sizes: requisition and allocation.  The
requisition is the size that the widget requests to be, and the
allocation is the size that the container actually gives it.  The
requisition is a completely unreliable way to determine the size of a
widget; you almost always want to look at the allocation.  Looking at
your patch, it looks as though ShowMoreRows() looks at the size request;
I'm not sure if that's the right thing to do there.
 
Comments on the patch:

* In general our coding style is to use under_scores rather than
camelCase.  I know that most of the existing code in the UI is
camelCase.  Anyway, not a big deal to change, but it's something to keep
in mind for future patches.

* Why have a separate busy private member?  Isn't it enough to check
whether or not timeoutId != 0?

* Shouldn't CategoryExtraRowComparer be a private class?

* MainSW doesn't really tell us what we want and it violates the coding
conventions.  Can we just change it to be ScrolledWindow?  Or maybe
ContentScrolledWindow?

* In RowHeight, is it safe to check just the height of tiles[0]?
Because you're getting size requests and not size allocations, I think
those could vary pretty wildly if the tiles want to be different sizes.
(Because our group has fixed-height rows, the allocated height of
tiles[0] would be sufficient, though.)

* There are a couple of places where you duplicate the code you added to
the Category.Reset() method, like the Columns and Rows property set
blocks.  It would be good to replace those with just calls to Reset().

Thanks,
Joe