-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/26/2012 04:34 PM, Ted Gould wrote:
> [...]
> 
>> * hud_search_init: - priv members tracker, matcher,
>> window_changed_sig, active_window, collector, usage, and appmenu
>> are initialized to NULL only to be overwritten uncoditionally a
>> few lines down
> 
> I actually do that on purpose.  And the reason is that I figure
> that the code in init() will change, and perhaps I'd screw up and
> not define a variable or put a conditional in that doesn't work.
> So the idea is to isolate the damage caused by those mistakes.  If
> the mistakes aren't made, the compiler will optimize the
> inefficiencies out anyway.

Fair enough.


> [...]
> 
>> * desktop2icon: - why a g_file_test?
>> g_desktop_app_info_new_from_filename probably does the exact same
>> test
> 
> So that we can have better error reporting... but, we're not doing
> that. So I added that.  r174

Agreed, better error reporting is always ... better.


> [...]
>> - rename indicator-penalty to indicator-weight?  It's not really
>> a penalty
> 
> Hmm, I actually do consider it a penalty... it makes your distance 
> further if you're an indicator.  It seems like it's penalizing
> those entrys?  I'm not dead set on the name, but it seems an apt
> description to me.

You use it to weigh the relative importance between menu items and
indicators.  Also, it's not a penalty anymore if it's greater than 100.

Not that important to me, though.  It's probably used as a penalty
most of the time anyway.


>> * search_and_sort: - second loop ("Calculate overall") could be
>> merged with the first
> 
> Yes, I did that to make the code easier to read.  I'm sure the
> compiler merges them when it unrolls them.

Fair.


>> - imo, sorting by summing the relative usage and distance isn't a
>> very good strategy, as both usages and distances will tend to
>> only have few very small and many very large values, which is
>> always tricky when dealing with percentages.
>> 
>> Somewhat exaggerated example: Usage count for "open" and "save"
>> is 10 each. I type "blur" (which is a perfect match) for the
>> first time:
>> 
>> distance        usage        total (as in usage_sort()) blur    0
>> 0 %         0   0 %      100 % open    4  50 %        10  50 %
>> 100 % save    4  50 %        10  50 %      100 %
>> 
>> Ranking those 3 the same is surely not what we want?
> 
> Yes.  I think there needs be some amount of multiplicative factor
> here. I consider that in the category of "search tuning" and
> something we should work on in the future.  Open to ideas here, but
> I want to merge this before optimizing.

I agree, this can be done after the merge.


>> * hud_search_suggestions: - found_list is passed to
>> search_and_sort() and then freed.  Should be moved into
>> search_and_sort entirely
> 
> That can't be done because the found list holds references to all
> the memory and the values aren't copied into the usage structures
> (and they're not real objects so they can't be referenced).  So
> when the found list can't be free'd before the search suggestions
> are built. Probably should turn the found structures into real
> objects so they can be ref'd appropriately.  But, that's a bigger
> refactor.

You're right of course.  Sorry, didn't catch that.


> [...]
>> * And most importantly: there are some seriously long lines (>
>> 200 chars) ;)
> 
> Hopefully you'll get rich at Canonical you'll be able to upgrade to
> a VT130 so those aren't an issue ;-)

So looking forward to that!

> Thanks for the detailed review!

Pleasure.  Thanks for the quick fixes!

 review approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPIXxXAAoJEAbD1bcyW6kFkh8H/RgU8V1Tm8xYF2zSaiVgQVzK
exKtAjR/RW94SHor3EwvJcQiyVBppbs+qbQzw3aZSm2taSiFGWlE1+PNvd0eodw3
fCxL/PDXIrB5Y0Xus8qYOC5dRveTpMaNVL6jsEDJHGr9wHS071+ZM1TWcXdeZ65V
p1rNMLzIWmYmY6bJ0qkmD8umzMhoU7hia/dCcWgd2nw59P7Qo/5kun3v4eqS/tMB
uS4er5/3eOyHdIddPr5waQYl2YVf6ueqoGLNL949z+6DaQ/ea4u3Ve970bzC/LM6
Lopj6Q5c8w5/T9r6Rc9GG7lHw8bLfk8hrzJ9RYi4qGhrO4PdYEZ5hhdEoaaC+u8=
=147+
-----END PGP SIGNATURE-----