Merge lp:~mr-fiorotto/wingpanel-indicator-notifications/trying-to-fix-1487291 into lp:~wingpanel-devs/wingpanel-indicator-notifications/wingpanel-indicator-notifications

Proposed by Giuliano Fiorotto on 2017-01-19
Status: Merged
Approved by: Adam Bieńkowski on 2017-03-01
Approved revision: 136
Merged at revision: 147
Proposed branch: lp:~mr-fiorotto/wingpanel-indicator-notifications/trying-to-fix-1487291
Merge into: lp:~wingpanel-devs/wingpanel-indicator-notifications/wingpanel-indicator-notifications
Diff against target: 13 lines (+2/-1)
1 file modified
src/Services/Notification.vala (+2/-1)
To merge this branch: bzr merge lp:~mr-fiorotto/wingpanel-indicator-notifications/trying-to-fix-1487291
Reviewer Review Type Date Requested Status
Adam Bieńkowski code / testing 2017-01-19 Approve on 2017-03-01
Review via email: mp+315180@code.launchpad.net

Commit message

Don't save "transient" notifications

Description of the change

Check if the transient parameter is in the hints Variant, if true don't add the notification to the list.

To post a comment you must log in.
Adam Bieńkowski (donadigo) wrote :

You should be using that check in get_valid method in Notification class instead of using it in add_entry.

review: Needs Fixing (code)
135. By Giuliano Fiorotto on 2017-01-19

changed check position to get_is_valid #1487291

Giuliano Fiorotto (mr-fiorotto) wrote :

> You should be using that check in get_valid method in Notification class
> instead of using it in add_entry.

I've moved that check in get_is_valid method in Notification class.

Adam Bieńkowski (donadigo) wrote :

Though, this code works, you are interpreting the transient value wrong. Currently you are checking if it only *exists*, while you should be checking if the "transient" value is false.

What I mean is lookup returns if the key exists not it's actual value e.g: you can execute a "Notify" method in d-feet to see how the indicator behaves with these parameters:

"AppName", 0, "dialog-information", "Summary", "Body", {}, {"transient": GLib.Variant('b', False) }, -1

You can see even if we sent "False" your solution will mark the notification invalid.

Please use "lookup_value" and "get_boolean":
var transient = hints.lookup_value ("transient", VariantType.BOOLEAN);
return ... && (transient == null || !transient.get_boolean ());

review: Needs Fixing (code / testing)
136. By Giuliano Fiorotto on 2017-02-05

fixed code accordig to @donadigo suggestion

Giuliano Fiorotto (mr-fiorotto) wrote :

We just checked if the transient field exist because in our tests if there was it was true, but of course it was wrong. Thanks for the d-feet suggestion :D

Adam Bieńkowski (donadigo) wrote :

Thanks for your work!

Looks good now.

review: Approve (code / testing)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Services/Notification.vala'
2--- src/Services/Notification.vala 2016-12-18 14:06:35 +0000
3+++ src/Services/Notification.vala 2017-02-05 18:32:07 +0000
4@@ -127,7 +127,8 @@
5 }
6
7 public bool get_is_valid () {
8- return app_info != null && hints.lookup_value (X_CANONICAL_PRIVATE_KEY, null) == null;
9+ var transient = hints.lookup_value("transient", VariantType.BOOLEAN);
10+ return app_info != null && hints.lookup_value (X_CANONICAL_PRIVATE_KEY, null) == null && (transient == null || !transient.get_boolean ());
11 }
12
13 public void close () {

Subscribers

People subscribed via source and target branches