Summary

Modernize the AccountEditor.

Review Request #1737 — Created Sept. 11, 2022 and submitted Sept. 16, 2022, 3:32 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17592

Depends On

Reviewers

Groups

pidgin

People

Description

This now uses libadwaita for everything, but will need some updates when
libadwaita 1.2 is released. Avatars are currently non-functional as the existing
buddy icon API leaves a lot to be desired and doesn't work for what we're trying
to do here.

Testing Done


Created new accounts with the new dialog.
Modified accounts with the new dialog.
Modified accounts with the old dialog and verified the changes showed up in the old dialog.
Created accounts with the old dialog and verified they were displayed properly by the new dialog.

Commits

Summary	ID
Modernize the AccountEditor. This now uses libadwaita for everything, but will need some updates when libadwaita 1.2 is released. Avatars are currently non-functional as the existing buddy icon API leaves a lot to be desired and doesn't work for what we're trying to do here.	3f4df179ae1956eebdfe6df3c507a1453f570d86

Issues

Description	From	Last Updated
Are these not cleared in finalize?	QuLogic	Sept. 16, 2022, 2:39 a.m.
Also not cleared in finalize?	QuLogic	Sept. 16, 2022, 2:39 a.m.
In this block, you add something to user_split_rows, but further down you've added something unconditionally to user_split_entries, so these two …	QuLogic	Sept. 16, 2022, 2:39 a.m.
NULL is a valid (empty) list, so the check is not necessary, though fine to keep.	QuLogic	Sept. 16, 2022, 2:40 a.m.
its	QuLogic	Sept. 16, 2022, 2:40 a.m.
This is repeated in every advanced widget creation function; maybe should go in pidgin_account_editor_add_advanced_option?	QuLogic	Sept. 16, 2022, 2:40 a.m.
FALSE?	QuLogic	Sept. 16, 2022, 2:41 a.m.
its	QuLogic	Sept. 16, 2022, 2:41 a.m.
Do you need to set this to be digits-only somehow? Or should it be a GtkSpinButton?	QuLogic	Sept. 16, 2022, 2:41 a.m.
FALSE?	QuLogic	Sept. 16, 2022, 2:42 a.m.
Don't need the trailing newline.	QuLogic	Sept. 16, 2022, 2:42 a.m.
Related to the leak question below, if you fix the leak, then will this string still exist through the dialog's …	QuLogic	Sept. 16, 2022, 2:50 a.m.
Doesn't this leak?	QuLogic	Sept. 16, 2022, 2:50 a.m.
Weird alignment. Also don't need G_OBJECT().	QuLogic	Sept. 16, 2022, 2:43 a.m.
Should this clear avatar_pixbuf as well?	QuLogic	Sept. 16, 2022, 2:43 a.m.
I wonder if you can add this to the top widget only, as it should be inherited, I think?	QuLogic	Sept. 16, 2022, 2:45 a.m.
You can inline this in the property, as it seems like proxy_port_adjustment is not used elsewhere.	QuLogic	Sept. 16, 2022, 2:44 a.m.

Change Summary:: Add bugs
Bugs:: PIDGIN-17592

pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Are these not cleared in finalize?
```
pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Also not cleared in finalize?
```

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been dropped. Show all issues

In this block, you add something to user_split_rows, but further down you've added something unconditionally to user_split_entries, so these two lists are not exactly in sync.

I don't see anything in the other iterating steps (update/save) that checks purple_account_user_split_is_constant, or for NULL to handle this out-of-sync-ness?

grim Sept. 14, 2022, 5:50 a.m.

They're not supposed to be n*sync. user_split_rows is used to remove children from the login_options preference group so we don't have to blindly remove children.

user_split_entries is synced to the user_splits of the protocol so that was can properly create the username in the save function.

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been resolved. Show all issues

NULL is a valid (empty) list, so the check is not necessary, though fine to keep.

pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
its
```

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been dropped. Show all issues

This is repeated in every advanced widget creation function; maybe should go in pidgin_account_editor_add_advanced_option?

grim Sept. 14, 2022, 6:01 a.m.

We'd to use a return address or something to do this as right now we're returning the editor which is only the row for the dropdown.

QuLogic Sept. 16, 2022, 2:40 a.m.

Ah yes, missed that they weren't all returning the row.

pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
FALSE?
```
pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
its
```

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been resolved. Show all issues

Do you need to set this to be digits-only somehow? Or should it be a GtkSpinButton?

QuLogic Sept. 13, 2022, 3:30 a.m.

In save, this is treated as a GtkSpinButton, so this must be wrong entirely?

pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
FALSE?
```
pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Don't need the trailing newline.
```

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been resolved. Show all issues

Related to the leak question below, if you fix the leak, then will this string still exist through the dialog's existence?

Would it be better to put the PurpleAccountOption as the object data instead of its contents?

pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Doesn't this leak?
```
pidgin/pidginaccounteditor.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Weird alignment. Also don't need G_OBJECT().
```

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been resolved. Show all issues

Should this clear avatar_pixbuf as well?

pidgin/pidginaccounteditor.c (Diff revision 1)

The issue has been resolved. Show all issues

I wonder if you can add this to the top widget only, as it should be inherited, I think?

pidgin/resources/Accounts/editor.ui (Diff revision 1)

The issue has been resolved. Show all issues

You can inline this in the property, as it seems like proxy_port_adjustment is not used elsewhere.

Change Summary:

address all the things!!!

Commits:

	Summary	ID
	Modernize the AccountEditor. This now uses libadwaita for everything, but will need some updates when libadwaita 1.2 is released. Avatars are currently non-functional as the existing buddy icon API leaves a lot to be desired and doesn't work for what we're trying to do here.	3590de5ec7c44bee666bc6d826dc20d12cae11bf
	Modernize the AccountEditor. This now uses libadwaita for everything, but will need some updates when libadwaita 1.2 is released. Avatars are currently non-functional as the existing buddy icon API leaves a lot to be desired and doesn't work for what we're trying to do here.	3f4df179ae1956eebdfe6df3c507a1453f570d86

Diff:

Revision 2 (+2596 -94)

Show changes

	pidgin/pidginaccounteditor.c
	pidgin/resources/Accounts/editor.ui

Ship it!

```
Ship It!
```

Status:: Completed