79792 - Changes to LDAP autocomplete prefs not honored if composition window open

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[Hong Kwon]

From Bugzilla Helper:
User-Agent: Mozilla/4.78b1 [en] (Windows NT 5.0; U)
BuildID:    2001050804

If you change your LDAP autocompletion settings (either the global autocomplete 
in Preferences or the per-account autocomplete in Mail & News Account Settings) 
while the composition window is open, the changes are not reflected until the 
composition window is closed and a new one is open.  



Reproducible: Always
Steps to Reproduce:
Scenario 1:

1. Make sure local address book AND ldap autocomplete off.
2. Open compose.
3. Type email address (hong for example)
4. Open Preferences->Mail&Newsgroups->Address Books
5. Enable LDAP autocomplete (make sure you have a server)
6. Erase the email address, or add a new one

Expected Results

Autocompletion occurs against directory

Actual result

No autocompletion occurs 

Scenario 2

1. make sure global autocomplete turned on, and per-account is turned OFF.
2. open compose window.
3. open Mail & News Account Settings->[your test account]->Server Settings->LDAP 
Directory Server
4. Click on Override and pick "None"
5. Close Account Settings
6. Type in 'hong'

Expected Results

No autocompletion occurs.

Actual result:

Autocompletion occurs.


This happens to changes in both the Preferences and the Mail Account Setup 
prefs.  The prefs are only being read for LDAP autocomplete when the window is 
not invoked, not for each autocomplete session.

This isn't a general addressbook problem because the behavior is different if 
you turn on local addressbook autocomplete while the composition window 
(autocompletion is toggled correctly).

This is a 4x parity problem as if you change the autocomplete settings while 
composing a mail msg, the settings are correctly applied in real time.

Comment 1

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 2

[Srilatha Moturi]

Attachment: patch v1

Comment 3

[Srilatha Moturi]

The above patch fixes both this bug and bug 80783 -ldap autocomplete should                                          
respect online/offline setting 

Comment 4

[Srilatha Moturi]

ccin mohanb for an r=, sspitzer for an sr=

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v2

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Previous iteration of the patch reintroduced some old code which has since been
revised in CVS.  This version fixes several regressions caused by that.  Also
cleans up formatting in setupLdapAutocompleteSession.

Comment 7

[Mohan Bhamidipati]

r=mohanb;

Comment 8

[Srilatha Moturi]

Attachment: patch with add and remove sessions

Comment 9

[Srilatha Moturi]

with the above patch, autocompletion doesn't work. Still trying to fix it.

Comment 10

[Srilatha Moturi]

Attachment: patch v4

Comment 11

[Srilatha Moturi]

With the latest patch autocompletion works only fot the first address field.

Comment 12

[Srilatha Moturi]

Attachment: patch v5

Comment 13

[Srilatha Moturi]

Attachment: autocomplete.xml patch

Comment 14

[Srilatha Moturi]

Attachment: autocomplete.xml patch

Comment 15

[Srilatha Moturi]

Attachment: Patch v6

Comment 16

[Srilatha Moturi]

with the latest patch autocomplete should work for all recipients

Comment 17

[Srilatha Moturi]

Attachment: patch v7

Comment 18

[Joe Hewitt (gone)]

looks good - sr=hewitt

Comment 19

[Srilatha Moturi]

Attachment: path v8

Comment 20

[Srilatha Moturi]

In the latest patch, the addSession gets called for all the recipients rather 
than jus recipient#1.

ccing ducarroz for an r=
hewitt, does your sr= still apply?

Comment 21

[Jean-Francois Ducarroz]

Few comments:

1)
@@ -558,6 +564,11 @@
     // sanity checks
     if (topic != "network:offline-status-changed") return;
     MessageComposeOfflineStateChanged(state == "offline");
+    if (state == "offline")
+      isOffline = true;
+    else
+      isOffline = false;
+    setupLdapAutocompleteSession();

can you do something like:
  isOffline = (state == "offline");
  MessageComposeOfflineStateChanged(isOffline);
  setupLdapAutocompleteSession();

2)
@@ -566,6 +577,7 @@
   var observerService =
Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
   observerService.AddObserver(messageComposeOfflineObserver,
"network:offline-status-changed");
   
+  isOffline = ioService.offline;
   // set the initial state of the send button
   MessageComposeOfflineStateChanged(ioService.offline);


can you pass isOffline to MessageComposeOfflineStateChanged() ?

3)
+function RemoveDirectoryServerObserver(prefstring)
+{
+  if (!prefstring) {
+    prefs.removeObserver("ldap_2.autoComplete.useDirectory",
directoryServerObserver);
+    prefs.removeObserver("ldap_2.autoComplete.directoryServer",
directoryServerObserver);
+  }
+  else
+  {
+    var str = prefstring + ".overrideGlobalPref";
+    prefs.removeObserver(prefstring, directoryServerObserver);
+    str = prefstring + ".directoryServer";
+    prefs.removeObserver(prefstring, directoryServerObserver);
+  }
+}

Something seems wrong here! either you don't need the variable str but then why
call twice prefs.removeObserver or
you need to pass str to the function prefs.removeObserver instead of prefstring.

4) Do we need to remove the seesion when we remove a recipient row?

Comment 22

[Srilatha Moturi]

Attachment: patch v9

Comment 23

[Srilatha Moturi]

The above patch does 1,2 and3 from ducarroz's comments.
4) Do we need to remove the seesion when we remove a recipient row?
I am not sure, but I think we do not have because it just holding a reference to 
the sessions and the references should go away when we remove the widget. 
i am assuming that we remove the widget associated with a row when we remove 
that recipient row.
Hewitt, what is your take on it?

Comment 24

[Jean-Francois Ducarroz]

much better. R=ducarroz

Comment 25

[Joe Hewitt (gone)]

looks good - sr=hewitt

Comment 26

[Srilatha Moturi]

Attachment: patch v10

Comment 27

[Srilatha Moturi]

In the last patch, overrideGlobalPref is changed to overrideGlobal_Pref in add 
and remove observer because the mail.identity.<idkey>.overrideGlobal_Pref is 
preference we save.
Tested this patch on windows and mac.

Comment 28

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to frozen 0.9.2.
(on behalf of drivers)

Comment 29

[Srilatha Moturi]

Testing that has to be done with this fix:
All the test cases in the following bugs
bug 79939
bug 79792
bug 80783
bug 82169
bug 81761

And try these test cases with more than one recipient. 
Try if autocompletion works for more than one recipient.
Delete and add recipients and see if autocompletion works for all recipients.

Test if autocompletion works for all recipients after cahnging global or 
mail/news account specific autcompletion preferences.

Test if autocompletion happens against the right ldap server with more than one 
compose window(each with a different identity) open, when you switch between 
windows.

Comment 30

[Srilatha Moturi]

the above comments are to help qa in testing this fix. It is important that this 
fix is tested with mutiple recipients.

Comment 31

[Srilatha Moturi]

Fix checked in

Comment 32

[Srilatha Moturi]

*** Bug 84577 has been marked as a duplicate of this bug. ***

Comment 33

[yulian chang]

Verified with 2001062204 builds on all platforms.