441135 - Clean up AbSearchDialog.xul's global story

Status: RESOLVED INACTIVE

(Thunderbird :: Address Book, defect)

Description

[Joey Minta]

Attachment: patch v1

This is similar to bug 441077, since the two dialogs have similar structure.  The one nice thing is that these files are already forked, which saves some trouble.  In short, ABSearchDialog includes a bunch of files it doesn't need, and has a bunch of annoying global variables too.  This patch cleans things up, reducing the global scope from 520 items to 283.  I didn't test to see if there was a similar improvement in window-opening time, as in bug 441077, but I'd bet on at least a few ms.

Comment 1

[Joey Minta]

Attachment: dependency map

This may help during the review.  It's the dependency map I wrote out, after reading through ABSearchDialog.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 326171 [details] [diff] [review]
patch v1

>-function onSearchStop() 
>-{
>-}

I don't want to drop this function, we need it for bug 125195 (which really I should look at sometime). Please could you add an XXX comment instead referencing that bug. The onunload in the xul should then become onunload="onSearchStop(); CloseAbView();", and you'll have to unmodify the key handlers.

>   if (directory.isRemote) {
>     if (booleanAnd)
>-      return nsMsgSearchScope.LDAPAnd;
>+      return Components.interfaces.nsMsgSearchScope.LDAPAnd;

I think nsMsgSearchScope could be declared as a const locally here.

> function onEnterInSearchTerm()
> {
>-  // on enter
>-  // if not searching, start the search
>-  // if searching, stop and then start again
>-  if (gSearchStopButton.getAttribute("label") == gSearchBundle.getString("labelForSearchButton")) { 
>-     onSearch(); 
>-  }
>-  else {
>-     onSearchStop();
>-     onSearch();
>-  }
>+  onSearch();
> }

As per my earlier comment, I don't want to drop this distinction even though its not doing much for us at the moment.

>       switch (searchTerm.attrib) {
>-       case nsMsgSearchAttrib.Name:
>+       case Components.interfaces.nsMsgSearchAttrib.Name:

I think its definitely worth making nsMsgSearchAttrib a local constant here.

>       switch (searchTerm.op) {
>-      case nsMsgSearchOp.Contains:
>+      case Components.interfaces.nsMsgSearchOp.Contains:

ditto with nsMsgSearchOp

>-    if (event.target.label == gSearchBundle.getString("labelForSearchButton"))
>+    if (event.target.label == document.getElementById("bundle_search").getString("labelForSearchButton"))

nit: please put the document.get.... on the next line.


>-    gPropertiesButton.setAttribute("disabled","true");
>-    gComposeButton.setAttribute("disabled","true");
>+    document.getElementById("propertiesButton").setAttribute("disabled","true");
>+    document.getElementById("composeButton").setAttribute("disabled","true");

nit: please add spaces after the commas, as you're touching these lines.

>   else
>-    gPropertiesButton.setAttribute("disabled","true");
>+    document.getElementById("propertiesButton").setAttribute("disabled","true");

ditto.

>       <hbox align="start">
>-        <button label="&propertiesButton.label;" id="propertiesButton" oncommand="onProperties()" accesskey="&propertiesButton.accesskey;" disabled="true"/>
>-        <button label="&composeButton.label;" id="composeButton" oncommand="onCompose()" accesskey="&composeButton.accesskey;" disabled="true"/>
>+        <button label="&propertiesButton.label;" id="propertiesButton" oncommand="AbEditSelectedCard()" accesskey="&propertiesButton.accesskey;" disabled="true"/>
>+        <button label="&composeButton.label;" id="composeButton" oncommand="AbNewMessage()" accesskey="&composeButton.accesskey;" disabled="true"/>

nit: please wrap these lines at appropriate places.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 326171 [details] [diff] [review])
> >-function onSearchStop() 
> >-{
> >-}
> 
> I don't want to drop this function, we need it for bug 125195 (which really I
> should look at sometime). Please could you add an XXX comment instead
> referencing that bug. The onunload in the xul should then become
> onunload="onSearchStop(); CloseAbView();", and you'll have to unmodify the key
> handlers.

Actually leave the key handlers as you have modified them in the patch. Sorry, my mistake, they should just have oncommand="closeWindow(true)" as per your version.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> they should just have oncommand="closeWindow(true)" as per your version.

Maybe s/closeWindow(true)/window.close()/g: see bug 441220 comment 5.

Comment 5

[Wayne Mery (:wsmwk)]

obsolete?

Comment 6

[Geoff Lankow (:darktrojan)]

More or less. It still could use a clean up (so could everything else), but it'd probably be easier to start from scratch than try to revive a patch this old.