- Download Diff

Summary:

Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Review Request #1356 — Created March 18, 2022 and submitted May 24, 2022, 5:14 a.m.

Information
Owner:	grim
Repository:	pidgin/pidgin
Branch:	default
Bugs:
Depends On:
Blocks:	1365, 1364
Reviewers
Groups:	pidgin
People:

Description

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.

Testing Done

Ran the new unit tests.

Commits

Summary
Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Screenshots

Issues

Description	From	Last Updated
Should be the GIR link syntax.	QuLogic	March 22, 2022, 2:12 a.m.
dynamic-target (or maybe the constant macro can be used?)	QuLogic	March 20, 2022, 5:44 a.m.
Should be [func@Account.get_id] or whatever it is.	QuLogic	March 20, 2022, 5:22 a.m.
recursing	QuLogic	March 22, 2022, 2:12 a.m.
Should use the macro instead of the dynamic-target string?	QuLogic	March 20, 2022, 5:22 a.m.
Not tabs.	QuLogic	March 20, 2022, 5:22 a.m.
This is going to be confusing for func, because it will be receiving a GMenuModel that is not the one …	QuLogic	March 22, 2022, 2:29 a.m.
Possibly should assert that n == n_items and l == NULL at the end.	QuLogic	March 20, 2022, 5:23 a.m.
Maybe rename to expected (in all tests).	QuLogic	March 20, 2022, 5:23 a.m.
So this doesn't work when the account is first enabled because it doesn't have a connection.	QuLogic	May 24, 2022, 2:55 a.m.
Missing period.	lifesfaded	March 25, 2022, 7:46 a.m.
If I've understood the connected signals correctly, this will never be true if you only have one account, since this …	QuLogic	May 24, 2022, 2:56 a.m.
Quote dynamic-target, to match the example below.	QuLogic	May 24, 2022, 5:06 a.m.
Should that not be a g_return_if_fail?	QuLogic	May 24, 2022, 5:06 a.m.
Could be g_free(item)?	QuLogic	May 24, 2022, 5:06 a.m.
Maybe also a test with a dynamic-target, but with a name that's not passed to purple_menu_populate_dynamic_targets?	QuLogic	May 24, 2022, 5:10 a.m.

Change Summary:

rebased

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 2 (+682)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c

libpurple/purplemenu.h (Diff revision 2)

Show all issues

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

grim March 19, 2022, 4:03 p.m.

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.

libpurple/purplemenu.h (Diff revision 2)

Show all issues

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

libpurple/purplemenu.c (Diff revision 2)

Show all issues

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

libpurple/purplemenu.c (Diff revision 2)
Show all issues
```
Not tabs.
```

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

Show all issues

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

libpurple/tests/test_menu.c (Diff revision 2)
Show all issues
```
Maybe rename to expected (in all tests).
```

Change Summary:

address issues in the review and add a public walk function that simplifies testing and helps us deal with the nested menu structures.

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 3 (+956 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

libpurple/purplemenu.h (Diff revisions 2 - 3)
Show all issues
```
Should be the GIR link syntax.
```
libpurple/purplemenu.h (Diff revisions 2 - 3)
Show all issues
```
recursing
```

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

Show all issues

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.

grim March 20, 2022, 9:02 a.m.

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.

QuLogic March 22, 2022, 2:17 a.m.

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

grim March 22, 2022, 2:29 a.m.

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.

QuLogic March 22, 2022, 2:30 a.m.

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

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 4 (+960 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

Ship it!

```
Ship It!
```

pidgin/pidginaccountsenabledmenu.c (Diff revision 4)

Show all issues

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

QuLogic March 22, 2022, 9:11 a.m.

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

Change Summary:

avoid a crash

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 5 (+968 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

libpurple/tests/test_menu.c (Diff revision 5)
Show all issues
```
Missing period.
```

Change Summary:

fix punctuation

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 6 (+968 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

pidgin/pidginaccountsenabledmenu.c (Diff revision 6)

Show all issues

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.

Status: Discarded

Change Summary:

dropping this because implementing a menu model is a much better way to do this.

Status: Re-opened

Change Summary:

refresh the menu when an account is done connecting as well as when it is disconnected.

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 7 (+1014 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

Change Summary:

rebased

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 8 (+1014 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

libpurple/purplemenu.h (Diff revision 8)

Show all issues

Quote dynamic-target, to match the example below.

libpurple/purplemenu.c (Diff revision 8)
Show all issues
```
Should that not be a g_return_if_fail?
```
libpurple/tests/test_menu.c (Diff revision 8)
Show all issues
```
Could be g_free(item)?
```

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

Show all issues

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

Change Summary:

rebased and addressed comments

Commits:

	Summary
-	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's
+	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Diff:

Revision 9 (+1066 -58)

Show changes

	libpurple/meson.build
	libpurple/purplemenu.h
	libpurple/purplemenu.c
	libpurple/tests/meson.build
	libpurple/tests/test_menu.c
	pidgin/pidginaccountsenabledmenu.c
	pidgin/resources/gtk/menus.ui

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 5.0.1

Add purple_menu_populate_dynamic_targets to dynamically update GMenu's

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Status: Discarded

Change Summary:

Status: Re-opened

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Status: Closed (submitted)