Charles inspired me to ask a few follow-up questions of my own! * larsu points out that only is required, not an additional include for ; I looked at this, and can't quite grok how this chaining works as doesn't appear to pull in any of the GObject headers? * Re: the NULLing out priv struct seems to be extra work. Ted, I understand your reasoning, but after looking at the documentation, it appears that the GObject initialization sequence handles NULLing/ZEROing out memory ( ultimately by g_type_create_instance() ). That said, the GObject Reference Manual section on Object Instantiation states: "Although one can expect all class instance and members ( except the fields pointing to the parents ) to be set to zero, some consider it good practice to explicitly set them." I would suggest a simple comment explaining your practice as a few of use have noticed this pattern and commented on it being odd. [Charles, I like the memset idea, however it really isn't needed due to the way GObject works.] * Regarding line length... the vt130 comment was funny, but not a real justification. I would suggest checking out a few public C/C++ styleguides ( GTK+, Google Style Guides, ... ) for reference, most if not all recommend 80 char lines. > 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!