Merge lp://staging/~3v1n0/unity/indicators-p into lp://staging/unity

Hi,

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;

...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());

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.

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.

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.

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.

The points above need to be fixed. You should also get someone else to look at this (someone who understands the panel code).

Cheers,

> 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 :)

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

Ah, actually I already fixed in another branch... I put the fix here too.

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

It's a kind of object factory, for me it's ok.

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 +

If you can remove this please do... :) Otherwise it's ok for me.

> 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 +
>
> If you can remove this please do... :) Otherwise it's ok for me.

Yes, done. It was possible to get that before as well, but I didn't like to connect to the appmenu specific signal before than the others :)

183 data->proxy = proxy_;

Makes me sad that it doesn't use the dbus proxy class from unity core.

194 + g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry, data, NULL);

Moreover this is pretty dangerous, it should at least grab a ref on the proxy. Alternatively such timeouts should be removed in the destructor.

1114 +#include <fcntl.h>

I hope this isn't needed?

> 183 data->proxy = proxy_;
>
> Makes me sad that it doesn't use the dbus proxy class from unity core.

Yes, you're right.
Unfortunately that code was written before it was available I guess, so I just didn't rewrite the whole thing.
I see if that's quickly feasible ;)

> 194 + g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry,
> data, NULL);
>
> Moreover this is pretty dangerous, it should at least grab a ref on the proxy.
> Alternatively such timeouts should be removed in the destructor.

Sure, again... I just followed the old school! :)

> 1114 +#include <fcntl.h>
>
> I hope this isn't needed?

Yes, no more needed. /me cleaning it.

> DBusIndicators: use glib::Object to handle the dbus proxy, so we have ref-counting for free

So much nicer, thx!

Gah... I was wanting to work out how to remove the IsAppmenu bit, but since the cure is worse than the symptoms, I'm prepared to accept it.

Attempt to merge into lp:unity failed due to conflicts:

text conflict in CMakeLists.txt
text conflict in plugins/unityshell/src/PanelIndicatorEntryView.cpp