Modernize the AccountEditor.

Review Request #1737 — Created Sept. 11, 2022 and submitted

grim
pidgin/pidgin
default
PIDGIN-17592
pidgin
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.
  • 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.
Summary
Modernize the AccountEditor.
Description From Last Updated

Are these not cleared in finalize?

QuLogicQuLogic

Also not cleared in finalize?

QuLogicQuLogic

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 …

QuLogicQuLogic

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

QuLogicQuLogic

its

QuLogicQuLogic

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

QuLogicQuLogic

FALSE?

QuLogicQuLogic

its

QuLogicQuLogic

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

QuLogicQuLogic

FALSE?

QuLogicQuLogic

Don't need the trailing newline.

QuLogicQuLogic

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

QuLogicQuLogic

Doesn't this leak?

QuLogicQuLogic

Weird alignment. Also don't need G_OBJECT().

QuLogicQuLogic

Should this clear avatar_pixbuf as well?

QuLogicQuLogic

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

QuLogicQuLogic

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

QuLogicQuLogic
QuLogic
QuLogic
  1. 
      
  2. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     

    Are these not cleared in finalize?

  3. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     

    Also not cleared in finalize?

  4. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     

    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?

    1. 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.

  5. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     
     

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

  6. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
  7. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     
     
     
     

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

    1. 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.

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

  8. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
  9. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
  10. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     

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

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

  11. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
  12. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     

    Don't need the trailing newline.

  13. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     

    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?

  14. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     

    Doesn't this leak?

  15. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     
     

    Weird alignment. Also don't need G_OBJECT().

  16. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     
     
     
     
     
     
     

    Should this clear avatar_pixbuf as well?

  17. pidgin/pidginaccounteditor.c (Diff revision 1)
     
     
     
     

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

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

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

  19. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...