Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Review Request #1356 — Created March 18, 2022 and submitted

grim
pidgin/pidgin
default
1365, 1364
pidgin

This is going to be used in the GMenu's for plugins, protocols, accounts, etc where there are dynamic items and we need to be able to set a target for them dynamically.

Ran the new unit tests.

Summary
Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
Description From Last Updated

Should be the GIR link syntax.

QuLogicQuLogic

dynamic-target (or maybe the constant macro can be used?)

QuLogicQuLogic

Should be [func@Account.get_id] or whatever it is.

QuLogicQuLogic

recursing

QuLogicQuLogic

Should use the macro instead of the dynamic-target string?

QuLogicQuLogic

Not tabs.

QuLogicQuLogic

This is going to be confusing for func, because it will be receiving a GMenuModel that is not the one …

QuLogicQuLogic

Possibly should assert that n == n_items and l == NULL at the end.

QuLogicQuLogic

Maybe rename to expected (in all tests).

QuLogicQuLogic

So this doesn't work when the account is first enabled because it doesn't have a connection.

QuLogicQuLogic

Missing period.

lifesfadedlifesfaded

If I've understood the connected signals correctly, this will never be true if you only have one account, since this …

QuLogicQuLogic

Quote dynamic-target, to match the example below.

QuLogicQuLogic

Should that not be a g_return_if_fail?

QuLogicQuLogic

Could be g_free(item)?

QuLogicQuLogic

Maybe also a test with a dynamic-target, but with a name that's not passed to purple_menu_populate_dynamic_targets?

QuLogicQuLogic
grim
QuLogic
  1. 
      
  2. libpurple/purplemenu.h (Diff revision 2)
     
     

    dynamic-target (or maybe the constant macro can be used?)

    1. I'm mixed on using the macro here as we can't use the macro in the .ui files which is what people are going to be doing way more often.

  3. libpurple/purplemenu.h (Diff revision 2)
     
     

    Should be [func@Account.get_id] or whatever it is.

  4. libpurple/purplemenu.c (Diff revision 2)
     
     

    Should use the macro instead of the dynamic-target string?

  5. libpurple/purplemenu.c (Diff revision 2)
     
     
     
     

    Not tabs.

  6. libpurple/tests/test_menu.c (Diff revision 2)
     
     

    Possibly should assert that n == n_items and l == NULL at the end.

  7. libpurple/tests/test_menu.c (Diff revision 2)
     
     

    Maybe rename to expected (in all tests).

  8. 
      
grim
QuLogic
  1. 
      
  2. libpurple/purplemenu.h (Diff revisions 2 - 3)
     
     

    Should be the GIR link syntax.

  3. libpurple/purplemenu.h (Diff revisions 2 - 3)
     
     

    recursing

  4. libpurple/purplemenu.c (Diff revisions 2 - 3)
     
     

    This is going to be confusing for func, because it will be receiving a GMenuModel that is not the one that was passed in to purple_menu_walk.

    1. Yeah that's why in our handler we check to make sure it's still a GMenu. Thoughts on how to handle this? It seemed intrinsic to me, but if you're not seeing that way so will others, so we should probably document it.

    2. The documentation is good; I guess you could pass root and actual, but maybe that's too much?

    3. I don't know that passing root actually does anyone any good? All of the items are index via an integer and you wouldn't have that especially when you're more than one level deep.

    4. Ah true, that seems a bit too much work then.

  5. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
QuLogic
  1. 
      
  2. pidgin/pidginaccountsenabledmenu.c (Diff revision 4)
     
     

    So this doesn't work when the account is first enabled because it doesn't have a connection.

    1. Here's the backtrace on this:

      (pidgin3:1150718): Purple-CRITICAL **: 05:09:50.696: purple_connection_get_id: assertion 'PURPLE_IS_CONNECTION(connection)' failed
      
      (gdb) bt
      #0  0x00007ffff7de2f77 in g_logv () at /lib64/libglib-2.0.so.0
      #1  0x00007ffff7de3233 in g_log () at /lib64/libglib-2.0.so.0
      #2  0x00007ffff7a732c8 in purple_connection_get_id (connection=0x0) at ../libpurple/connection.c:283
      #3  0x00007ffff7f65dce in pidgin_accounts_enabled_menu_refresh_helper (account=0x7fff80009ec0, data=0x1511840) at ../pidgin/pidginaccountsenabledmenu.c:50
      #4  0x00007ffff7a95280 in purple_account_manager_foreach (manager=0x6dc780, callback=0x7ffff7f65d23 <pidgin_accounts_enabled_menu_refresh_helper>, data=0x1511840) at ../libpurple/purpleaccountmanager.c:323
      #5  0x00007ffff7f65f49 in pidgin_accounts_enabled_menu_refresh (menu=0x1511840) at ../pidgin/pidginaccountsenabledmenu.c:89
      #6  0x00007ffff7f6613f in pidgin_accounts_enabled_menu_new () at ../pidgin/pidginaccountsenabledmenu.c:166
      #7  0x00007ffff7f6811b in pidgin_application_populate_dynamic_menus (application=0x4720f0) at ../pidgin/pidginapplication.c:141
      #8  0x00007ffff7f68cf8 in pidgin_application_startup (application=0x4720f0) at ../pidgin/pidginapplication.c:724
      #9  0x00007ffff79f7aba in g_signal_emit_valist () at /lib64/libgobject-2.0.so.0
      #10 0x00007ffff79f7c03 in g_signal_emit () at /lib64/libgobject-2.0.so.0
      #11 0x00007ffff6dce841 in g_application_register () at /lib64/libgio-2.0.so.0
      #12 0x00007ffff6dcefae in g_application_real_local_command_line () at /lib64/libgio-2.0.so.0
      #13 0x00007ffff6dcf366 in g_application_run () at /lib64/libgio-2.0.so.0
      #14 0x00007ffff7f6042a in pidgin_start (argc=1, argv=0x7fffffffcf38) at ../pidgin/libpidgin.c:446
      #15 0x00000000004011b9 in main (argc=1, argv=0x7fffffffcf38) at ../pidgin/pidgin.c:53
      
  3. 
      
grim
lifesfaded
  1. 
      
  2. libpurple/tests/test_menu.c (Diff revision 5)
     
     

    Missing period.

  3. 
      
grim
QuLogic
  1. 
      
  2. pidgin/pidginaccountsenabledmenu.c (Diff revision 6)
     
     

    If I've understood the connected signals correctly, this will never be true if you only have one account, since this is only triggered on account enabled/disabled.

    If you have multiple accounts, then this may be true if the second account gets enabled after the first gets a connection, but that depends on some timing.

  3. 
      
grim
grim
grim
grim
QuLogic
  1. 
      
  2. libpurple/purplemenu.h (Diff revision 8)
     
     

    Quote dynamic-target, to match the example below.

  3. libpurple/purplemenu.c (Diff revision 8)
     
     
     

    Should that not be a g_return_if_fail?

  4. libpurple/tests/test_menu.c (Diff revision 8)
     
     

    Could be g_free(item)?

  5. libpurple/tests/test_menu.c (Diff revision 8)
     
     

    Maybe also a test with a dynamic-target, but with a name that's not passed to purple_menu_populate_dynamic_targets?

  6. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...