Summary

Create PurplePerson.

Review Request #1838 — Created Sept. 25, 2022 and submitted Sept. 27, 2022, 7:45 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17668

Depends On

Reviewers

Groups

pidgin

People

Description

This automatically manages the sorting of contacts and will report the highest
ranked one as the priority contact.

Testing Done

Ran the unit tests

Commits

Summary	ID
Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	6d1f75b68b5351c58e2f1a900e64a006356a168c

Issues

Description	From	Last Updated
I forgot that I forgot to finish the docs, I'll get to that tonight..	grim	Sept. 26, 2022, 5:43 a.m.
"after add all the contacts" add was supposed to be adding?	lifesfaded	Sept. 26, 2022, 5:45 a.m.
priority	lifesfaded	Sept. 26, 2022, 5:45 a.m.
priority	lifesfaded	Sept. 26, 2022, 5:45 a.m.
Since	QuLogic	Sept. 27, 2022, 5:10 a.m.
user's Or maybe rewrite like purple_person_set_avatar	QuLogic	Sept. 27, 2022, 5:10 a.m.
Ref should actually go in the if; see /r/1836.	QuLogic	Sept. 27, 2022, 5:11 a.m.
Extra blank line.	QuLogic	Sept. 27, 2022, 5:11 a.m.
When you remove a contact from the person, it doesn't appear that these signal handlers are removed.	QuLogic	Sept. 27, 2022, 5:11 a.m.
But avatar is NULL, which is the same as not passing it, so it doesn't exactly confirm that this worked.	QuLogic	Sept. 27, 2022, 5:11 a.m.
const?	QuLogic	Sept. 27, 2022, 5:11 a.m.
Why no assert for the second contact?	QuLogic	Sept. 27, 2022, 5:11 a.m.
This was only for debugging, wasn't it?	QuLogic	Sept. 27, 2022, 5:11 a.m.
sorted_presence and sorted_contact are not unref'd.	QuLogic	Sept. 27, 2022, 5:12 a.m.
I think the contacts should only be sorted when removed is true to avoid unnecessary sorting. Also this causes unexpected …	ivanhoe	Sept. 27, 2022, 5:29 a.m.
Maybe skip this if nothing removed as well?	QuLogic	Sept. 27, 2022, 6:54 a.m.
This callback is not added to contact.	QuLogic	Sept. 27, 2022, 6:54 a.m.

Change Summary:

add the ticket

Bugs:

+	PIDGIN-17668

Show all issues

I forgot that I forgot to finish the docs, I'll get to that tonight..

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

Show all issues

"after add all the contacts" add was supposed to be adding?

libpurple/tests/test_person.c (Diff revision 1)
Show all issues
```
priority
```
libpurple/tests/test_person.c (Diff revision 1)
Show all issues
```
priority
```

Change Summary:

Address issues

Commits:

	Summary	ID
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	29558c3158db7119c701ee6ecbfbdb7c24ddbb75
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	3502f3475725c33ec5c25b053e5bf549ebfa082b

Diff:

Revision 2 (+2030)

Show changes

	libpurple/meson.build
	libpurple/purpleperson.h
	libpurple/purpleperson.c
	libpurple/tests/meson.build
	libpurple/tests/test_person.c

libpurple/purpleperson.h (Diff revision 2)
Show all issues
```
Since
```

libpurple/purpleperson.h (Diff revision 2)

Show all issues

user's

Or maybe rewrite like purple_person_set_avatar

libpurple/purpleperson.c (Diff revision 2)
Show all issues
```
Ref should actually go in the if; see /r/1836.
```
libpurple/purpleperson.c (Diff revision 2)
Show all issues
```
Extra blank line.
```

libpurple/purpleperson.c (Diff revision 2)

Show all issues

When you remove a contact from the person, it doesn't appear that these signal handlers are removed.

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

Show all issues

But avatar is NULL, which is the same as not passing it, so it doesn't exactly confirm that this worked.

libpurple/tests/test_person.c (Diff revision 2)
Show all issues
```
const?
```

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

Show all issues

Why no assert for the second contact?

grim Sept. 26, 2022, 11:01 a.m.

I had some reason for this at the time that made sense.. but I don't recall, and the tests pass when I just turning this to an else.

libpurple/tests/test_person.c (Diff revision 2)
Show all issues
```
This was only for debugging, wasn't it?
```

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

Show all issues

sorted_presence and sorted_contact are not unref'd.

Change Summary:

rebased and addressed issues.

Commits:

	Summary	ID
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	3502f3475725c33ec5c25b053e5bf549ebfa082b
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	362ac6e7d8da4076b280c41538f305169445710d

Diff:

Revision 3 (+2066)

Show changes

	libpurple/meson.build
	libpurple/purpleperson.h
	libpurple/purpleperson.c
	libpurple/tests/meson.build
	libpurple/tests/test_person.c

libpurple/purpleperson.c (Diff revision 3)

Show all issues

I think the contacts should only be sorted when removed is true to avoid unnecessary sorting.
Also this causes unexpected behaviour in my opinion when you consider following unit test where the wrong contact is tried to be removed:
static void
test_purple_person_no_remove(void) {
  PurpleAccount *account = NULL;
  PurpleContact *contact1 = NULL;
  PurpleContact *contact2 = NULL;
  PurplePerson *person1 = NULL;
  PurplePerson *person2 = NULL;
  gboolean person1_changed = FALSE;
  gboolean person2_changed = FALSE;

  account = purple_account_new("test", "test");
  contact1 = purple_contact_new(account, "user1");
  contact2 = purple_contact_new(account, "user2");
  person1 = purple_person_new();
  person2 = purple_person_new();

  purple_person_add_contact(person1, contact1);
  purple_person_add_contact(person2, contact2);

  g_signal_connect(person1, "items-changed",
                   G_CALLBACK(test_purple_person_items_changed_cb), &person1_changed);
  g_signal_connect(person2, "items-changed",
                   G_CALLBACK(test_purple_person_items_changed_cb), &person2_changed);

  g_assert_false(purple_person_remove_contact(person1, contact2));
  g_assert_false(person1_changed);

  g_clear_object(&person1);
  g_clear_object(&person2);
  g_clear_object(&account);
}

With the current code the second assertion fails.

grim Sept. 27, 2022, 1:30 a.m.

I agree that we should only sort if we removed something.

That said, we can't exactly test for that, because the order shouldn't change. The items aren't picked randomly and if nothing else changes, they should sort in the same order regardless.

Also, your example test doesn't need a second person, PurpleContact can and will exist outside of a PurplePerson.

Change Summary:

Don't sort in the remove function if we didn't actually remove anything.

Commits:

	Summary	ID
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	362ac6e7d8da4076b280c41538f305169445710d
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	036516628336df177f188bb096ea55cb123343ef

Diff:

Revision 4 (+2070)

Show changes

	libpurple/meson.build
	libpurple/purpleperson.h
	libpurple/purpleperson.c
	libpurple/tests/meson.build
	libpurple/tests/test_person.c

libpurple/purpleperson.c (Diff revision 4)
Show all issues
```
Maybe skip this if nothing removed as well?
```
libpurple/purpleperson.c (Diff revision 4)
Show all issues
```
This callback is not added to contact.
```

Change Summary:

address issues and avoid a use-after-free

Commits:

	Summary	ID
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	036516628336df177f188bb096ea55cb123343ef
	Create PurplePerson. This automatically manages the sorting of contacts and will report the highest ranked one as the priority contact.	6d1f75b68b5351c58e2f1a900e64a006356a168c

Diff:

Revision 5 (+2092)

Show changes

	libpurple/meson.build
	libpurple/purpleperson.h
	libpurple/purpleperson.c
	libpurple/tests/meson.build
	libpurple/tests/test_person.c

Ship it!

```
Ship It!
```

Review Board 6.0.2

Create PurplePerson.

Information

Reviewers

Change Summary:

Bugs:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Status: Closed (submitted)