Try to merge initAcountActionsButton() and updateButtons() in AccountManager.js

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 15.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

5 years ago
1. It looks like those 2 functions have almost the same logic, just set the properties on different objects (Seamonkey buttons, Thunderbird menuitems). That could probably be merged.
2. Besides removing duplicate code (and possibility of divergence) it seems it is also inefficient as TB also calls updateButtons() on each selection of an account in AM, decides the necessary properties just to then bail out of the function without setting anything.
3. the initAcountActionsButton() function has a typo.

Do I understand this correctly? Of course I'd like to merge only the common parts. The TB specific things like "Add IM account" item should be preserved.
(Assignee)

Comment 1

5 years ago
Created attachment 611168 [details] [diff] [review]
patch

UI should be unchanged by this patch, it is just code reorganization.
I have tested this works fine in Thunderbird.

Ian: please check it works on Seamonkey.

Florian: please check the hiding of "Add IM account" item still works as intended.
Attachment #611168 - Flags: review?(iann_bugzilla)
Attachment #611168 - Flags: feedback?(florian)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 738810
Comment on attachment 611168 [details] [diff] [review]
patch

(In reply to :aceman from comment #1)
> Florian: please check the hiding of "Add IM account" item still works as
> intended.

I haven't really tested this, just looked at the code, but I don't think it breaks the hiding of the "Add IM account" item. If you want to test it, you can use the advanced configuration editor to set mail.chat.enabled to false.
Attachment #611168 - Flags: feedback?(florian) → feedback+
(Assignee)

Comment 3

5 years ago
Thanks, I did that and it worked. I just wanted to be sure I am not missing anything (as I do not use/try IM yet).

Comment 4

5 years ago
Comment on attachment 611168 [details] [diff] [review]
patch

>+function initAccountActionsButtons(menupopup) {
>+  let account = getCurrentAccount();
>+  let tree = document.getElementById("accounttree");

>+  updateItems(tree, account,
You might as well inline tree and account as they're only used the once.
>+    document.getElementById("accountActionsAddMailAccount"),
>+    document.getElementById("accountActionsDropdownSetDefault"),
>+    document.getElementById("accountActionsDropdownRemove"));

>+function updateItems(tree, account, addAccountItem, setDefaultItem, removeItem) {
>+  let canSetDefault = true;
>+  let canDelete = true;
Is it worth having both these set as false to start with and adjusting the logic?

>+function updateBlockedItems(menuItems) {
As these aren't necessarily menuItems, perhaps use aItems as the variable name.

r=me with those answered/addressed.
Attachment #611168 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 613737 [details] [diff] [review]
patch v2

Updated, please see the inverted logic again.
Attachment #611168 - Attachment is obsolete: true
Attachment #613737 - Flags: review?(iann_bugzilla)

Comment 6

5 years ago
Comment on attachment 613737 [details] [diff] [review]
patch v2

>+    return; // Thunderbird isn't using these
Nit: Comment starts with a capital letter so should end with a full stop.
Attachment #613737 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 618018 [details] [diff] [review]
patch v3

Well, capital T in Thunderbird does not really start a sentence :) But no problem, at least I fixed bitrot. I still bitrot myself with other patches, seems my preventive steps are not enough.
Attachment #613737 - Attachment is obsolete: true
Attachment #618018 - Flags: review?(mconley)
Comment on attachment 618018 [details] [diff] [review]
patch v3

Review of attachment 618018 [details] [diff] [review]:
-----------------------------------------------------------------

aceman:

This looks really good - again, I'm so happy that you're cleaning up our old code.  Keep up the great work!

Just two notes below.  With those fixed, I'll give it another spin / review - and you should also get someone from SeaMonkey to look at this.

-Mike

::: mailnews/base/prefs/content/AccountManager.js
@@ +694,5 @@
> + * since these buttons aren't under the IFRAME.
> + */
> +function updateBlockedItems(aItems) {
> +  // for each loop does not work here.
> +  for (let i = 0; i < aItems.length; i++) {

squib and I have been preferring this looping pattern lately:

for each (let [, item] in Iterator(aItems)) {
  let prefstring = item.getAttribute("prefstring");
  ...
}

@@ +700,4 @@
>      if (!prefstring)
>        continue;
>  
> +    if (Services.prefs.prefIsLocked(prefstring))

The old code seems to check if the preference is locked, is a bool, and is set to "true":

"if (prefBranch.prefIsLocked(prefstring) && prefBranch.getBoolPref(prefstring))"

We should probably carry over that logic, or we might risk some edge-case breakage.
Attachment #618018 - Flags: review?(mconley) → review-
(Assignee)

Comment 9

5 years ago
(In reply to Mike Conley (:mconley) from comment #8)
> Just two notes below.  With those fixed, I'll give it another spin / review
> - and you should also get someone from SeaMonkey to look at this.
Ian from Seamonkey looked at it.

> squib and I have been preferring this looping pattern lately:
> 
> for each (let [, item] in Iterator(aItems)) {
>   let prefstring = item.getAttribute("prefstring");
>   ...
> }
And what is the gain from this besides making the code unreadable for me? :)
Also, be aware I pass menupopup into as aItems and that is some special array that can't be iterated via for each (return 1 more element than there are menu items). So let's hope this Iterator stuff works.

> "if (prefBranch.prefIsLocked(prefstring) &&
> prefBranch.getBoolPref(prefstring))"
> 
> We should probably carry over that logic, or we might risk some edge-case
> breakage.

It does it only in the TB version, not SM. But OK let's make them the same. I do not understand why they would even need be locked (setting to true would be fine) but just carry it over.
(Assignee)

Comment 10

5 years ago
Created attachment 619284 [details] [diff] [review]
patch v4
Attachment #618018 - Attachment is obsolete: true
Attachment #619284 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #619284 - Flags: review?(iann_bugzilla)

Comment 11

5 years ago
Comment on attachment 619284 [details] [diff] [review]
patch v4

>+    if (Services.prefs.prefIsLocked(prefstring) &&
>+        Services.prefs.getBoolPref(prefstring))
>+      item.setAttribute("disabled", true);
This would break SeaMonkey users that use this functionality as SM currently requires the pref to be only locked, not locked, a boolean and true.
Potentially there are edge cases from the Thunderbird side where it has only been locked not set to true, though prior to 2009 I believe the requirement was the same as for SM (i.e. locked pref only).
r- :(
Attachment #619284 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 619403 [details] [diff] [review]
patch v5

OK, so let's differentiate TB and SM.
Attachment #619284 - Attachment is obsolete: true
Attachment #619284 - Flags: review?(mconley)
Attachment #619403 - Flags: review?(iann_bugzilla)

Comment 13

5 years ago
Comment on attachment 619403 [details] [diff] [review]
patch v5


>+/**
>+ * Disable buttons/menu items if their control preference is locked.
>+ * Seamonkey: Not currently handled by WSM or the main loop yet
Nit: SeaMonkey not Seamonkey
>+ * since these buttons aren't under the IFRAME.
>+ *
>+ * @param aItems  the elements to be checked
>+ * @param aMustBeTrue  if true then the value of the pref must be set to true
Nit: "...pref must be boolean and set to true..."
>+ *                     to trigger the disabling (TB requires this, SM not)
Attachment #619403 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 619405 [details] [diff] [review]
patch v6

Ok, thanks.
Attachment #619403 - Attachment is obsolete: true
Attachment #619405 - Flags: review?(mconley)
aceman:

Can I convince you to write some tests for this?

-Mike
(Assignee)

Comment 16

5 years ago
You'd have a hard time at that :) But you can try, depends on how long does it take you to teach tests to me :)
(In reply to :aceman from comment #16)
> You'd have a hard time at that :) But you can try, depends on how long does
> it take you to teach tests to me :)

Our tests can be found under mail/test/mozmill.  You'll want to create the test in the "accounts" subdirectory of that folder.

Your test should programmatically add a variety of account types during its setup, and then select them one by one, checking that the menulist displays the correct items as enabled and disabled.

Sound do-able?

-Mike
(Assignee)

Comment 18

5 years ago
No, I have not yet created any xpcshell nor mozmill test.
Flags: in-testsuite?
Comment on attachment 619405 [details] [diff] [review]
patch v6

So the code looks good, and it appears to work as advertised, but I really would like to see some tests for this stuff.

Can you prepare some tests in another patch on this bug?
Attachment #619405 - Flags: review?(mconley) → review+
(Assignee)

Comment 20

5 years ago
Not yet.
Depends on how much time you have to go through it on IRC :)
aceman:

Any luck on those tests?

-Mike
(Assignee)

Comment 22

5 years ago
We can try to look at them today.
(Assignee)

Comment 23

5 years ago
Created attachment 622905 [details] [diff] [review]
WIP patch of a test
(Assignee)

Comment 24

5 years ago
The WIP test does not need any flags, it was only uploaded so that mconley can try it out and help me with some issues.

I'll upload a more complete version (testing all menu items) soon.
(Assignee)

Comment 25

5 years ago
Created attachment 623274 [details] [diff] [review]
mozmill test

So my first mozmill test comes! Many thanks to mconley.

Mconley, please see if I do not import any unneded modules. I just copied many declarations and some may be useless for this test.
Attachment #622905 - Attachment is obsolete: true
Attachment #623274 - Flags: review?(mconley)
(Assignee)

Comment 26

5 years ago
Mconley, I hardcoded some pref names in the test. Maybe I should fetch them from the attributes of the menutitems so that the test works if they change?
Comment on attachment 623274 [details] [diff] [review]
mozmill test

Review of attachment 623274 [details] [diff] [review]:
-----------------------------------------------------------------

Great work aceman, especially considering this is your first Mozmill test.

Just a few things I noticed. After this, your patch will be in pretty decent shape, and I'll give it a spin.

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

This should be MPL2.

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +var MODULE_NAME = "test-archive-actions";

I know you mostly copied most of these from other tests, but MODULE_NAME, RELATIVE_ROOT and MODULE_REQUIRES should be consts.

@@ +40,5 @@
> +var RELATIVE_ROOT = "../shared-modules";
> +var MODULE_REQUIRES = ["folder-display-helpers", "window-helpers",
> +                       "account-manager-helpers"];
> +
> +Components.utils.import("resource:///modules/mailServices.js");

You should be able to use Cu here instead of Components.utils.  It should be available automatically.

@@ +43,5 @@
> +
> +Components.utils.import("resource:///modules/mailServices.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +var mozmill = {};

Neither mozmill.js nor controller.js is used, and can be removed.

@@ +48,5 @@
> +Components.utils.import("resource://mozmill/modules/mozmill.js", mozmill);
> +var controller = {};
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);

I also don't think elib is required - see below for the instance where you used it.

@@ +50,5 @@
> +Components.utils.import("resource://mozmill/modules/controller.js", controller);
> +var elib = {};
> +Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
> +
> +var accountManager = MailServices.accounts;

Why the alias?

@@ +57,5 @@
> +var nntpAccount;
> +var originalAccountCount;
> +
> +function setupModule(module) {
> +  let wh = collector.getModule("window-helpers");

You're not using wh, fdh, or amh, so these can be shortened to:

collector.getModule("window-helpers").installInto(module);
collector.getModule("folder-display-helpers").installInto(module);
etc

@@ +104,5 @@
> + * @param accountKey  the key of the account to select
> + * @param isSetAsDefaultEnabled  true if the menuitem should be enabled, false otherwise
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise

Nit, missing a star on this line.

@@ +106,5 @@
> + * @param isRemoveEnabled        true if the menuitem should be enabled, false otherwise
> + * @param isAddAccountEnabled    true if the menuitems (Add Mail Account+Add Other Account)
> +                                 should be enabled, false otherwise
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,

Nit - we tend to name function parameters starting with a, like:

aAccountKey, aController, aIsSetAsDefaultEnabled.

See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes

@@ +109,5 @@
> + */
> +function subtest_check_account_actions(amc, accountKey, isSetAsDefaultEnabled,
> +                                       isRemoveEnabled, isAddAccountEnabled)
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));

Controller.waitForElement is deprecated.

Do you really need to wait for the element to appear?  I don't think it's inserted dynamically...in which case, you can probably remove this wait.

@@ +112,5 @@
> +{
> +  amc.waitForElement(new elib.Elem(amc.window.document.getElementById("accountActionsButton")));
> +
> +  let rowIndex = 0; // count which row in the account tree we need to click
> +  let accountTreeNode = amc.window.document.getElementById("account-tree-children");

You should just be able to use amc.e("account-tree-children");

@@ +136,5 @@
> +  amc.click(amc.eid("accountActionsButton"), 5, 5);
> +  wait_for_popup_to_open(amc.e("accountActionsDropdown"));
> +
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);

You can use assert_not_equals here, I believe.

@@ +139,5 @@
> +  let actionAddMailAccount = amc.window.document.getElementById("accountActionsAddMailAccount");
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");

amc.e("accountActionsAddOtherAccount"). Happens a few more times in this function.

@@ +140,5 @@
> +  assert_equals(actionAddMailAccount != undefined, true);
> +  assert_equals(!actionAddMailAccount.getAttribute("disabled"), isAddAccountEnabled);
> +
> +  let actionAddOtherAccount = amc.window.document.getElementById("accountActionsAddOtherAccount");
> +  assert_equals(actionAddOtherAccount != undefined, true);

assert_not_equals. Also use a few times below when checking against undefined.

@@ +215,5 @@
> +  Services.prefs.unlockPref(disableItemPref);
> +  Services.prefs.getDefaultBranch("").deleteBranch(disableItemPref);
> +}
> +
> +function teardownModule(module) {

Please put the teardownModule directly below the setupModule.
Attachment #623274 - Flags: review?(mconley) → review-
(Assignee)

Comment 28

5 years ago
Created attachment 624493 [details] [diff] [review]
mozmill test v2

Thanks, done.
Attachment #623274 - Attachment is obsolete: true
Attachment #624493 - Flags: review?(mconley)
Comment on attachment 624493 [details] [diff] [review]
mozmill test v2

Review of attachment 624493 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the two nits I found, this looks good. With those fixed, r=me.

Great job on your first Mozmill test!

::: mail/test/mozmill/account/test-account-actions.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

MPL 2.

@@ +40,5 @@
> +
> +Cu.import("resource:///modules/mailServices.js");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var imapAccount;

We prefer let over var, and you might as well define them all in a row, like

let imapAccount, nntpAccount, originalAccountCount;
Attachment #624493 - Flags: review?(mconley) → review+
(Assignee)

Comment 30

5 years ago
Created attachment 626173 [details] [diff] [review]
mozmill test v3

Thanks, done!
Attachment #624493 - Attachment is obsolete: true
Attachment #626173 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [checkin both patches]
https://hg.mozilla.org/comm-central/rev/445fb8b2b687
https://hg.mozilla.org/comm-central/rev/9f3422bc53d8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin both patches]
Target Milestone: --- → Thunderbird 15.0
Unfortunately, this seems to have permanent orange on the Linux 32 bit builders, I've therefore backed out just the unit test:

https://hg.mozilla.org/comm-central/rev/7e8025cb9781

https://tbpl.mozilla.org/php/getParsedLog.php?id=11980780&tree=Thunderbird-Trunk

EST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | setupModule
Test Failure: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgAccountManager.createIncomingServer]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::setupModule
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test_account_actions
Test Skipped: "setupModule failed."
WARNING | test-account-actions.js::test_account_actions | (SKIP) setupModule failed.
TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | teardownModule
Test Failure: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgAccountManager.removeAccount]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-actions.js | test-account-actions.js::teardownModule


(I would have disabled it on Linux, but I don't think we can skip setupModule with skip-if)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

5 years ago
Did all other platforms work?

I find it strange it fails in nsIMsgAccountManager.createIncomingServer().
(Assignee)

Comment 34

5 years ago
I developed the test on 32bit Linux and it worked. Any ideas?
(Assignee)

Comment 35

5 years ago
Created attachment 626948 [details] [diff] [review]
mozmill test v4

Fixed test. Actually the test-archive-options.js test was not cleaning up after itself and the 2 tests collided (created an account with the same server name).
The problem did not appear if running each test standalone.
Attachment #626173 - Attachment is obsolete: true
Attachment #626948 - Flags: review?(mconley)
Comment on attachment 626948 [details] [diff] [review]
mozmill test v4

Review of attachment 626948 [details] [diff] [review]:
-----------------------------------------------------------------

Just one nit. With that fixed, I think this is good to go (again).

(I ran the tests locally, and this looked fine.)

::: mail/test/mozmill/account/test-account-actions.js
@@ +17,5 @@
> +  collector.getModule("window-helpers").installInto(module);
> +  collector.getModule("folder-display-helpers").installInto(module);
> +  collector.getModule("account-manager-helpers").installInto(module);
> +
> +  // There may already be some accounts existent.

Probably better: "There may be pre-existing accounts from other tests."
Attachment #626948 - Flags: review?(mconley) → review+
(Assignee)

Comment 37

5 years ago
Created attachment 628073 [details] [diff] [review]
mozmill test v5

Thanks!
Attachment #626948 - Attachment is obsolete: true
Attachment #628073 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [check in the test only, the patch is checked-in, comment 31]
Mozmill test checked in: https://hg.mozilla.org/comm-central/rev/9e2dc0559734
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in the test only, the patch is checked-in, comment 31]
You need to log in before you can comment on or make changes to this bug.