Closed Bug 953626 Opened 10 years ago Closed 10 years ago

add a context menu to the account manager

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, 2 obsolete files)

*** Original post on bio 179 at 2009-04-24 20:45:00 UTC ***

A right click on an account should open a menu with all the possible actions related to the account.

This is also a nice thing for extensions that can then add stuff in that menu without adding more visual elements to the window.
Blocks: 953623
Blocks: 953627
*** Original post on bio 179 at 2009-08-02 22:28:30 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
Attached patch Patch V1 (obsolete) — Splinter Review
*** Original post on bio 179 as attmnt 184 at 2009-08-11 22:12:00 UTC ***

- Adding a read only property on account.xml binding to get the active button Element
- Disable double right click on an account
- showHideContextItems to show, hide and disable items depending on the context
- Fixing a small bug that prevented the activeButton from being disabled in case of autologin is enabled, no account has crashed and an account has an unknown protocol.
- Adding the context menu (Connect/Disconnect, Delete, Properties, New Account).
If the context menu is not on an account, then the only item displayed is "New Account"
Attachment #8351928 - Flags: review?(florian)
Assignee: nobody → romain
Status: NEW → ASSIGNED
Blocks: 953628
Comment on attachment 8351928 [details] [diff] [review]
Patch V1

*** Original change on bio 179 attmnt 184 at 2009-08-14 00:46:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351928 - Flags: review?(florian) → review-
*** Original post on bio 179 at 2009-08-14 00:46:18 UTC ***

Comment on attachment 8351928 [details] [diff] [review] (bio-attmnt 184)
Patch V1

>diff --git a/instantbird/base/content/instantbird/account.xml b/instantbird/base/content/instantbird/account.xml
>--- a/instantbird/base/content/instantbird/account.xml
>+++ b/instantbird/base/content/instantbird/account.xml
>@@ -393,25 +393,36 @@
>      <property name="account">
>        <getter>
>          <![CDATA[
>            return this._account;
>          ]]>
>        </getter>
>      </property>
> 
>+     <property name="activeButton">
>+       <getter>
>+         <![CDATA[
>+           let action = this._account.disconnected ? "connect" : "disconnect";
>+           return document.getAnonymousElementByAttribute(this, "anonid", action);
>+         ]]>
>+       </getter>
>+     </property>

Adding this seems like a good idea. I'm a bit surprised it's used by a single method though.

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

>+  showHideContextItems: function am_showHideContextItems() {
>+    let targetElt = document.popupNode;
>+    let isAccount = false;
What about doing
      let isAccount = targetElt instanceof Ci.nsIDOMXULSelectControlItemElement;

>+    let buttonIsConnect = false;
>+    if (targetElt instanceof Ci.nsIDOMXULSelectControlItemElement) {
if (isAccount)

>+      isAccount = true;
Now you can remove this.

>+      buttonIsConnect = targetElt.activeButton.getAttribute("anonid") == "connect";
If is strange. You execute twice the code of the activeButton property.
And... you end up doing something like getAnonymousElementByAttribute(this, "anonid", ...).getAttribute("anonid") !

>+
>+      // Here we disable the current selected menuitem if needed
>+      let buttonName = buttonIsConnect ? "connect" : "disconnect";
>+      document.getElementById("context-" + buttonName)
>+              .disabled = targetElt.activeButton.disabled

I think using <command> tags here, and having the code that disable the button disable the command would simplify the code here.

(Nit: missing ; )
>+    }
>+
>+    document.getElementById("context-connect").hidden =
>+      !isAccount || !buttonIsConnect;
>+    document.getElementById("context-disconnect").hidden =
>+      !isAccount || buttonIsConnect;
>+
>+    let separators = document.getElementsByClassName("context-account-separator");
>+    for (let i = 0; i < separators.length; ++i)
>+      separators[i].hidden = !isAccount;
>+    document.getElementById("context-delete").hidden = !isAccount;
>+    document.getElementById("context-edit").hidden = !isAccount;

You can probably simplify that by using a <broadcaster> element.
See https://developer.mozilla.org/en/XUL/broadcaster

>+  },
>+
>   selectAccount: function am_selectAccount(aAccountId) {
>     this.accountList.selectedItem = document.getElementById(aAccountId);
>     this.accountList.ensureSelectedElementIsVisible();
>   },
>   onAccountSelect: function am_onAccountSelect() {
>     // Horrible hack here too, see Bug 953624 (bio 177)
>     setTimeout(function(aThis) {
>       aThis.accountList.selectedItem.setButtonFocus();


>   setAutoLoginNotification: function am_setAutoLoginNotification() {
>     var pcs = Components.classes["@instantbird.org/purple/core;1"]
>                         .getService(Ci.purpleICoreService);
>     var autoLoginStatus = pcs.autoLoginStatus;
>+    let isOffline = false;
> 
>-    if (autoLoginStatus == pcs.AUTOLOGIN_ENABLED)
>+    if (autoLoginStatus == pcs.AUTOLOGIN_ENABLED) {
>+      this.setOffline(isOffline);
>       return;
>+    }
> 
This is needed, but unrelated to this bug. I'll check it in, but not in the same changeset.

>diff --git a/instantbird/base/content/instantbird/accounts.xul b/instantbird/base/content/instantbird/accounts.xul
>--- a/instantbird/base/content/instantbird/accounts.xul
>+++ b/instantbird/base/content/instantbird/accounts.xul
>@@ -63,19 +63,46 @@
>    <command id="cmd_close" oncommand="gAccountManager.close()"/>
>  </commandset>
> 
>  <keyset id="accountsKeys">
>    <key id="key_close1" key="w" modifiers="accel" command="cmd_close"/>
>    <key id="key_close2" keycode="VK_ESCAPE" command="cmd_close"/>
>  </keyset>
> 
>+<menupopup id="accountsContextMenu"
Indentation is wrong here.

>+           onpopupshowing="gAccountManager.showHideContextItems()">
>+  <menuitem id="context-connect"
>+            label="&account.connect.label;"
>+            accesskey="&account.connect.accesskey;"
>+            oncommand="gAccountManager.connect()"/>
>+  <menuitem id="context-disconnect"
>+            label="&account.disconnect.label;"
>+            accesskey="&account.disconnect.accesskey;"
>+            oncommand="gAccountManager.disconnect()"/>
>+  <menuseparator class="context-account-separator" />
No space before />

>+  <menuitem id="context-delete"
>+            label="&account.delete.label;"
>+            accesskey="&account.delete.accesskey;"
>+            oncommand="gAccountManager.delete()"/>
>+  <menuitem id="context-edit"
>+            label="&account.edit.label;"
>+            accesskey="&account.edit.accesskey;"
>+            oncommand="gAccountManager.edit()"/>
>+  <menuseparator class="context-account-separator" />
Ditto.

>+  <menuitem id="context-newAccount"
>+            label="&accountManager.newAccount.label;"
>+            accesskey="&accountManager.newAccount.accesskey;"
>+            oncommand="gAccountManager.new()"/>
>+</menupopup>

I tend to think this code would be a lot nicer to read if it was using a <command> tag for each action.
Attached patch Major Code refactoring (obsolete) — Splinter Review
*** Original post on bio 179 as attmnt 194 at 2009-08-15 21:03:00 UTC ***

Since I had to use commands, and broadcaster for synchronization between Buttons and Context Menu Items, I did a major refactoring in the code of the Account Manager, so that it is (I guess) more readable, takes less RAM.

I moved the bottom-buttons of the richlistitem in a new binding called buttons.
This binding is enabled only on the selected item, the other ones have a binding called nobuttons (xbl hack for "none").

Most of the code in the actual binding has been moved either on accounts.js, or in the new binding.
connect, disconnect, delete, observe were moved in accounts.js
setFocusButton, proceedDefaultAction in the new binding.

Offline is now an attribute of the richlistbox and no more one of the richlistitems (we can't have items that are online and other offline).
This attribute has no function but to be used with CSS if the developers want to add an effect on the richlistbox when offline.

Adding a property to access the buttons binding, and an other to access the Icon (used on connecting notification now handled in accounts.js).

Adding a hack in account.xml to prevent it from being completely unusable due to https://bugzilla.mozilla.org/show_bug.cgi?id=506491

The disable Timer is now global to the account manager, we can't have two buttons disabled for timer at the same time.
Fixing a few bugs with this timer, now it is cancelled when the account is deleted, and it no longer enable back the button if Instantbird has gone into offline mode during these 500ms.

The hiding of the context menu items connect / disconnect is independent from the current way the buttons are hidden.
The menu items already listen to the broadcaster for the hidden property so that putting an hidden attribute on the command won't work for the menu items.
Instead they are hidden when the menu appears.

While the menu is displayed, this is not possible to change the items (disabled or hidden), so the user has to make a new menu appears, it seems to be the normal behavior.

Adding a handle for VK_DELETE key on an account, it seems to be an intuitive way to delete an account.
Attachment #8351938 - Flags: review?(florian)
Comment on attachment 8351928 [details] [diff] [review]
Patch V1

*** Original change on bio 179 attmnt 184 at 2009-08-15 21:03:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351928 - Attachment is obsolete: true
Comment on attachment 8351938 [details] [diff] [review]
Major Code refactoring

*** Original change on bio 179 attmnt 194 at 2009-08-15 22:24:39 UTC ***

r=me with a few very minor details (discussed by IM directly with Romain) addressed.

I'll test myself the next version of the patch :)
Attachment #8351938 - Flags: review?(florian) → review+
*** Original post on bio 179 as attmnt 195 at 2009-08-15 22:27:00 UTC ***

Minor details fixed, tested with GNU/Linux.
Comment on attachment 8351938 [details] [diff] [review]
Major Code refactoring

*** Original change on bio 179 attmnt 194 at 2009-08-15 22:27:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351938 - Attachment is obsolete: true
*** Original post on bio 179 at 2009-08-16 08:11:06 UTC ***

(In reply to comment #6)
pushed as https://hg.instantbird.org/instantbird/rev/8852c7bcb091
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 0.2 → 0.2b1
Depends on: 955340
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: