Merge lp:~nikwen/indicator-messages/clear-all-unescape-fix into lp:indicator-messages/15.04

Proposed by Niklas Wenzel on 2015-04-25
Status: Merged
Approved by: Lars Karlitski on 2015-04-30
Approved revision: 449
Merged at revision: 445
Proposed branch: lp:~nikwen/indicator-messages/clear-all-unescape-fix
Merge into: lp:indicator-messages/15.04
Diff against target: 32 lines (+18/-2)
1 file modified
src/im-application-list.c (+18/-2)
To merge this branch: bzr merge lp:~nikwen/indicator-messages/clear-all-unescape-fix
Reviewer Review Type Date Requested Status
Lars Karlitski (community) 2015-04-25 Approve on 2015-04-30
Review via email: mp+257471@code.launchpad.net

Commit message

Unescape action names when passing them to the proxy in im_application_list_remove_all()

Description of the change

Unescape action names when passing them to the proxy in im_application_list_remove_all().

This fixes an issue in account-polld and ubuntu-push where no new Gmail notifications would be shown because account-polld still thinks that a "You have about %d more unread messages" notification is being shown although it has been cleared using the "Clear all" button.

To post a comment you must log in.
Lars Karlitski (larsu) wrote :

Good catch. Thanks for the patch!

Why are you using GArray? I'd prefer writing into a strv directly to save the extra allocation. It will also make the code much shorter:

  unescaped_source_actions = g_new0 (gchar *, g_strv_length (source_actions) + 1);
  for (i = 0; source_actions[i]; i++)
    unescaped_source_actions[i] = unescape_action_name (source_actions[i]);

Please follow code style of the rest of the file: put space between function names and open paren and declare all variables at the beginning of a scope.

review: Needs Fixing
Niklas Wenzel (nikwen) wrote :

Lars, thank you very much for reviewing this that fast!

I agree that your suggestion is much nicer. I'll fix the MP and adjust the code style (totally forgot about the latter, sorry).

After all, it's been the first time I used the GTK framework. ;)

Niklas Wenzel (nikwen) wrote :

Do we need to use g_new0? Isn't g_new enough since we make sure to set all items properly afterwards?

Niklas Wenzel (nikwen) wrote :

Sorry, I forgot about the last element. g_new0 looks right.

446. By Niklas Wenzel on 2015-04-30

Remove GArray and fix code style

447. By Niklas Wenzel on 2015-04-30

Fix build error

448. By Niklas Wenzel on 2015-04-30

Use guint instead of int

Niklas Wenzel (nikwen) wrote :

Ok, this seems to perform fine during testing. Lars, could you please have a look at the new version?

Lars Karlitski (larsu) wrote :

Thanks!

Sorry for the major nitpick, but do you mind removing the trailing space in line 20 of the diff?

449. By Niklas Wenzel on 2015-04-30

Remove spaces

Niklas Wenzel (nikwen) wrote :

Done, thanks. :)

Lars Karlitski (larsu) wrote :

Thanks very much!

review: Approve
Niklas Wenzel (nikwen) wrote :

Thanks for approving. :)

Will it be possible to get this into the vivid images before the RTM branch off?

Lars Karlitski (larsu) wrote :

> Will it be possible to get this into the vivid images before the RTM branch
> off?

Let's start by SRUing the fix to vivid. The RTM team can then decide whether they want it. Please update the bug according to:

  https://wiki.ubuntu.com/StableReleaseUpdates#Procedure

Niklas Wenzel (nikwen) wrote :

Hi Lars,

Actually, I was talking primarily about the phone images and apparently these follow a different release cycle than the desktop. We still have devel-proposed builds being generated and if you currently create a landing via the citrain, these changes get merged into an overlay PPA.
Next week, a new RTM series will be branched off from the vivid images plus the overlay PPA.

If we can get this merged and landed this week, this will still make it into the upcoming RTM images without having to follow the normal RTM landing procedure. I'd really love to see this happen.

Additionally, we can SRU this to vivid, of course. If you want, I can update the bug accordingly. However, the SRU procedure requires the bug task to be "Fix Released". For that, we'll need it to be merged into the development series anyway, so it will probably be better to do this as soon as possible. As mentioned, if we can get it in this week, it will save us time when it comes to getting it into the RTM builds.

Would you mind merging this and creating the landing?

Cheers,
Niklas

Useful links from the mailing list:
https://lists.launchpad.net/ubuntu-phone/msg12207.html
https://lists.launchpad.net/ubuntu-phone/msg12313.html
https://lists.launchpad.net/ubuntu-phone/msg12498.html

Niklas Wenzel (nikwen) wrote :

Thank you very much for merging this. Will there be a landing for the phone?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/im-application-list.c'
--- src/im-application-list.c 2015-04-03 14:51:51 +0000
+++ src/im-application-list.c 2015-04-30 14:24:08 +0000
@@ -522,10 +522,26 @@
522522
523 if (app->proxy != NULL) /* If it is remote, we tell the app we've cleared */523 if (app->proxy != NULL) /* If it is remote, we tell the app we've cleared */
524 {524 {
525 guint i;
526 gchar **unescaped_source_actions;
527 gchar **unescaped_message_actions;
528
529 /* Unescape action names */
530 unescaped_source_actions = g_new0 (gchar *, g_strv_length (source_actions) + 1);
531 for (i = 0; source_actions[i]; i++)
532 unescaped_source_actions[i] = unescape_action_name (source_actions[i]);
533
534 unescaped_message_actions = g_new0 (gchar *, g_strv_length (message_actions) + 1);
535 for (i = 0; message_actions[i]; i++)
536 unescaped_message_actions[i] = unescape_action_name (message_actions[i]);
537
525 indicator_messages_application_call_dismiss (app->proxy, 538 indicator_messages_application_call_dismiss (app->proxy,
526 (const gchar * const *) source_actions,539 (const gchar * const *) unescaped_source_actions,
527 (const gchar * const *) message_actions,540 (const gchar * const *) unescaped_message_actions,
528 app->cancellable, NULL, NULL);541 app->cancellable, NULL, NULL);
542
543 g_strfreev (unescaped_source_actions);
544 g_strfreev (unescaped_message_actions);
529 }545 }
530546
531 g_strfreev (source_actions);547 g_strfreev (source_actions);

Subscribers

People subscribed via source and target branches