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	ID
Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	3add3ad26aeebf88cd8a62401834285075b4c429

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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	760af769598d36c6e7ccf71912ebadd4195943ea
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	a53fa17b83b045978bccf1981060fde542bc806e

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)

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Should be [func@Account.get_id] or whatever it is.
```
libpurple/purplemenu.c (Diff revision 2)
The issue has been resolved. Show all issues
```
Should use the macro instead of the dynamic-target string?
```
libpurple/purplemenu.c (Diff revision 2)
The issue has been resolved. Show all issues
```
Not tabs.
```
libpurple/tests/test_menu.c (Diff revision 2)
The issue has been resolved. Show all issues
```
Possibly should assert that n == n_items and l == NULL at the end.
```
libpurple/tests/test_menu.c (Diff revision 2)
The issue has been resolved. 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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	a53fa17b83b045978bccf1981060fde542bc806e
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	aaa8cc22122d5de16b51daed6b9007e144826a14

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)
The issue has been resolved. Show all issues
```
Should be the GIR link syntax.
```
libpurple/purplemenu.h (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
recursing
```

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

The issue has been resolved. 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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	aaa8cc22122d5de16b51daed6b9007e144826a14
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	06c6b2e55f6760f4a4e2fd81f63623b7859d684e

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)

The issue has been resolved. 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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	06c6b2e55f6760f4a4e2fd81f63623b7859d684e
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	b048eea7c12f05b8de7f9621c0843d9cd1506b9c

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)
The issue has been resolved. Show all issues
```
Missing period.
```

Change Summary:

fix punctuation

Commits:

	Summary	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	b048eea7c12f05b8de7f9621c0843d9cd1506b9c
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	7068dd5697cb7c43167f57617098dc9a1e9cebdf

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)

The issue has been resolved. 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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	7068dd5697cb7c43167f57617098dc9a1e9cebdf
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	292bb6da167db3a96994e02a2dbf0c7581132ecf

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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	292bb6da167db3a96994e02a2dbf0c7581132ecf
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	2d5f55b9478687cd52565aac54a2220cf2a14dd7

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)
The issue has been resolved. Show all issues
```
Quote dynamic-target, to match the example below.
```
libpurple/purplemenu.c (Diff revision 8)
The issue has been resolved. Show all issues
```
Should that not be a g_return_if_fail?
```
libpurple/tests/test_menu.c (Diff revision 8)
The issue has been resolved. Show all issues
```
Could be g_free(item)?
```

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

The issue has been resolved. 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	ID
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	2d5f55b9478687cd52565aac54a2220cf2a14dd7
	Add purple_menu_populate_dynamic_targets to dynamically update GMenu's	3add3ad26aeebf88cd8a62401834285075b4c429

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!
```

Status:: Completed