Closed Bug 953628 Opened 10 years ago Closed 10 years ago

make account reordering possible (shift+up/down arrow, drag&drop, context menu)

Categories

(Instantbird Graveyard :: Account manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: romain)

References

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 181 at 2009-04-24 20:52:00 UTC ***

It should be possible to reorder accounts. The way to store this is already implemented at the account list is displayed in the order of this preference:
messenger.accounts

For the UI, using keyboard (shift+up/down arrow), drag and drop and context menu entries would be nice.

This is currently a minor improvement that will allow users to see the accounts that they really care about first in the list when opening the window, but for a later release (0.3 most likely), it will be really useful to set preferences between accounts to select which account to use to start a conversation with a grouped contact.
Blocks: 953623
*** Original post on bio 181 at 2009-08-02 22:28:33 UTC ***

Mass change of target milestone from 0.2a1 to 0.2 for bugs that we haven't fixed for alpha 1 but still think we are likely to fix before 0.2 final.
Target Milestone: 0.2a1 → 0.2
Depends on: 953626
Attached patch proposed patch (obsolete) — Splinter Review
*** Original post on bio 181 as attmnt 207 at 2009-08-23 01:15:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351951 - Flags: review?(florian)
Assignee: nobody → romain
Status: NEW → ASSIGNED
Attached patch proposed patch V2 (obsolete) — Splinter Review
*** Original post on bio 181 as attmnt 208 at 2009-08-23 01:38:00 UTC ***

Sorry I forgot to test when the list was empty :(.
I knew I would forgot it...
Fixed a few errors and preventing some new to appear in the whole keypress handler (null checks).
Attachment #8351952 - Flags: review?(florian)
Comment on attachment 8351951 [details] [diff] [review]
proposed patch

*** Original change on bio 181 attmnt 207 at 2009-08-23 01:38:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351951 - Attachment is obsolete: true
Attachment #8351951 - Flags: review?(florian)
Attached patch Patch V3 (obsolete) — Splinter Review
*** Original post on bio 181 as attmnt 222 at 2009-08-27 14:37:00 UTC ***

- Fit the code to the last Mercurial change set
- Fixing colors to fit the system's defaults
- Changing the size of this border to 3px (down from 5px)
- Fixing a bug that occurred when you were dragging an account on itself
- Adding some comments
- Cursor icon changes depending on what item is hovered
- prevent from dragging the whole account manager with a throw (which is catched in the mozilla's nsDragAndDrop.js)
- Fixing the DND behavior when there is only one account
- Fixing the border behavior which prevented the default border to be restored
- Fixing a bug that occurred when you were dropping an account on the first position, and right click to show up a menu (Move Up was enabled).
- C++: now removes the preference observer on quit
Attachment #8351966 - Flags: review?(florian)
Comment on attachment 8351952 [details] [diff] [review]
proposed patch V2

*** Original change on bio 181 attmnt 208 at 2009-08-27 14:37:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351952 - Attachment is obsolete: true
Attachment #8351952 - Flags: review?(florian)
Comment on attachment 8351966 [details] [diff] [review]
Patch V3

*** Original change on bio 181 attmnt 222 at 2009-08-27 17:02:28 UTC ***

The change of InitAccounts into UpdateAccountList in purplexpcom is not acceptable as it. It makes loading of the account list at startup O(N^2). And even in the case of updating the list, there's no real reason for an O(N^2) algorithm.
I think you should keep the InitAccounts method as it is, and add another method to refresh the list of accounts. And the code will be more readable that way.

I also requested several small changes in the front-end part directly by private IM while I was reading the patch and we were discussing it.

Looks good otherwise. A lot of good work here! Thanks.
Attachment #8351966 - Flags: review?(florian) → review-
Attached patch Patch V4 (obsolete) — Splinter Review
*** Original post on bio 181 as attmnt 223 at 2009-08-27 18:54:00 UTC ***

Fixing the things discussed above.
Attachment #8351967 - Flags: review?(florian)
Comment on attachment 8351966 [details] [diff] [review]
Patch V3

*** Original change on bio 181 attmnt 222 at 2009-08-27 18:54:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351966 - Attachment is obsolete: true
Comment on attachment 8351967 [details] [diff] [review]
Patch V4

*** Original change on bio 181 attmnt 223 at 2009-08-27 19:22:54 UTC ***

>diff --git a/instantbird/base/content/instantbird/accounts.js b/instantbird/base/content/instantbird/accounts.js

>+  onDrop : function amdnd_onDrop(aEvent, aTransferData, aSession) {

>+  getSupportedFlavours : function amdnd_getSupportedFlavours() {

Coding style nit: no space before ':'.


>diff --git a/purple/purplexpcom/src/purpleCoreService.cpp b/purple/purplexpcom/src/purpleCoreService.cpp

> 
>   /* Set to true before the end of the initialization because
>-     InitAccounts calls GetProtoById that ensures the core is
>+     UpdateAccountList calls GetProtoById that ensures the core is
>      initialized */
>   mInitialized = PR_TRUE;
I think you wanted to revert that change.

>+nsresult purpleCoreService::UpdateAccountList()
>+{

>+  nsCOMPtr<purpleAccount> account;

>+          NS_ADDREF(account = mAccounts[j]);
>+          mAccounts.RemoveObject(account);
>+          mAccounts.InsertObjectAt(account, i);
>+          NS_RELEASE(account);
When you assign something into an nsCOMPtr, it automatically addrefs. And it will automatically release too when you assign something else into the same variable, or when leaving the scope. So, you don't need these NS_ADDREF and NS_RELEASE.

r=me with these fixed!
Attachment #8351967 - Flags: review?(florian) → review+
Attached patch Patch V5 (obsolete) — Splinter Review
*** Original post on bio 181 as attmnt 224 at 2009-08-27 20:06:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351968 - Flags: review?(florian)
Attached patch Patch V6 (!!)Splinter Review
*** Original post on bio 181 as attmnt 225 at 2009-08-27 20:17:00 UTC ***

Oooooooops, there was an affectation inside the NS_ADDREF...
Attachment #8351969 - Flags: review?(florian)
Comment on attachment 8351967 [details] [diff] [review]
Patch V4

*** Original change on bio 181 attmnt 223 at 2009-08-27 20:17:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351967 - Attachment is obsolete: true
Comment on attachment 8351968 [details] [diff] [review]
Patch V5

*** Original change on bio 181 attmnt 224 at 2009-08-27 20:17:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351968 - Attachment is obsolete: true
Attachment #8351968 - Flags: review?(florian)
Comment on attachment 8351969 [details] [diff] [review]
Patch V6 (!!)

*** Original change on bio 181 attmnt 225 at 2009-08-27 20:25:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351969 - Flags: review?(florian) → review+
*** Original post on bio 181 at 2009-08-27 21:05:40 UTC ***

pushed as:
http://hg.instantbird.org/instantbird/rev/fa203d705e44
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 0.2 → 0.2b1
Depends on: 953708
You need to log in before you can comment on or make changes to this bug.