On Thu, 2012-01-26 at 12:37 +0000, Lars Uebernickel wrote:
> hud-search.h:
> 
> * no need to include both glib.h and glib-object.h

Old template :-)  Fixed r167

> hud-search.c:
> 
> * G_DEFINE_TYPE already declares _init and _class_init functions, no need to
>   do it again in line 57-58

Ditto.  Fixed r168

> * 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.

>   - active_app is not initialized at all when active_window is NULL
>     (are private structs initalized to zero when allocating the instance?)

They are, but being explicit for the reasons mentioned above.  r169

> * hud_search_dispose:
>   - using g_clear_object would save some lines

Cool!  I didn't know about that function.  It sure will!  r170

> * hud_search_finalize:
>   - can be removed, it only chains up to parent class

Yeah, I typically leave it there for future use.  Low runtime cost, huge
developer savings when you need to put something there :-)

> * hud_search_new:
>   - no need to cast the return value of g_object_new

Fair.  r171

> * found_list_to_usage_array:
>   - The loop stops once it finds an item in found_list that has a distance
>     greater than max-distance - is found_list sorted? It doesn't look that
>     way from a quick glance at dbusmenu_collector_search.  break -> continue?

Yes!  The list used to be sorted, but no longer is.  r172

>   - move get_settings_uint out of the loop

Done. r173

> * 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

>   - renaming to desktop_to_icon would make it fit better

Done.  r175

> * search_current_app:
>   - no need to call dbusmenu_collector_search when address or path are NULL
>     (i.e. no current app)

Fixed for the desktop file as well.  r176

> * search_indicators:
>   - move get_settings_uint out of the loop

Fixed.  r177

>   - 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.

> * 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.

>   - 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.

> * 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.

>   - g_list_append is O(n), use g_list_prepend and reverse the list afterwards
>     (shouldn't be much of an issue with only 5 items, though)

Fixed. r178

> * hud_search_execute:
>   - input isn't checked (whether 'key' really contains a variant and if 'app',
>     'display', 'address', 'path', 'id' are not NULL)

Fixed. r179

> * active_window_changed:
>   - dfeet, terminal, and hud prototype windows are ignored (seems like old
>     debugging code), which they probably shouldn't in the final version

Yup.  There's a bug tracking that.

> * hud_search_suggest_new:
>   - is 'key' the same key that is extracted in hud_search_execute?
>     If so, it's using 'db' and not 'display' in the 2nd tuple position

Actually it's all correct, just named incorrectly.  Fixed the names.
r180

> * 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 ;-)

Thanks for the detailed review!