Merge lp://qastaging/~3v1n0/unity/dbus-indicators-proxy into lp://qastaging/unity

787 + bool auto_reconnect)

What are you actually trying to achieve with this? GDBusProxy automatically reconnects itself if you construct it using a known service name (as opposed to unique bus address). Therefore it seems redundant to me.

If you want to make sure that a service is alive, all you need to do is watch changes to the connected state and if it disconnects, just call any method from given interface and that will dbus-activate the service again.

> 787 + bool auto_reconnect)
>
> What are you actually trying to achieve with this? GDBusProxy automatically
> reconnects itself if you construct it using a known service name (as opposed
> to unique bus address). Therefore it seems redundant to me.

Yeah, I know, but it doesn't work here for the reason we discovered...

> If you want to make sure that a service is alive, all you need to do is watch
> changes to the connected state and if it disconnects, just call any method
> from given interface and that will dbus-activate the service again.

That's what I've tried to do, but it doesn't work here because glib::DBusProxy notify us about the disconnection event when actually the name has vanished, but the proxy is not disconnected yet.
Adding a timeout fixes it, but we should handle that in a better way.

> 787 + bool auto_reconnect)
>
> What are you actually trying to achieve with this? GDBusProxy automatically
> reconnects itself if you construct it using a known service name (as opposed
> to unique bus address). Therefore it seems redundant to me.

Ok this is now fixed into lp:~3v1n0/unity/dbus-indicators-proxy that will merge in this one as you know ;)

> Ok this is now fixed into lp:~3v1n0/unity/dbus-indicators-proxy that will
> merge in this one as you know ;)

Ops, I meant lp:~3v1n0/unity/dbus-proxy-name-owner-watch

The prerequisite lp:~3v1n0/unity/indicators-p has not yet been merged into lp:unity.

Overall I'm liking this a lot - mostly because of "(+234/-516)" :) I have some concerns though:

269 + RequestSyncAll();
270 +
271 + reconnect_timeout_id_ = g_timeout_add_seconds(1, [](gpointer data) -> gboolean {
272 + auto self = static_cast<DBusIndicators::Impl*>(data);
273 +
274 + if (!self->gproxy_.IsConnected())
275 + {
276 + self->RequestSyncAll();
277 + return TRUE;
278 + }
279 +
280 + self->reconnect_timeout_id_ = 0;
281 + return FALSE;
282 + }, this);

Are you sure you want to call RequestSyncAll also outside of the lambda expression? Plus you should be checking if reconnect_timeout_id == 0, before calling g_timeout_add...

146 + struct CallData
147 + {
148 + Impl* self;
149 + GVariant *parameters;
150 + };

I'm not really liking this, it's unclear who owns the variant (or rather why isn't it unreffed) - using our Variant wrapper should fix this. What's worse is that the CallData can be alive even after the Impl instance is destroyed which can lead to a crash.

> Overall I'm liking this a lot - mostly because of "(+234/-516)" :)

Ehehe ;)

> Are you sure you want to call RequestSyncAll also outside of the lambda
> expression?

Yes. Because after we get a disconnection event we can try immediately to restore the service, it that fails we retry every 1 second. This assures that if we have a valid u-p-s running or in the system as well, we use it!

> Plus you should be checking if reconnect_timeout_id == 0, before
> calling g_timeout_add...

Yes, sure.

> 146 + struct CallData
> 147 + {
> 148 + Impl* self;
> 149 + GVariant *parameters;
> 150 + };
>
> I'm not really liking this, it's unclear who owns the variant (or rather why
> isn't it unreffed) - using our Variant wrapper should fix this.

Well, as you know the proxy handles it, so I didn't ref/unreff'ed it... But I'll move to glib::Variant.

> What's worse
> is that the CallData can be alive even after the Impl instance is destroyed
> which can lead to a crash.

This should be very unlikely using an idle, but I'll make that more safe as well.

Awesome, thanks!

No proposals found for merge of lp:~3v1n0/unity/indicators-p into lp:unity.