954075 - Split purpleInit.cpp into multiple files

Status: RESOLVED FIXED

(Chat Core :: General, enhancement)

Description

[Instantbot]

*** Original post on bio 638 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2010-12-21 23:49:00 UTC ***

purpleInit.cpp is currently a very large file with 1291 lines; it would be nice to split it up into a few separate files to make it easier to understand.

Current thinking is
 - conversation related stuff
 - buddy/contact list stuff
 - account/connection stuff
 - remaining init things stay in purpleInit.cpp

Marking as a blocker for bug 954064 (bio 628) because that involves a new type of things to init.  It probably technically isn't blocking, though... it's just why I wanted to do this :)

Comment 1

[Instantbot]

Attachment: move the conversation stuff out

*** Original post on bio 638 as attmnt 440 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-22 00:51:00 UTC ***

This seemed to be the easiest of the various things to move, so I figured I might as well use this as a proof of concept.

I've tried to minimize the changes for easier review - there are things I'd want to change, but they're not part of this bug :)

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 638 at 2010-12-22 23:34:46 UTC ***

Comment on attachment 8352184 [details] [diff] [review] (bio-attmnt 440)
move the conversation stuff out

I don't think it's worth splitting that ugly file if the moved out stuff still ends up being called from purpleInit.

What would you think of creating an  init_libpurple_conversations function in purpleInitConv.cpp that would contain what you have now in purplexpcom_init_connect_to_conversations_signals and purple_conversations_set_ui_ops(&conversation_uiops); ?

That function would be called from purpleCoreService just after init_libpurple.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8352184 [details] [diff] [review]
move the conversation stuff out

*** Original change on bio 638 attmnt 440 at 2010-12-22 23:48:25 UTC ***

r-, per comment 2 and the following discussion on IRC.

Comment 4

[Instantbot]

Attachment: move into one function called by purpleCoreService::Init

*** Original post on bio 638 as attmnt 441 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-23 05:12:00 UTC ***

As suggested.

This only works, I think, because the conversation ui_ops are only used when creating a new conversation (and therefore way after initialization).  This might not work for some of the other things, in which case they might require two calls instead of one.  But we can worry about that when we get there, and it shouldn't be difficult.

Comment 5

[Instantbot]

Comment on attachment 8352184 [details] [diff] [review]
move the conversation stuff out

*** Original change on bio 638 attmnt 440 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-23 05:12:13 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Instantbot]

*** Original post on bio 638 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2010-12-23 06:52:23 UTC ***

Comment on attachment 8352185 [details] [diff] [review] (bio-attmnt 441)
move into one function called by purpleCoreService::Init

Please pretend I was smart enough to remove
extern PurpleConversationUiOps conversation_uiops;
in purpleInit.cpp, thanks? :)

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8352185 [details] [diff] [review]
move into one function called by purpleCoreService::Init

*** Original change on bio 638 attmnt 441 at 2010-12-23 11:58:24 UTC ***

Looks good. I just pushed this as https://hg.instantbird.org/instantbird/rev/bdb42edd2287 with the change mentioned in comment 5 and a few additional #include removals that we discussed on IRC.

Comment 8

[Instantbot]

Attachment: part 2: accounts and connection

*** Original post on bio 638 as attmnt 447 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-24 07:49:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Instantbot]

Attachment: part 3: buddies and contacts

*** Original post on bio 638 as attmnt 448 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-24 07:51:00 UTC ***

These were only tested on linux (I think my windows harddisk is complaining...).  I hope I'm not overwhelming you...

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8352191 [details] [diff] [review]
part 3: buddies and contacts

*** Original change on bio 638 attmnt 448 at 2010-12-24 15:54:05 UTC ***

I don't think libpurple commands should be moved into a file named purpleInitContacts (and as we should rewrite them completely for compatibility with our current js-proto effort, I don't think moving them into their own file would be a valuable use of your time).

Additionnally, it seems there are possible simplifications by merging the code of your libpurple_init_contacts function and of the InitBuddyList method of purpleCoreService.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8352190 [details] [diff] [review]
part 2: accounts and connection

*** Original change on bio 638 attmnt 447 at 2010-12-24 15:57:23 UTC ***

Looks good. I just pushed this as https://hg.instantbird.org/instantbird/rev/c0df270a2f48 with the additional removal of the unused "#define CUSTOM_PLUGIN_PATH     "protocols"" line in both purpleInit.cpp and the new purpleInitAccounts.cpp files.

Comment 12

[Instantbot]

*** Original post on bio 638 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2010-12-25 02:47:03 UTC ***

(In reply to comment #9)

Notes for myself, regarding order things have to happen in:
  purple_set_blist(purple_blist_new());
needs to happen before
  rv = mAccountsService->InitAccounts();
in order to set up
  buddies_cache
(that's just the first thing that asserted, anyway)

export G_DEBUG=fatal_criticals is useful for seeing these things in a debugger.

Comment 13

[Instantbot]

Attachment: merge into ::InitContacts

*** Original post on bio 638 as attmnt 449 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-25 08:28:00 UTC ***

See comment 11 on why the blist init is there and ugly :(  And I need the contacts to be ready before loading things into it...

Comment 14

[Instantbot]

Comment on attachment 8352191 [details] [diff] [review]
part 3: buddies and contacts

*** Original change on bio 638 attmnt 448 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-25 08:28:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Florian Quèze [:florian]]

*** Original post on bio 638 at 2010-12-25 09:21:21 UTC ***

I'm confused by comment 11 because I see neither how mAccountsService->InitAccounts affects the buddy list, nor how attachment 8352192 [details] [diff] [review] (bio-attmnt 449) makes the call to InitBuddyList before mAccountsService->InitAccounts.

Comment 16

[Instantbot]

Attachment: backtrace of GLIB-Critical error

*** Original post on bio 638 as attmnt 450 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-25 09:46:00 UTC ***

(In reply to comment #13)
> I'm confused by comment 11 because I see neither how
> mAccountsService->InitAccounts affects the buddy list, nor how attachment 8352192 [details] [diff] [review] (bio-attmnt 449) 
> makes the call to InitBuddyList before mAccountsService->InitAccounts.
Attachment 8352192 [details] [diff] (bio-attmnt 449) just didn't remove the purple_set_blist(purple_blist_new()); from init_libpurple().  ( http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/purpleInit.cpp#840 )

Sorry, I should have included the backtrace... see attachment.  (I cheated by just commenting out the call instead of moving it, but it's the same.)

So the actual problem was that http://lxr.instantbird.org/instantbird/source/purple/libpurple/blist.c#3231 was hooking up an account-created signal handler, before blist setup.  Which happens due to purple_core_init, and only worked before because accounts had no chance of being created before the call to purple_set_blist.

Still thinking; maybe there's a better answer here...

Comment 17

[Florian Quèze [:florian]]

Attachment: purpleInitContacts - another proposal

*** Original post on bio 638 as attmnt 456 at 2010-12-27 00:03:00 UTC ***

I'm not really satisfied with attachment 8352192 [details] [diff] [review] (bio-attmnt 449). Here is another proposal (I'll attach an interdiff so that you can clearly see the changes).

(In reply to comment #9)

> Additionnally, it seems there are possible simplifications by merging the code
> of your libpurple_init_contacts function and of the InitBuddyList method of
> purpleCoreService.

This was very confusing, sorry. I didn't want the code touching imIContactsService to be moved outside of purpleCoreService (and neither a method or purpleCoreService to be implemented outside of purpleCoreService.cpp). I was just wondering if you could move the purple_blist_load(); and connect_to_blist_signals(); lines to the libpurple related code.

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8352192 [details] [diff] [review]
merge into ::InitContacts

*** Original change on bio 638 attmnt 449 at 2010-12-27 00:03:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Florian Quèze [:florian]]

Attachment: interdiff showing the changes in the previous proposal

*** Original post on bio 638 as attmnt 457 at 2010-12-27 00:05:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 20

[Instantbot]

Comment on attachment 8352199 [details] [diff] [review]
purpleInitContacts - another proposal

*** Original change on bio 638 attmnt 456 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-27 20:38:44 UTC ***

yes, getting somebody who knows the code to do things works better :)  (All I cared was that the file got smaller overall, so this is great for me)

Comment 21

[Florian Quèze [:florian]]

*** Original post on bio 638 at 2011-01-06 16:25:24 UTC ***

Comment on attachment 8352199 [details] [diff] [review] (bio-attmnt 456)
purpleInitContacts - another proposal

Pushed this as https://hg.instantbird.org/instantbird/rev/ba17c12c2206

Leaving the bug open as the code related to the handling of libpurple preferences can still be moved out of purpleInit.cpp