-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use properties instead of private fields in dunstify #1444
Conversation
hi @bynect ,
|
Yes see #1443 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1444 +/- ##
=======================================
Coverage 65.32% 65.32%
=======================================
Files 50 50
Lines 8763 8763
Branches 1034 1034
=======================================
Hits 5724 5724
Misses 3039 3039
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pulled v1.12.1, wouldn't build, switched to master. Built and |
/* I'm sorry for taking a peek */ | ||
return kn->id; | ||
GValue value = G_VALUE_INIT; | ||
g_value_init(&value, G_TYPE_UINT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is really weird… on Debian, this results in:
(dunstify:5973): GLib-GObject-CRITICAL **: 14:52:22.702: g_value_get_int: assertion 'G_VALUE_HOLDS_INT (value)' failed
Seems like an INT vs UINT mismatch. But upstream declares it as an int, not uint:
https://github.com/GNOME/libnotify/blob/570982f616838abba6bdd8ca2bdb2f32f3b1d1de/libnotify/notification.c#L167
So I'm really confused how this could work anywhere else. Any ideas?
If, one the other hand, I do what notify-send does:
int get_id(NotifyNotification *n)
{
gint id;
g_object_get(G_OBJECT(n), "id", &id, NULL);
return id;
}
then it works just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very strange since ids are represented as uint (https://specifications.freedesktop.org/notification-spec/latest/protocol.html#id-1.10.3.3.4). Also that code is ignoring the fact that upstream thinks that the value should be an int. I'll have to check the code of libnotify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what version of debian/libnotify are you on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very strange since ids are represented as uint (https://specifications.freedesktop.org/notification-spec/latest/protocol.html#id-1.10.3.3.4). Also that code is ignoring the fact that upstream thinks that the value should be an int. I'll have to check the code of libnotify
Yeah they probably are uint in the dbus protocol but the property is declared in libnotify as int here: https://github.com/GNOME/libnotify/blob/570982f616838abba6bdd8ca2bdb2f32f3b1d1de/libnotify/notification.c#L167
Also what version of debian/libnotify are you on?
0.8.4-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that Debian enables assertions and whatever build of libnotify you have doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. I'm on gentoo, but maybe that's more of a glib configuration? Anyway I'll just change it like notifysend does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not tested properly but should work