Summary

Phase 1 of the Notifications API

Review Request #1502 — Created June 10, 2022 and submitted June 15, 2022, 5:33 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17633

Depends On

Reviewers

Groups

pidgin

People

Description

* Created PurpleNotification with unit tests.
* Created PurpleNotificationManager with unit tests.

Testing Done

Ran the unit tests and ran Pidgin in the devenv.

Commits

Summary	ID
Phase 1 of the Notifications API * Created PurpleNotification with unit tests. * Created PurpleNotificationManager with unit tests.	34edd95f2ab1afd7332c9cdae22a73a72904814b

Issues

Description	From	Last Updated
Should there be a check that interactive actually changed, like it is done in purple_notification_set_read?	ivanhoe	June 13, 2022, 5:11 a.m.
I can't figure out why but at this location there is an invalid read showing up in valgrind when this …	ivanhoe	June 13, 2022, 5:11 a.m.
I'm encountering these curly braces on the same line as the function name so often that I start to wonder …	ivanhoe	June 11, 2022, 1:28 a.m.
Is this correct? The notifications hash table is created with g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_object_unref); so my understanding is that the …	ivanhoe	June 13, 2022, 5:13 a.m.

libpurple/purplenotificationmanager.c (Diff revision 1)

The issue has been resolved. Show all issues

I can't figure out why but at this location there is an invalid read showing up in valgrind when this function is called a 2nd time in test_purple_notification_manager_double_remove.

==13780== Invalid read of size 1
==13780==    at 0x49F72FA: g_str_hash (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x49F5E97: g_hash_table_lookup (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x48D08AB: purple_notification_manager_remove (purplenotificationmanager.c:334)
==13780==    by 0x109BA8: test_purple_notification_manager_double_remove (test_notification_manager.c:158)
==13780==    by 0x4A32694: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A323BA: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A32B98: g_test_run_suite (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A32C0C: g_test_run (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x10A481: main (test_notification_manager.c:250)
==13780==  Address 0x10d340d0 is 0 bytes inside a block of size 37 free'd
==13780==    at 0x4840EB7: free (vg_replace_malloc.c:872)
==13780==    by 0x4A0F177: g_free (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x48CF5AD: purple_notification_finalize (purplenotification.c:227)
==13780==    by 0x496ABE2: g_object_unref (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)
==13780==    by 0x48D09C7: purple_notification_manager_remove (purplenotificationmanager.c:365)
==13780==    by 0x109B6B: test_purple_notification_manager_double_remove (test_notification_manager.c:156)
==13780==    by 0x4A32694: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A323BA: ??? (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A32B98: g_test_run_suite (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A32C0C: g_test_run (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x10A481: main (test_notification_manager.c:250)
==13780==  Block was alloc'd at
==13780==    at 0x483E670: malloc (vg_replace_malloc.c:381)
==13780==    by 0x4B7F618: __vasprintf_internal (vasprintf.c:71)
==13780==    by 0x4A59035: g_vasprintf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A29B44: g_strdup_vprintf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A29B8F: g_strdup_printf (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x4A480FD: g_uuid_string_random (in /usr/lib/i386-linux-gnu/libglib-2.0.so.0.7200.2)
==13780==    by 0x48CEEFA: purple_notification_set_id (purplenotification.c:66)
==13780==    by 0x48CF6CE: purple_notification_init (purplenotification.c:243)
==13780==    by 0x4986887: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)
==13780==    by 0x496B16D: ??? (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)
==13780==    by 0x496CA56: g_object_new_valist (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)
==13780==    by 0x496CE78: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.7200.2)

ivanhoe June 10, 2022, 7:34 p.m.

This would be solved by checking the hash table size and returning early if it's 0 before doing the hash_table_lookup. But shouldn't g_hash_table_lookup check if the hash table is zero?

libpurple/purplenotification.c (Diff revision 1)

I think that curly brace at the end should get a new line :)

libpurple/purplenotification.c (Diff revision 1)

I think that curly brace at the end should get a new line. Same for purple_notification_initand purple_notification_class_init.

libpurple/purplenotification.c (Diff revision 1)

I think it would read better if it were This **is** always represented ...

libpurple/purplenotification.c (Diff revision 1)
```
displaying
```

libpurple/purplenotification.c (Diff revision 1)

Curly brace again here and next 3 functions.

libpurple/purplenotification.c (Diff revision 1)
```
Curly brace
```
libpurple/purplenotification.c (Diff revision 1)
```
curly brace
```

libpurple/purplenotification.c (Diff revision 1)

curly brace here and next two functions

libpurple/purplenotification.c (Diff revision 1)

The issue has been resolved. Show all issues

Should there be a check that interactive actually changed, like it is done in purple_notification_set_read?

libpurple/purplenotification.c (Diff revision 1)
```
curly brace
```

libpurple/purplenotificationmanager.c (Diff revision 1)
```
curly brace here and next five functions
```
libpurple/purplenotificationmanager.c (Diff revision 1)
```
curly brace
```

libpurple/tests/test_notification.c (Diff revision 1)

The issue has been dropped. Show all issues

I'm encountering these curly braces on the same line as the function name so often that I start to wonder if I remember your style preferences correctly. They are all over the place here too, but there are also instances where they are on their own line so one or the other should be fixed.

grim June 11, 2022, 1:27 a.m.

Braces are dynamic depending on the context to help improve readability. They go on the line if they fit. If arguments have to wrap, then the brace goes on the next line by itself.
Simple case:

static void
foo_bar(const gchar *arg1) {
}


Wrapping args:

static void
foo_bar(const gchar *arg1, const gchar *arg2, const gchar *arg3, const gchar *arg4, const gchar *arg5,
        const gchar *arg6, const gchar *arg7)
{
}


Always putting the brace on a new line is a way and precisely why I abhor Allman C style. However, K&R says the braces for a function goes on a newline as well.. But that hinders readiblity but adding additional lines I need to process while skimming.
But I hear you saying, then why the brace on it's own line when the args wrap.. That's easy...

static void
foo_bar(const gchar *arg1, const gchar *arg2, const gchar *arg3, const gchar *arg4, const gchar *arg5,
        const gchar *arg6, const gchar *arg7) {
    printf("hello world!\n");
}


Since the parameter list is aligned to the first parameter, now we have this weird indentation to deal with that could be an additional argument or could be code. This makes it impossible to skim the code and you're forced to process both lines completely to understand it.
And just to circle back.. When the parameters don't wrap, there's an unambiguous indentation so the code skimability is high again.

static void
foo_bar(const ghcar *arg1) {
    printf("hello world!\n");
}


So again, while skimming the code, i can concentrate on just the specific indentation level I'm interested in and nothing more.

Change Summary:

fix the invalid read and only notify when the interactive property actually changes.

Commits:

	Summary	ID
	Phase 1 of the Notifications API * Created PurpleNotification with unit tests. * Created PurpleNotificationManager with unit tests.	a033148eb7a63c2502ec70118f2ec1fa825e2562
	Phase 1 of the Notifications API * Created PurpleNotification with unit tests. * Created PurpleNotificationManager with unit tests.	34edd95f2ab1afd7332c9cdae22a73a72904814b

Diff:

Revision 2 (+3430)

Show changes

	libpurple/core.c
	libpurple/meson.build
	libpurple/purplenotification.h
	libpurple/purplenotification.c
	libpurple/purplenotificationmanager.h
	libpurple/purplenotificationmanager.c
	libpurple/purpleprivate.h
	libpurple/tests/meson.build
	3 more

libpurple/tests/test_notification_manager.c (Diff revisions 1 - 2)

The issue has been dropped. Show all issues

Is this correct? The notifications hash table is created with g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_object_unref); so my understanding is that the id is not freed when the entry is removed from the hash table.
If I understand your comment correctly wouldn't an alternative solution be to strdup the id? I tried that and it does not solve the invalid read.

And regardless of me understanding this comment, is this a well usable API? I mean in this test case it is simple but when used in real application code you don't know in advance that an id is attempted to be removed a 2nd time. Would you then always acquire an additional reference, just in case? In test_purple_notification_manager_add_remove for example, the additional reference is not acquired, would we do that in real application code?

grim June 11, 2022, 10:07 a.m.

You pass a GDestroyNotify for the key if you're putting in allocated data that you own. The PurpleNotification owns the pointer that we're using as a key, not the PurpleNotificationManager. As you mentioned, we could g_strdup() the key, but that's wasteful when the PurpleNotificationManager is already taking ownership of the PurpleNotification. In this case, the calling code was incorrect because it no longer owned a reference.

We don't add another reference in test_purple_notification_manager_add_remove because the life time of the pointer is still good even though we don't have ownership of it. As for doing this in real application code, probably not. Or the id would most likely be duplicated. But it's hard to say for certain, because we haven't figured out those specific use cases yet.

Ship it!

```
Ship It!
```

Status:: Completed