237210 - In <abCommon.js>, "Warning: function GetSelectedCardTypes does not always return a value"; plus various cleanups including a few additionnal fixes.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

{{
Warning: function GetSelectedCardTypes does not always return a value
Source File: chrome://messenger/content/addressbook/abCommon.js
Line: 350
Source Code:
}
}}

Code is: (I added 2 comments)
{{
function GetSelectedCardTypes()
{

// SergeG: {beginning of code removed}

  if (mailingListCnt && cardCnt)
    return kListsAndCards;        // lists and cards selected
  else if (mailingListCnt && !cardCnt) {
    if (mailingListCnt > 1)
      return kMultipleListsOnly; // only multiple mailing lists selected
    else
      return kSingleListOnly;    // only single mailing list
  }
  else if (!mailingListCnt && cardCnt)
    return kCardsOnly;           // only card(s) selected

// SergeG:
//  else // if (!mailingListCnt && !cardCnt)
//    // impossible case !

}
}}

The code logic is "right"; the warning is "right" :->

Removing/commenting |if (!mailingListCnt && cardCnt)| should be enough to get
rid of the warning.

Anyway, I'll do a little more cleanup too ...
(but going to bed for now)
... and the check-in will have to wait for v1.8a tree to open anyway !

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <abCommon.js>

Bug fix,
plus lots of cleanup;

Includes:
{{
-  // XXX todo remove once #116341 is fixed
-  if (!sortColumn)
-    return;
}}

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 143873 [details] [diff] [review]
(Av1) <abCommon.js>


'r=?':
Could you review and test it ?

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 143873 [details] [diff] [review]
(Av1) <abCommon.js>

>@@ -530,14 +507,15 @@ function GetAddressesForCards(cards)
>     return addresses;
> 
>   var count = cards.length;
>-  if (count > 0)
>+  if (count > 0) {
>     addresses += GenerateAddressFromCard(cards[0]);
> 
>-  for (var i = 1; i < count; i++) { 
>-    var generatedAddress = GenerateAddressFromCard(cards[i]);
>-
>-    if (generatedAddress)
>-      addresses += "," + generatedAddress;
>+    for (var i = 1; i < count; ++i) {
>+      var generatedAddress = GenerateAddressFromCard(cards[i]);
>+      if (generatedAddress)
>+        addresses += ",";
>+      addresses += generatedAddress;

Pay special attention to this split:
it should fix a potential bug, isn't it ?
(I copied the split from another function in this file.)

>+    }
>   }
>   return addresses;
> }

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <abCommon.js>

Av1, with much less cleanup.

Includes:
{{
-  // XXX todo remove once #116341 is fixed
-  if (!sortColumn)
-    return;
}}

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 144136 [details] [diff] [review]
(Av1b) <abCommon.js>


'r=?':
Could you review and test it ?

Comment 6

[[e-mail]]

Comment on attachment 144136 [details] [diff] [review]
(Av1b) <abCommon.js>

>@@ -264,12 +260,8 @@ function GetParentRow(aTree, aRow)
>   var row = aRow;
>   var level = aTree.view.getLevel(row);
>   var parentLevel = level;
>-  while (parentLevel >= level) {
>-    row--;
>-    if (row == -1)
>-      return row;
>+  while ((parentLevel >= level) && (--row != -1))
>     parentLevel = aTree.view.getLevel(row);
>-  }
>   return row;
> }
Bletch, hewitt must have been asleep when he wrote that.
Unless I'm mistaken, nsITreeView.idl lists a function that does all that, so
whomever calls this should be fixed to use the tree view instead.

>     if (generatedAddress)
>-      addresses += "," + generatedAddress;
>+      addresses += ",";
>+    addresses += generatedAddress;
I can't see the point of this.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <abCommon.js> ++

Av1b, with comment 6 suggestion(s).

Includes:
{{
-  // XXX todo remove once #116341 is fixed
-  if (!sortColumn)
-    return;
}}

Could you review and test it ?

Comment 8

[[e-mail]]

Comment on attachment 145174 [details] [diff] [review]
(Av2) <abCommon.js> ++

Haven't tested this yet, just some nits.

>+const ldapUrlPrefix = "moz-abldapdirectory://";
Ideally this ought to be renamed to start with a k like the others.

selected
>+  if (mailingListCnt > 0)
>+    if (cardCnt > 0)
>+      return kListsAndCards;
>     else
>+      return (mailingListCnt == 1) ? kSingleListOnly : kMultipleListsOnly;
>+  else
>+    return kCardsOnly;
I don't like these nested ifs, especially with the returns.
You should be able to rewrite it like this:
if ...
  return kCardsOnly;
if ...
  return kListsAndCards;
if ...
  return kSingleListOnly;
return kMultipleListsOnly;

Comment 9

[[e-mail]]

Comment on attachment 145174 [details] [diff] [review]
(Av2) <abCommon.js> ++

>           if ((selectedDir.indexOf(ldapUrlPrefix, 0)) == 0)
If you change the name as I suggested, please also change this -
selectedDir.lastIndexOf(kLdapUrlPrefix) == 0 is more efficient.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) <abCommon.js> ++

Av2 (was Av1c), with comment 8 & 9 suggestion(s).


> I don't like these nested ifs, especially with the returns.

I tried the reverse approach this time: |return (() ? :)|...

> >	      if ((selectedDir.indexOf(ldapUrlPrefix, 0)) == 0)

(for the record)
If this function is from </xpcom/ds/nsISupportsArray.*>,
either the |, 0| was extraneous, or the function should have been
|IndexOfStartingAt|, shouldn't it ?
Otherwise, I did not find its definition.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 145607 [details] [diff] [review]
(Av3) <abCommon.js> ++


'r=?' again: (see comment 10)

Comment 12

[[e-mail]]

Comment on attachment 145607 [details] [diff] [review]
(Av3) <abCommon.js> ++

So near and yet so far ;-)

>+          if (selectedDir.lastIndexOf(kLdapUrlPrefix) == 0)
http://devedge.netscape.com/library/manuals/2000/javascript/1.5/reference/strin
g.html#1197005
The , 0 means that the check is only done at the beginning of the string.
In other words, it only checks if the substring matches at the beginning.

>+  return (mailingListCnt == 0) ? kCardsOnly :
>+           ((cardCnt > 0) ? kListsAndCards :
>+             ((mailingListCnt == 1) ? kSingleListOnly :
>+               kMultipleListsOnly));
Hmm... OK, but I think all those (s make it look ugly...

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av3b) <abCommon.js> ++ [Checked in: See comment 18]

Av3, with comment 12 suggestion(s).

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]

(In reply to comment #12)
> >+          if (selectedDir.lastIndexOf(kLdapUrlPrefix) == 0)
> http://devedge.netscape.com/library/manuals/2000/javascript/1.5/reference/strin
> g.html#1197005
> The , 0 means that the check is only done at the beginning of the string.

Plain JS function, uh :-> Thanks.
It's you who "removed" that second parameter in comment 9 :-/

> >+  return (mailingListCnt == 0) ? > Hmm... OK, but I think all those (s make it look ugly...

Any better now !?

Comment 15

[[e-mail]]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]

I'll leave it as an exercise for the reader to work out what time it was when I
tried to write that comment :-[

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]


'approval1.7=?':
Simple JS code fix & cleanup, no/low risk.

Comment 17

[[e-mail]]

Fix checked in.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]

(In reply to comment #17)
> Fix checked in.

Actually, neil checked in a modified version:
{{
-	     var prefName = selectedDir.substr(kLdapUrlPrefix.length,
selectedDir.length);
+	     var prefName = selectedDir.substr(kLdapUrlPrefix.length);

-	 args.selectedDirectoryString =
selecteduri.substr(kLdapUrlPrefix.length, selecteduri.length);
+	 args.selectedDirectoryString =
selecteduri.substr(kLdapUrlPrefix.length);
}}

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) <abCommon.js> additional cleanup

Follow-up patch:
no misbehaviour, only a few code "cleanups" from Av1.

Comment 20

[[e-mail]]

Comment on attachment 146384 [details] [diff] [review]
(Bv1-SM) <abCommon.js> additional cleanup

>Index: mozilla/mailnews/addrbook/resources/content/abCommon.js
>         var card = directory.addressLists.GetElementAt(i);
>-        card = card.QueryInterface(Components.interfaces.nsIAbCard);
>-        cards[i] = card;
>+        cards[i] = card.QueryInterface(Components.interfaces.nsIAbCard);
This can be further improved, see:
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abC
ardOverlay.js#180

>-  if (gAbView)
>-    return gAbView.URI;
>-  else 
>-    return null;
>+  return gAbView ? gAbView.URI : null;
I think you can write this as return gAbView && gAbView.URI;

>-    if(directory.description)
>-      email = directory.description;
>-    else
>-      email = card.displayName;
>+    email = directory.description ? directory.description : card.displayName;
This should say email = directory.description || card.displayName;

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b-SM) <abCommon.js> additional cleanup [Checkin: Comment 24]

Bv1, with comment 20 suggestion(s).

(I'm learning JavaScript here :-))

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]

'approval1.7=?':
Trivial U.I. code cleanup, low risk.
(already on Trunk)

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 145624 [details] [diff] [review]
(Av3b) <abCommon.js> ++
[Checked in: See comment 18]

warning cleanups can happen on the trunk. there's not much user value, even if
the risk is low -- and warning cleanups have broken features in the past.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 146946 [details] [diff] [review]
(Bv1b-SM) <abCommon.js> additional cleanup
[Checkin: Comment 24]

I just checked this in for Serge:

mailnews/addrbook/resources/content/abCommon.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCommon.js,v  <-- 
abCommon.js
new revision: 1.103; previous revision: 1.102

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) <abCommon.js> additional cleanup [Checked in: Comment 26]

Bv1b port to TB,
with the two changes from comment 18.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 194022 [details] [diff] [review]
(Cv1-TB) <abCommon.js> additional cleanup
[Checked in: Comment 26]

Checked in on behalf of Serge.

Checking in mail/components/addrbook/content/abCommon.js;
/cvsroot/mozilla/mail/components/addrbook/content/abCommon.js,v  <--  abCommon.js
new revision: 1.13; previous revision: 1.12