Status

--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: clokep, Assigned: florian)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1.2-wanted])

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
*** Original post on bio 759 at 2011-04-19 13:36:00 UTC ***

This was discussed on IRC so I figured I'd file a bug about it before we forget.

purple/purplexpcom/src/ contains kind of a mess of files that could use some clean up, we have protocols + override protocols lumped in with the Mac Dock code, along with the purple xpcom interface classes. flo expressed some interest in cleaning this up, but I'm not sure exactly the right way to go with it.

I'm guessing we'd still want

purple/purplexpcom/src/ to house the interface source files
purple/purplexpcom/public/ to house the idl files

Maybe a:
purple/purplexpcom/protocols/ to house twitter.* and *OverrideProtocol.* and jsTestProtocol.*

I'm not sure where imStatusUtils.jsm, jsProtoHelper.jsm, socket.jsm (and possibly a doXHRequest.jsm from bug 954192 (bio 758)) would go, or if they're fine here.

I feel like nsDockTile.* is just in the total wrong place and shoule be under instantbird/components/ (this is also from WebRunner/Prism it seems...and if I end up ripping out MinTrayR to replace it with the new WebRunner code, it's possible this could all go in together).

Sorry for the rambling bug report, but let me know if we can make a decision and I'll go ahead and create a patch.
*** Original post on bio 759 at 2011-04-21 16:10:35 UTC ***

Historically (version 0.1.*), Instantbird was a XUL front-end above libpurple. There were 2 directories:
* instantbird/ for the XUL UI
* purple/ for libpurple, its dependencies and the XPCOM bridge to use it from the UI.
The goal of the separation was that the content of purple/ could be reusable by any Mozilla application wanting to use libpurple.
instantbird/ contained only things that could only be used for Instantbird.

Then, with Instantbird 0.2, we added some stuff that is completely unrelated to libpurple, but may be relevant to reuse for another Mozilla IM application. I'm thinking specifically about the message filtering, the message themes and emoticons themes code (that code is in instantbird/{components,modules}/).

With the ongoing Instantbird 0.3 development, the amount of code that could be reused by any Mozilla based IM application but is completely unrelated to libpurple significantly increased, especially because of the js-proto effort.

To reorganize all the files in a structure that makes sense, we would probably need to make the code go in at least 3 buckets instead of the 2 current ones:
- one for Instantbird specific code
- one for the code of a Mozilla based IM application
- one for the usage of libpurple.
The separation between these last 2 things can't be perfect yet as the application still relies on purpleCoreService for a few things.

This separation is still somehow present in the code as we have 2 different prefix in file names: ib, im and purple. I'm not sure they have always be used wisely however :-/.


Things that from my point of view are currently out-of-place:
 - lots of modules in instantbird/modules/
 - some components in instantbird/components/ (especially the logger and the smileProtocolHandler)
 - all the js-proto stuff in purple/purplexpcom/src/
 - I agree that the Mac Dock badge and the Windows/Linux systray implementations should be closer.
Whiteboard: [1.2-wanted]
Created attachment 8352686 [details]
List of files currently in instantbird/ and purple/

*** Original post on bio 759 as attmnt 944 at 2011-10-26 14:33:00 UTC ***

I've made a list of all the files we currently have in our repository in instantbird/ and purple/. When many files in the same folder have no reason to move, I haven't shown them (I've just put instead: "* (NN files)") to make the list appear shorter.
(Reporter)

Comment 3

5 years ago
*** Original post on bio 759 at 2011-10-26 15:29:12 UTC ***

(In reply to comment #1)
> This separation is still somehow present in the code as we have 2 different
> prefix in file names: ib, im and purple. I'm not sure they have always be used
> wisely however :-/.
For the most part I think we can rename these without harm.  We need to be careful with some of the modules, however, as they might be used in extensions. (We could change the name, keep the old one and throw a warning and re-export everything though?)

> Things that from my point of view are currently out-of-place:
>  - lots of modules in instantbird/modules/
>  - some components in instantbird/components/ (especially the logger and the
> smileProtocolHandler)
>  - all the js-proto stuff in purple/purplexpcom/src/
>  - I agree that the Mac Dock badge and the Windows/Linux systray
> implementations should be closer.
I agree with these, the js-proto stuff probably needs to be reorganized better into a protocols type directory structure too. (Assuming we'll add more.)
*** Original post on bio 759 at 2011-10-26 22:37:19 UTC ***

New directory layout proposal:

chat/core/components/public/
chat/core/components/src/
chat/core/modules/
chat/content/
  * put the generic xbl bindings here, browserRequest (and maybe the xul/js files for some windows Thunderbird may share like the Accounts manager? Duplicating the xul/js files and sharing the xbl may be cleaner.)
chat/themes/
  * css files and icons related to the xul/xbl files we include in chat/content
  * icons for the generic and unknown protocol plugins.
chat/locales/en-US/
chat/protocols/
chat/protocols/{jsTest, twitter, irc, xmpp, ...)/
  * each protocol should include its icons
(Reporter)

Comment 5

5 years ago
*** Original post on bio 759 at 2011-10-26 22:50:04 UTC ***

(In reply to comment #4)
> New directory layout proposal:
> 
> chat/core/components/public/
> chat/core/components/src/
> chat/core/modules/
> chat/content/
>   * put the generic xbl bindings here, browserRequest (and maybe the xul/js
> files for some windows Thunderbird may share like the Accounts manager?
> Duplicating the xul/js files and sharing the xbl may be cleaner.)
> chat/themes/
>   * css files and icons related to the xul/xbl files we include in chat/content
>   * icons for the generic and unknown protocol plugins.
> chat/locales/en-US/
> chat/protocols/
> chat/protocols/{jsTest, twitter, irc, xmpp, ...)/
>   * each protocol should include its icons

Would each protocol also include it's .properties files?  I think they should. Maybe something like:
chat/protocols/twitter/{<external library if there is one, e.g. meanwhile>, icons, locales, src, doc, test}

But maybe that's over kill, most things can probably just go in the main folder (except libraries should probably be kept separate).
*** Original post on bio 759 at 2011-10-26 23:24:13 UTC ***

(In reply to comment #5)

> Would each protocol also include it's .properties files?

We would need a Makefile and jar.mn file for each of theses "locales" folder.
Would we also want to create these new protocol folders for each protocol in the l10n repositories? I don't really think so.

> Maybe something like:
> chat/protocols/twitter/{<external library if there is one, e.g. meanwhile>,
> icons, locales, src, doc, test}

I thought the icons would go in a themes subfolder, but if the only thing we ever put there is the icons, "icons" sounds good.

I'm not sure an "src" folder would be of any help. We thought we would put the code directly in the protocol's folder.

doc is ok (I'm assuming it's just a place to store some text files that we like to keep close to the implementation, but that nothing is done with that folder during the build process.)

tests would be lovely, if we have something to put inside it :).

> But maybe that's over kill, most things can probably just go in the main folder
> (except libraries should probably be kept separate).

Yeah, I would prefer that we "start" with something simple, rather than adding complexity that isn't needed now. :)
(Reporter)

Comment 7

5 years ago
*** Original post on bio 759 at 2011-10-26 23:33:29 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> > Would each protocol also include it's .properties files?
> 
> We would need a Makefile and jar.mn file for each of theses "locales" folder.
> Would we also want to create these new protocol folders for each protocol in
> the l10n repositories? I don't really think so.
Ah, I didn't think of that.

> I thought the icons would go in a themes subfolder, but if the only thing we
> ever put there is the icons, "icons" sounds good.
I don't know if there would be anything else to go in there.

> I'm not sure an "src" folder would be of any help. We thought we would put the
> code directly in the protocol's folder.
That sounds good to me too!

> doc is ok (I'm assuming it's just a place to store some text files that we like
> to keep close to the implementation, but that nothing is done with that folder
> during the build process.)
For IRC I have a pretty big folder text/html files.  I ended up downloading them cause they can be hard to find again / websites go offline, etc.

Sounds like a plan! Let's do it.
*** Original post on bio 759 at 2011-10-26 23:46:04 UTC ***

(In reply to comment #7)

> > I'm not sure an "src" folder would be of any help. We thought we would put the
> > code directly in the protocol's folder.
> That sounds good to me too!

Opps, I meant "I thought" of course.

> For IRC I have a pretty big folder text/html files.  I ended up downloading
> them cause they can be hard to find again / websites go offline, etc.

Be careful with the licenses ;).
 
> Sounds like a plan! Let's do it.

How do we start? :)
Just create the folders we have discussed and move there the files that obviously should be there, and figure out the problematic files later?
Or work in a branch which we would merge only once we are done moving stuff around and making libpurple optional?
*** Original post on bio 759 at 2011-11-08 00:25:41 UTC ***

The first part of the work done here has been pushed as:
https://hg.instantbird.org/instantbird/rev/351bf781388d - create a chat/ folder to contain the core code that isn't related to libpurple
Created attachment 8352718 [details] [diff] [review]
Part 2 (v1)

*** Original post on bio 759 as attmnt 976 at 2011-11-08 00:28:00 UTC ***

- Rewrite the core service (including the list of protocols), the account list and the handling of user status (buddy icon, idle, offline, ...) in JS components that don't depend on libpurple at all.
- Load libpurple lazily if we need a protocol plugin that depends on it.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Created attachment 8352719 [details] [diff] [review]
Part 2 - additional change to libpurple plugin registration

*** Original post on bio 759 as attmnt 977 at 2011-11-08 11:53:00 UTC ***

With this additional change, libpurple plugins packaged as add-on will automatically register in the new im-protocol-plugin category.
Created attachment 8352721 [details] [diff] [review]
Part 2 - additional changes found during self-review

*** Original post on bio 759 as attmnt 980 at 2011-11-08 23:59:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 759 at 2011-11-09 00:21:30 UTC ***

Pushed attachment 8352718 [details] [diff] [review] (bio-attmnt 976) (+ 977 and 980) as https://hg.instantbird.org/instantbird/rev/48709551a8bc

Updated

5 years ago
Depends on: 954600

Updated

5 years ago
Depends on: 954596
*** Original post on bio 759 at 2011-11-15 19:00:39 UTC ***

Follow-ups:

* Pushed on Wed Nov 09:
https://hg.instantbird.org/instantbird/rev/3ee46e975997 - fix bustage on cloberred builds.
https://hg.instantbird.org/instantbird/rev/40d3eaad9fe6 - fix twitter conversations
https://hg.instantbird.org/instantbird/rev/c06176f81f71 - fix packaging of new components
https://hg.instantbird.org/instantbird/rev/251e0a4186b0 - process auto-login at startup
https://hg.instantbird.org/instantbird/rev/966657ce46fd - avoid harmless JS error while starting a profile with an empty account list
https://hg.instantbird.org/instantbird/rev/2db854d92232 - fix twitter conversations for real this time

* Just pushed:
https://hg.instantbird.org/instantbird/rev/36cd75e06c0a - fix selection of auto-reconnect timer times
https://hg.instantbird.org/instantbird/rev/b4d86b27b180 - bug 954600 (bio 1168)
https://hg.instantbird.org/instantbird/rev/8cdbef751c1d - fix saving of account specific proxy changes
https://hg.instantbird.org/instantbird/rev/0868366b02e6 - fix the parameters of the example getAccount method in jsProtoHelper
https://hg.instantbird.org/instantbird/rev/7f6548d61894 - fix selection of new account ids (this was a dataloss bug!)
https://hg.instantbird.org/instantbird/rev/051d48073c9a - bug 954596 (bio 1164)
Depends on: 954590
Created attachment 8352808 [details] [diff] [review]
Part 3 - Mass purpleI* -> prplI* rename

*** Original post on bio 759 as attmnt 1066 at 2011-12-07 15:36:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 759 at 2011-12-08 10:18:41 UTC ***

Comment on attachment 8352808 [details] [diff] [review] (bio-attmnt 1066)
Part 3 - Mass purpleI* -> prplI* rename

Pushed as https://hg.instantbird.org/instantbird/rev/886c14911cc2

Left to do:

- find an acceptable solution for the purpleIProxy* interfaces

- move the localizable files for chat/ stuff from chrome://purple/ to chrome://chat/
(Reporter)

Comment 17

5 years ago
Created attachment 8352814 [details] [diff] [review]
Move imIStatusInfo and imITagsService/imITag to separate files

*** Original post on bio 759 as attmnt 1072 at 2011-12-11 22:31:00 UTC ***

As discussed on IRC, this moves imIStatusInfo to an imIStatusInfo.idl file and imITagsService/imITag into an imITagsService.idl file. Please be gentle if this is wrong, I don't touch idls often. :)
Attachment #8352814 - Flags: review?(florian)
(Reporter)

Comment 18

5 years ago
Created attachment 8352816 [details] [diff] [review]
Move imIStatusInfo and imITagsService/imITag to separate files v2

*** Original post on bio 759 as attmnt 1074 at 2011-12-11 23:50:00 UTC ***

I apparently messed up building when I tested my patch, here's an updated version that actually builds + review comments on IRC from flo.
(Reporter)

Comment 19

5 years ago
Comment on attachment 8352814 [details] [diff] [review]
Move imIStatusInfo and imITagsService/imITag to separate files

*** Original change on bio 759 attmnt 1072 at 2011-12-11 23:50:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352814 - Attachment is obsolete: true
Attachment #8352814 - Flags: review?(florian)
(Reporter)

Comment 20

5 years ago
Comment on attachment 8352816 [details] [diff] [review]
Move imIStatusInfo and imITagsService/imITag to separate files v2

*** Original change on bio 759 attmnt 1074 at 2011-12-11 23:55:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352816 - Flags: review?(florian)
Comment on attachment 8352816 [details] [diff] [review]
Move imIStatusInfo and imITagsService/imITag to separate files v2

*** Original change on bio 759 attmnt 1074 at 2011-12-12 00:40:32 UTC ***

Looks good, thanks!
Attachment #8352816 - Flags: review?(florian) → review+
(Reporter)

Comment 22

5 years ago
*** Original post on bio 759 at 2011-12-12 01:14:22 UTC ***

(In reply to comment #19)
> Comment on attachment 8352816 [details] [diff] [review] (bio-attmnt 1074) [details]
> Move imIStatusInfo and imITagsService/imITag to separate files v2
> 
> Looks good, thanks!

Checked in as http://hg.instantbird.org/instantbird/rev/6c9fd7291335
Created attachment 8352826 [details] [diff] [review]
Handle connection info changes in imAccounts.js instead of the UI

*** Original post on bio 759 as attmnt 1084 at 2011-12-15 17:01:00 UTC ***

This has no reason to be handled in the UI and it would really annoy me to have to duplicate this code in the Thunderbird UI.

Notes:
- I haven't really tested this (just checked that there's no parse error in the changes).
- I suspect this doesn't work correctly (both before and after the patch) for the case where the password was missing and the user added it.
Attachment #8352826 - Flags: review?(clokep)
(Reporter)

Comment 24

5 years ago
Comment on attachment 8352826 [details] [diff] [review]
Handle connection info changes in imAccounts.js instead of the UI

*** Original change on bio 759 attmnt 1084 at 2011-12-16 01:37:18 UTC ***

This looks fine and was briefly tested and works.
Attachment #8352826 - Flags: review?(clokep) → review+
*** Original post on bio 759 at 2011-12-19 11:54:29 UTC ***

Comment on attachment 8352826 [details] [diff] [review] (bio-attmnt 1084)
Handle connection info changes in imAccounts.js instead of the UI

https://hg.instantbird.org/instantbird/rev/b26956bb43d6

Also pushed today:
https://hg.instantbird.org/instantbird/rev/88fda421b08e - fix jsTest for compatibility with the changes made here.
*** Original post on bio 759 at 2011-12-23 16:33:53 UTC ***

(In reply to comment #16)

> Left to do:

> - move the localizable files for chat/ stuff from chrome://purple/ to
> chrome://chat/

Done: https://hg.instantbird.org/instantbird/rev/788a90ffd9d7

I also moved the theme files so that chat/ doesn't depend on instantbird/themes: https://hg.instantbird.org/instantbird/rev/c663d78550e9

Left to do:
- find a solution for the purpleProxy* thing
- rewrite the code opening the authorization dialogs when someone added the user in his contact list. The current code is in C++ in http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/purpleInitAccounts.cpp#210
We need to rewrite it anyway so that JS prpls can use it too, so I didn't bother with moving the current strings used for it from the instantbird.properties file.
- find a better packaging solution for the default emoticon and message themes.

I think these 3 points would better be handled in other bugs though :).
(Reporter)

Comment 27

5 years ago
*** Original post on bio 759 at 2011-12-23 18:33:18 UTC ***

(In reply to comment #24)
> Left to do:
> - find a solution for the purpleProxy* thing
bug 953602 (bio 155) perhaps?

> - rewrite the code opening the authorization dialogs when someone added the
> user in his contact list. The current code is in C++ in
> http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/purpleInitAccounts.cpp#210
> We need to rewrite it anyway so that JS prpls can use it too, so I didn't
> bother with moving the current strings used for it from the
> instantbird.properties file.
> - find a better packaging solution for the default emoticon and message themes.
> 
> I think these 3 points would better be handled in other bugs though :).

I agree, I'm closing this.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 954714
You need to log in before you can comment on or make changes to this bug.