Phase 1 of the Notifications API

Review Request #1502 — Created June 10, 2022 and submitted

grim
pidgin/pidgin
default
PIDGIN-17633
pidgin
* Created PurpleNotification with unit tests.
* Created PurpleNotificationManager with unit tests.

Ran the unit tests and ran Pidgin in the devenv.

Summary
Phase 1 of the Notifications API
Description From Last Updated

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

ivanhoeivanhoe

I can't figure out why but at this location there is an invalid read showing up in valgrind when this ...

ivanhoeivanhoe

I'm encountering these curly braces on the same line as the function name so often that I start to wonder ...

ivanhoeivanhoe

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 ...

ivanhoeivanhoe
ivanhoe
  1. 
      
  2. libpurple/purplenotificationmanager.c (Diff revision 1)
     
     

    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)
    
    1. 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?

  3. 
      
ivanhoe
  1. 
      
  2. libpurple/purplenotification.c (Diff revision 1)
     
     

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

  3. 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.

  4. libpurple/purplenotification.c (Diff revision 1)
     
     

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

  5. libpurple/purplenotification.c (Diff revision 1)
     
     

    displaying

  6. libpurple/purplenotification.c (Diff revision 1)
     
     

    Curly brace again here and next 3 functions.

  7. libpurple/purplenotification.c (Diff revision 1)
     
     

    Curly brace

  8. libpurple/purplenotification.c (Diff revision 1)
     
     

    curly brace

  9. libpurple/purplenotification.c (Diff revision 1)
     
     

    curly brace here and next two functions

  10. libpurple/purplenotification.c (Diff revision 1)
     
     

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

  11. libpurple/purplenotification.c (Diff revision 1)
     
     

    curly brace

  12. 
      
ivanhoe
  1. 
      
  2. libpurple/purplenotificationmanager.c (Diff revision 1)
     
     

    curly brace here and next five functions

  3. libpurple/purplenotificationmanager.c (Diff revision 1)
     
     

    curly brace

  4. 
      
ivanhoe
  1. 
      
  2. libpurple/tests/test_notification.c (Diff revision 1)
     
     

    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.

    1. 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.

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

    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?

    1. 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.

  3. 
      
ivanhoe
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...