DBus Menu

Merge lp:~chrisccoulson/libdbusmenu/lp1011073 into lp:libdbusmenu/13.10

  1. lp1011073
  2. Merge into trunk.13.10
Proposed by Chris Coulson
Status: Merged
Approved by: Ted Gould
Approved revision: 446
Merged at revision: 445
Proposed branch: lp:~chrisccoulson/libdbusmenu/lp1011073
Merge into: lp:libdbusmenu/13.10
Diff against target: 39 lines (+12/-3)
1 file modified
libdbusmenu-glib/menuitem.c (+12/-3)
To merge this branch: bzr merge lp:~chrisccoulson/libdbusmenu/lp1011073
Reviewer Review Type Date Requested Status
Ted Gould (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Mathieu Trudel-Lapierre Approve
Review via email: mp+164545@code.launchpad.net

Commit message

Fix the long-standing "nm-applet stops working after a few days / hours" issue; properly handle "unique" IDs for indicator menuitems.

Description of the change

This should fix the long-standing "nm-applet stops working after a few days / hours" issue :)

To post a comment you must log in.
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Approve. looks good to me.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:445
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~chrisccoulson/libdbusmenu/lp1011073/+merge/164545/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/libdbusmenu-ci/3/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libdbusmenu-raring-amd64-ci/3

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libdbusmenu-ci/3/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

"&=" is not an appropriate way to deal with integers, it's premature
optimization. It's find to use something like:

if (menuitem_next_id >= G_MAXINT) {
�* DO STUFF *�
}

That makes it much easier for the person fixing bugs in this code in the
future. It documents what you're thinking.

  review -1

review: Disapprove
Revision history for this message
Ted Gould (ted) :
review: Needs Fixing
Revision history for this message
Ted Gould (ted) wrote :

Hmm, "review -1" in e-mail is more severe than I thought.

Revision history for this message
Charles Kerr (charlesk) wrote :

Besides the fact that "&=" is an unusual way to deal with the boundary case, it's also inconsistent with the more conventional % implementation at line 391.

Chris, this would be a great fix to get in, could you revise this?

lp:~chrisccoulson/libdbusmenu/lp1011073 updated
446. By Chris Coulson

Update for review comments

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:446
http://jenkins.qa.ubuntu.com/job/libdbusmenu-ci/4/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libdbusmenu-saucy-amd64-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libdbusmenu-ci/4/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ted Gould (ted) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'libdbusmenu-glib/menuitem.c'
--- libdbusmenu-glib/menuitem.c 2013-01-24 15:51:14 +0000
+++ libdbusmenu-glib/menuitem.c 2013-05-28 14:48:33 +0000
@@ -270,7 +270,7 @@
270 g_object_class_install_property (object_class, PROP_ID,270 g_object_class_install_property (object_class, PROP_ID,
271 g_param_spec_int(PROP_ID_S, "ID for the menu item",271 g_param_spec_int(PROP_ID_S, "ID for the menu item",
272 "This is a unique indentifier for the menu item.",272 "This is a unique indentifier for the menu item.",
273 -1, 30000, -1,273 -1, G_MAXINT, -1,
274 G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));274 G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));
275275
276 /* Check transfer functions for GValue */276 /* Check transfer functions for GValue */
@@ -391,7 +391,11 @@
391 case PROP_ID:391 case PROP_ID:
392 priv->id = g_value_get_int(value);392 priv->id = g_value_get_int(value);
393 if (priv->id > menuitem_next_id) {393 if (priv->id > menuitem_next_id) {
394 menuitem_next_id = priv->id + 1;394 if (priv->id == G_MAXINT) {
395 menuitem_next_id = 1;
396 } else {
397 menuitem_next_id = priv->id + 1;
398 }
395 }399 }
396 break;400 break;
397 default:401 default:
@@ -410,7 +414,12 @@
410 switch (id) {414 switch (id) {
411 case PROP_ID:415 case PROP_ID:
412 if (priv->id == -1) {416 if (priv->id == -1) {
413 priv->id = menuitem_next_id++;417 priv->id = menuitem_next_id;
418 if (menuitem_next_id == G_MAXINT) {
419 menuitem_next_id = 1;
420 } else {
421 menuitem_next_id += 1;
422 }
414 }423 }
415 if (dbusmenu_menuitem_get_root(DBUSMENU_MENUITEM(obj))) {424 if (dbusmenu_menuitem_get_root(DBUSMENU_MENUITEM(obj))) {
416 g_value_set_int(value, 0);425 g_value_set_int(value, 0);

Subscribers

People subscribed via source and target branches

to all changes: