Merge lp://staging/~seb128/xchat-indicator/use-libmessaging-menu into lp://staging/xchat-indicator

Proposed by Sebastien Bacher
Status: Merged
Approved by: Ken VanDine
Approved revision: 45
Merged at revision: 42
Proposed branch: lp://staging/~seb128/xchat-indicator/use-libmessaging-menu
Merge into: lp://staging/xchat-indicator
Diff against target: 526 lines (+123/-217)
2 files modified
configure.ac (+1/-2)
indicator.c (+122/-215)
To merge this branch: bzr merge lp://staging/~seb128/xchat-indicator/use-libmessaging-menu
Reviewer Review Type Date Requested Status
Ken VanDine Pending
Review via email: mp+121441@code.staging.launchpad.net

Description of the change

Use libmessaging-menu rather than libindicate

To post a comment you must log in.
Revision history for this message
Lars Karlitski (larsu) wrote :

Looks good to me from a libmessaging-menu point of view. A couple of comments:

* 342: Is xchat_plugin_deinit called every time the xchat-gnome process is stopped, or only when disabling the plugin? messaging_menu_app_unregister doesn't need to be called on app exit if the app should stay in the messaging menu. g_object_unref (mmapp) should be called, though.

* There's still a LIBINDICATE_REQUIRED variable in configure.ac.

These are not really important, just to let you know:

* 141, 166: append_source_with_time (mmapp, id, icon, label, g_get_real_time()) can be written as append_source (mmapp, id, icon, label) (i.e. current time is the default if nothing else is given)

* 102: has_source is not necessary before remove_source

Thanks for porting!

43. By Sebastien Bacher

updated to fix some of the issues pointed by the first code review

44. By Sebastien Bacher

use consistant tab spacing

Revision history for this message
Sebastien Bacher (seb128) wrote :

> * 342: Is xchat_plugin_deinit called every time the xchat-gnome process is stopped, or only when disabling the plugin?
> messaging_menu_app_unregister doesn't need to be called on app exit if the app should stay in the messaging menu.
> g_object_unref (mmapp) should be called, though.

good point, I changed for an unref call

> * There's still a LIBINDICATE_REQUIRED variable in configure.ac.

cleaned

> * 141, 166: append_source_with_time (mmapp, id, icon, label, g_get_real_time()) can be written as append_source
> (mmapp, id, icon, label) (i.e. current time is the default if nothing else is given)
> * 102: has_source is not necessary before remove_source

good to know, fixed as well, thanks

45. By Sebastien Bacher

try to handle focus_win_cb correctly...

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Has this been merged already ?!
/me avid xchat user

Revision history for this message
Sebastien Bacher (seb128) wrote :

it didn't get merged but got uploaded to Ubuntu for quantal

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: