Code review comment for lp:~desrt/indicator-appmenu/hud-rewrite-wip

Revision history for this message
Allison Karlitskaya (desrt) wrote :

> * About the text conflict, we could use <glib/gi18n.h> instead of locale.h and
> libintl.h

How do I resolve conflicts that only exist in merge requests? Do I have to merge up my branch? I guess that's what I'll try...

The intl headers are from Seb's earlier unrelated patch -- they only conflict because I also added a header at the same place. He's not using _() so the glib header is not really required...

> * In hud_indicator_source_name_vanished(), I think you meant to invoke
> hud_source_unuse()?

Yes! Good catch. Thanks :)

> * I think the answer is probably no, but wanted to ask if hud_window_source's
> dispose or finalize need to do anything about active_collector or its
> "changed" signal that hud_window_source connected to.

Strictly speaking, the answer is that I should be doing this. At the same time, though, it's not a bug, because the window source finalizer is never run (since the hud service runs forever and quits by exiting, without freeing the sources). I was already planning to do a bit of a finalizer cleanup so that we could add support for freeing things on exit (to increase valgrindability) so I'll make this change now as well. Thanks.

« Back to merge proposal