Summary

Serialize the ContactManager

Review Request #2054 — Created Nov. 10, 2022 and updated June 13, 2023, 7:37 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17687

Depends On

Reviewers

Groups

pidgin

People

Description

This is saved in a SQLite database inside the user config directory named
contacts.db. It does its best to synchronize with PurpleBuddy, but not
everything emits a signal when it changes.

Also, stuff like custom avatars are ignored as those are currently store
on the Contact and we don't have a mapping to PurplePerson yet.

Testing Done

Ran with the demo account and verified everything about the contacts was stored via the sqlite3 command line.

Commits

Summary	ID
Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	5ba998bd1b25c115904aab504d80b2fd6531a2a9

Issues

Description	From	Last Updated
Figure out how to handle unit tests. This doens't know when we're testing or not, so it always tries to …	grim	Nov. 29, 2022, 4:07 a.m.
Does this need a rebase for PurpleContactInfo stuff?	QuLogic	Dec. 4, 2022, 8:38 a.m.
Why is this in the account manager header?	QuLogic	Dec. 2, 2022, 7:05 a.m.
NULL works here.	QuLogic	Dec. 2, 2022, 7:06 a.m.
name and value are not freed here.	QuLogic	Dec. 2, 2022, 7:06 a.m.
name and value are not freed at the end of the loop.	QuLogic	Dec. 2, 2022, 7:06 a.m.
Not sure why this calls sync_tags but not update_contact_tags? And should everything get a transaction around it?	QuLogic	Dec. 4, 2022, 8:39 a.m.
Ditto.	QuLogic	Dec. 4, 2022, 8:39 a.m.
Don't need the semicolon, I think?	QuLogic	Dec. 2, 2022, 7:07 a.m.
Extra tab?	QuLogic	Dec. 4, 2022, 10:25 p.m.
Would contacts ever change accounts?	QuLogic	June 13, 2023, 2:58 a.m.
sqlite3_prepare_v2 is preferred.	QuLogic	June 13, 2023, 2:58 a.m.
stmt is not finalized in error or success.	QuLogic	June 13, 2023, 2:58 a.m.
stmt is not finalized in error or success.	QuLogic	June 13, 2023, 2:58 a.m.
I believe it might be better to prepare this outside the loop, and bind/step inside only. See lifecycle at https://www.sqlite.org/c3ref/stmt.html	QuLogic	June 13, 2023, 2:58 a.m.
But if stmt preparation is not moved outside the loop, then this one is not finalized in the case of …	QuLogic	June 13, 2023, 2:58 a.m.
But if stmt preparation is not moved outside the loop, then this one is not finalized in the case of …	QuLogic	June 15, 2023, 6:35 a.m.
Same here about moving outside loop.	QuLogic	June 15, 2023, 6:35 a.m.
Same here about finalizing on success.	QuLogic	June 13, 2023, 2:58 a.m.
Can move into the only if that uses it.	QuLogic	June 13, 2023, 2:58 a.m.
It's just gpointer.	QuLogic	June 13, 2023, 2:58 a.m.
Can be combined into g_clear_list.	QuLogic	June 13, 2023, 2:58 a.m.

Change Summary:: Fix typos.
Description:: This is saved in a SQLite database inside the user config directory named
~ contacts.db. It does it's best to synchronize with PurpleBuddy, but not
~ contacts.db. It does its best to synchronize with PurpleBuddy, but not
everything emits a signal when it changes.

~
Also, stuff like custom avatars are
~ ignore as those are currently store on the Contact and we don't have a mapping
~
Also, stuff like custom avatars are ignored as those are currently store
~ on the Contact and we don't have a mapping to PurplePerson yet.
- to PurplePerson yet.

Change Summary:

rebase, just found a bug in the unit tests that needs to be addressed yet.

Commits:

	Summary	ID
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	50d9d3808fd1e3e7fd6df28e9256fb6b9d9862fc
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	6e350f0c88c5c0fea35bb7734c7dd755fa89c851

Diff:

Revision 2 (+974 -10)

Show changes

	libpurple/core.c
	libpurple/purpleaccountmanager.h
	libpurple/purplecontactmanager.h
	libpurple/purplecontactmanager.c
	libpurple/purpleprivate.h
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/resources/libpurple.gresource.xml
	1 more

Change Summary:: add the issue that this is for
Bugs:: PIDGIN-17687

libpurple/purpleaccountmanager.h (Diff revision 2)
The issue has been resolved. Show all issues
```
Why is this in the account manager header?
```
1. grim Nov. 29, 2022, 3:46 a.m.
  I must have fixed up my open files...
libpurple/purplecontactmanager.c (Diff revision 2)
The issue has been resolved. Show all issues
```
NULL works here.
```

libpurple/purplecontactmanager.c (Diff revision 2)

The issue has been resolved. Show all issues

name and value are not freed here.

grim Nov. 29, 2022, 4:05 a.m.

They're free'd via the bind method.
I used the following POC to test. Notice the typo in the statement, you can correct it and verify that we still get double free's for both success and failure.

#include <glib.h>
#include <sqlite3.h>

int
main(int argc, char *argv[]) {
    sqlite3 *db = NULL;
    sqlite3_stmt *stmt = NULL;
    char *bar = NULL;

    sqlite3_open(":memory:", &db);

    sqlite3_prepare(db, "select * from foo wher bar=?", -1, &stmt, NULL);

    bar = g_strdup("allocated string");

    sqlite3_bind_text(stmt, 1, bar, -1, g_free);

    sqlite3_step(stmt);

    g_free(bar);

    return 0;
}

grim Nov. 29, 2022, 4:06 a.m.

Derp, this was supposed to be on the other comment below. This block was leaking.

libpurple/purplecontactmanager.c (Diff revision 2)

The issue has been resolved. Show all issues

name and value are not freed at the end of the loop.

grim Nov. 29, 2022, 4:05 a.m.
```
They're free'd via the bind method.
```

grim Nov. 29, 2022, 4:06 a.m.

See the comment above for why this is already free'd.

libpurple/purplecontactmanager.c (Diff revision 2)

The issue has been resolved. Show all issues

Not sure why this calls sync_tags but not update_contact_tags? And should everything get a transaction around it?

libpurple/purplecontactmanager.c (Diff revision 2)
The issue has been resolved. Show all issues
```
Ditto.
```
libpurple/purplecontactmanager.c (Diff revision 2)
The issue has been resolved. Show all issues
```
Don't need the semicolon, I think?
```

Fix it!

An issue was opened. Show all issues

Figure out how to handle unit tests. This doens't know when we're testing or not, so it always tries to create purple_user_config_dir() / 'contacts.db' which causes failures.

Change Summary:

rebase and address issues, the unit test failures remain however.

Commits:

	Summary	ID
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	6e350f0c88c5c0fea35bb7734c7dd755fa89c851
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	d56d5b5ae5213ec31d9468012801c73c5ab1a694

Diff:

Revision 3 (+994 -10)

Show changes

	libpurple/core.c
	libpurple/purplecontactmanager.h
	libpurple/purplecontactmanager.c
	libpurple/purpleprivate.h
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/resources/libpurple.gresource.xml
	libpurple/resources/contactmanager/01-initial.sql

Fix it!

An issue was opened. Show all issues

Does this need a rebase for PurpleContactInfo stuff?

libpurple/purplecontactmanager.c (Diff revision 3)
The issue has been resolved. Show all issues
```
Extra tab?
```

Change Summary:

rebase, fix whitespace

Commits:

	Summary	ID
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	d56d5b5ae5213ec31d9468012801c73c5ab1a694
	Serialize the ContactManager This is saved in a SQLite database inside the user config directory named contacts.db. It does it's best to synchronize with PurpleBuddy, but not everything emits a signal when it changes. Also, stuff like custom avatars are ignore as those are currently store on the Contact and we don't have a mapping to PurplePerson yet.	5ba998bd1b25c115904aab504d80b2fd6531a2a9

Diff:

Revision 4 (+994 -10)

Show changes

	libpurple/core.c
	libpurple/purplecontactmanager.h
	libpurple/purplecontactmanager.c
	libpurple/purpleprivate.h
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/resources/libpurple.gresource.xml
	libpurple/resources/contactmanager/01-initial.sql

Fix it!

libpurple/purplecontactmanager.c (Diff revision 4)

An issue was opened. Show all issues

Would contacts ever change accounts?

grim June 13, 2023, 4:24 a.m.

A contact is supposed to just tie a contact info to an account, so they shouldn't.

QuLogic June 13, 2023, 7:37 a.m.

Should there be account_id in the UPDATE then, or should it be part of the WHERE?

libpurple/purplecontactmanager.c (Diff revision 4)
An issue was opened. Show all issues
```
sqlite3_prepare_v2 is preferred.
```

libpurple/purplecontactmanager.c (Diff revision 4)

An issue was opened. Show all issues

stmt is not finalized in error or success.

libpurple/purplecontactmanager.c (Diff revision 4)

An issue was opened. Show all issues

stmt is not finalized in error or success.

libpurple/purplecontactmanager.c (Diff revision 4)

An issue was opened. Show all issues

I believe it might be better to prepare this outside the loop, and bind/step inside only.

See lifecycle at https://www.sqlite.org/c3ref/stmt.html

libpurple/purplecontactmanager.c (Diff revision 4)

An issue was opened. Show all issues

But if stmt preparation is not moved outside the loop, then this one is not finalized in the case of success.

libpurple/purplecontactmanager.c (Diff revision 4)

The issue has been dropped. Show all issues

But if stmt preparation is not moved outside the loop, then this one is not finalized in the case of success.

libpurple/purplecontactmanager.c (Diff revision 4)
An issue was opened. Show all issues
```
Same here about moving outside loop.
```
libpurple/purplecontactmanager.c (Diff revision 4)
An issue was opened. Show all issues
```
Same here about finalizing on success.
```
libpurple/purplecontactmanager.c (Diff revision 4)
An issue was opened. Show all issues
```
Can move into the only if that uses it.
```
libpurple/purplecontactmanager.c (Diff revision 4)
An issue was opened. Show all issues
```
It's just gpointer.
```
libpurple/purpletags.c (Diff revision 4)
An issue was opened. Show all issues
```
Can be combined into g_clear_list.
```

		This is saved in a SQLite database inside the user config directory named
~		contacts.db. It does it's best to synchronize with PurpleBuddy, but not
	~	`contacts.db`. It does its best to synchronize with `PurpleBuddy`, but not
		everything emits a signal when it changes.

~		Also, stuff like custom avatars are
~		ignore as those are currently store on the Contact and we don't have a mapping
	~	Also, stuff like custom avatars are ignored as those are currently store
	~	on the Contact and we don't have a mapping to `PurplePerson` yet.
-		to PurplePerson yet.