Code review comment for lp://staging/~3v1n0/unity/indicators-p

> 202 + // We have to do this because on certain systems X won't have time
> to
> 203 + // respond to our request for XUngrabPointer and this will cause the
> 204 + // menu not to show
> 205 + auto data = new ShowEntryData();
>
> Can we use a smart pointer for this please? Once you use a smart pointer you
> won't need this:
>
> 279 + delete data;

Mh, I didn't used these, not to change too much the pre-existent code.

> ...and there's a larger design issue around how we create Indicator instances.
> This code:
>
> 671 - Indicator::Ptr indicator(new Indicator(name));
> 672 + Indicator* indptr;
> 673 +
> 674 + if (name == "libappmenu.so")
> 675 + indptr = new AppmenuIndicator(name);
> 676 + else
> 677 + indptr = new Indicator(name);
> 678 +
> 679 + Indicator::Ptr indicator(indptr);
>
> Makes me uneasy - especially this bit:
>
> 685 +
> 686 + if (indicator->IsAppmenu())
> 687 + {
> 688 + AppmenuIndicator *appmenu =
> dynamic_cast<AppmenuIndicator*>(indicator.get());
> 689 +
> 690 + if (appmenu)
> 691 + appmenu->on_show_appmenu.connect(sigc::mem_fun(owner_,
> &Indicators::OnShowAppMenu));
> 692 + }
> 693 +
>
> Why are we using a boolean member variable to encode class type? Maybe I'm
> missing something, but why not just do this:
>
> AppmenuIndicator *appmenu = dynamic_cast<AppmenuIndicator*>(indicator.get());

Using that bool check is faster than doing an unneeded dymamic_cast (actually we could even use just a static_cast there). And currently we only have two kinds of indicators that we must care of, standard indicators and appmenu indicators. Maybe we could use a GetType or something...

> if (appmenu)
> appmenu->on_show_appmenu.connect(sigc::mem_fun(owner_,
> &Indicators::OnShowAppMenu));
>
> ..or even better, hook up the signal for all indicators, and have child
> classes that don't need the signal simply not use it.

Mh, I don't like too much the idea of adding a signal that is not related to all the indicators to the Indicator class. Also because that class will be filled with some other stuff soon, and I don't want to repeat them in the base class...

> This is wrong:
>
> 818 + return proxy_->id().c_str();
>
> The method returns a std::string, so you don't need the ".c_str" on the end.

True. The mistake was already there, but I didn't notice it. Thanks.

> These:
>
> 1127 +#define COMPIZ_OPTIONS_PATH
> "/apps/compiz-1/plugins/unityshell/screen0/options"
> 1128 +#define MENU_TOGGLE_KEYBINDING_PATH
> COMPIZ_OPTIONS_PATH"/panel_first_menu"
>
> Don't need to be #defines - nor do the three #defines right above them.

Mh... In a C scope they don't look so bad... Also I should repeat some strings without them.

> In panel-service.c, I'm 99% sure there's a memory leak on line 829, where we
> do this:
>
> gchar *entry_id = get_indicator_entry_id_by_entry (entry);
> g_hash_table_insert (self->priv->id2entry_hash, entry_id, entry);
>
> entry_id needs to be freed.

Not really: http://developer.gnome.org/glib/2.29/glib-Hash-Tables.html#g-hash-table-insert :)