Closed
Bug 739573
Opened 12 years ago
Closed 12 years ago
Try to merge initAcountActionsButton() and updateButtons() in AccountManager.js
Categories
(MailNews Core :: Account Manager, defect)
MailNews Core
Account Manager
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 15.0
People
(Reporter: aceman, Assigned: aceman)
References
()
Details
Attachments
(2 files, 10 obsolete files)
9.22 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
8.57 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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.
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)
Comment 2•12 years ago
|
||
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+
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 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+
Updated, please see the inverted logic again.
Attachment #611168 -
Attachment is obsolete: true
Attachment #613737 -
Flags: review?(iann_bugzilla)
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+
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 8•12 years ago
|
||
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-
(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•12 years ago
|
||
Attachment #618018 -
Attachment is obsolete: true
Attachment #619284 -
Flags: review?(mconley)
Attachment #619284 -
Flags: review?(iann_bugzilla)
Comment 11•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
Ok, thanks.
Attachment #619403 -
Attachment is obsolete: true
Attachment #619405 -
Flags: review?(mconley)
Comment 15•12 years ago
|
||
aceman: Can I convince you to write some tests for this? -Mike
Assignee | ||
Comment 16•12 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 :)
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
No, I have not yet created any xpcshell nor mozmill test.
Flags: in-testsuite?
Comment 19•12 years ago
|
||
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•12 years ago
|
||
Not yet. Depends on how much time you have to go through it on IRC :)
Comment 21•12 years ago
|
||
aceman: Any luck on those tests? -Mike
Assignee | ||
Comment 22•12 years ago
|
||
We can try to look at them today.
Assignee | ||
Comment 23•12 years ago
|
||
Assignee | ||
Comment 24•12 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•12 years ago
|
||
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•12 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 27•12 years ago
|
||
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•12 years ago
|
||
Thanks, done.
Attachment #623274 -
Attachment is obsolete: true
Attachment #624493 -
Flags: review?(mconley)
Comment 29•12 years ago
|
||
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•12 years ago
|
||
Thanks, done!
Attachment #624493 -
Attachment is obsolete: true
Attachment #626173 -
Flags: review+
Keywords: checkin-needed
Whiteboard: [checkin both patches]
Comment 31•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/445fb8b2b687 https://hg.mozilla.org/comm-central/rev/9f3422bc53d8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin both patches]
Target Milestone: --- → Thunderbird 15.0
Comment 32•12 years ago
|
||
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•12 years ago
|
||
Did all other platforms work? I find it strange it fails in nsIMsgAccountManager.createIncomingServer().
Assignee | ||
Comment 34•12 years ago
|
||
I developed the test on 32bit Linux and it worked. Any ideas?
Assignee | ||
Comment 35•12 years ago
|
||
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 36•12 years ago
|
||
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•12 years ago
|
||
Thanks!
Attachment #626948 -
Attachment is obsolete: true
Attachment #628073 -
Flags: review+
Keywords: checkin-needed
Whiteboard: [check in the test only, the patch is checked-in, comment 31]
Comment 38•12 years ago
|
||
Mozmill test checked in: https://hg.mozilla.org/comm-central/rev/9e2dc0559734
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 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.
Description
•