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
1=== modified file 'libdbusmenu-glib/menuitem.c'
2--- libdbusmenu-glib/menuitem.c 2013-01-24 15:51:14 +0000
3+++ libdbusmenu-glib/menuitem.c 2013-05-28 14:48:33 +0000
4@@ -270,7 +270,7 @@
5 g_object_class_install_property (object_class, PROP_ID,
6 g_param_spec_int(PROP_ID_S, "ID for the menu item",
7 "This is a unique indentifier for the menu item.",
8- -1, 30000, -1,
9+ -1, G_MAXINT, -1,
10 G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));
11
12 /* Check transfer functions for GValue */
13@@ -391,7 +391,11 @@
14 case PROP_ID:
15 priv->id = g_value_get_int(value);
16 if (priv->id > menuitem_next_id) {
17- menuitem_next_id = priv->id + 1;
18+ if (priv->id == G_MAXINT) {
19+ menuitem_next_id = 1;
20+ } else {
21+ menuitem_next_id = priv->id + 1;
22+ }
23 }
24 break;
25 default:
26@@ -410,7 +414,12 @@
27 switch (id) {
28 case PROP_ID:
29 if (priv->id == -1) {
30- priv->id = menuitem_next_id++;
31+ priv->id = menuitem_next_id;
32+ if (menuitem_next_id == G_MAXINT) {
33+ menuitem_next_id = 1;
34+ } else {
35+ menuitem_next_id += 1;
36+ }
37 }
38 if (dbusmenu_menuitem_get_root(DBUSMENU_MENUITEM(obj))) {
39 g_value_set_int(value, 0);

Subscribers

People subscribed via source and target branches

to all changes: