Messaging Menu

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

  1. clear-all-unescape-fix
  2. Merge into trunk.15.04
Proposed by Niklas Wenzel
Status: Merged
Approved by: Lars Karlitski
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) Approve
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.
Revision history for this message
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
Revision history for this message
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. ;)

Revision history for this message
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?

Revision history for this message
Niklas Wenzel (nikwen) wrote :

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

lp:~nikwen/indicator-messages/clear-all-unescape-fix updated
446. By Niklas Wenzel

Remove GArray and fix code style

447. By Niklas Wenzel

Fix build error

448. By Niklas Wenzel

Use guint instead of int

Revision history for this message
Niklas Wenzel (nikwen) wrote :

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

Revision history for this message
Lars Karlitski (larsu) wrote :

Thanks!

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

lp:~nikwen/indicator-messages/clear-all-unescape-fix updated
449. By Niklas Wenzel

Remove spaces

Revision history for this message
Niklas Wenzel (nikwen) wrote :

Done, thanks. :)

Revision history for this message
Lars Karlitski (larsu) wrote :

Thanks very much!

review: Approve
Revision history for this message
Niklas Wenzel (nikwen) wrote :

Thanks for approving. :)

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

Revision history for this message
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

Revision history for this message
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

Revision history for this message
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
1=== modified file 'src/im-application-list.c'
2--- src/im-application-list.c 2015-04-03 14:51:51 +0000
3+++ src/im-application-list.c 2015-04-30 14:24:08 +0000
4@@ -522,10 +522,26 @@
5
6 if (app->proxy != NULL) /* If it is remote, we tell the app we've cleared */
7 {
8+ guint i;
9+ gchar **unescaped_source_actions;
10+ gchar **unescaped_message_actions;
11+
12+ /* Unescape action names */
13+ unescaped_source_actions = g_new0 (gchar *, g_strv_length (source_actions) + 1);
14+ for (i = 0; source_actions[i]; i++)
15+ unescaped_source_actions[i] = unescape_action_name (source_actions[i]);
16+
17+ unescaped_message_actions = g_new0 (gchar *, g_strv_length (message_actions) + 1);
18+ for (i = 0; message_actions[i]; i++)
19+ unescaped_message_actions[i] = unescape_action_name (message_actions[i]);
20+
21 indicator_messages_application_call_dismiss (app->proxy,
22- (const gchar * const *) source_actions,
23- (const gchar * const *) message_actions,
24+ (const gchar * const *) unescaped_source_actions,
25+ (const gchar * const *) unescaped_message_actions,
26 app->cancellable, NULL, NULL);
27+
28+ g_strfreev (unescaped_source_actions);
29+ g_strfreev (unescaped_message_actions);
30 }
31
32 g_strfreev (source_actions);

Subscribers

People subscribed via source and target branches