Closed Bug 187454 Opened 22 years ago Closed 1 month ago

Autocomplete form fields from Address Book

Categories

(Camino Graveyard :: OS Integration, enhancement)

Product:
Camino Graveyard
Component:
OS Integration
Platform:
All
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: calum, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

Working Patch
16.52 KB, patch
stuart.morgan+bugzilla
: review-
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera/0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera/0.6

When typing text in to any text field, Chimera should look up what has just been
typed in and see if it matches any of the contents of the users Address Book
(eg. name, email address, postal address etc.). If a match is found, the match
should appear in the text field so the user does not have to type the rest. 

Reproducible: Always

Steps to Reproduce:
Attached file Proposed patch (obsolete) — 
This patch autocompletes all of the users details, and their friends email
addresses only. This reduces overhead, but I have not been able to test it on a
large Address Book yet. 

Autocompleting friends email addresses is included as it may be useful for
webmail applications.
This is an excellent idea to get immediate workability with auto-complete. And
hopefully at some point, the user will be able to add arbitrarily to the
auto-complete list without having to open the address book and editing the "me"
card and naming each value?
All this patch does is complete text that is already in the Address Book. If you
want to have properties other than name, email address etc. available then what
are they? This may be something that Apple should implement rather than the
Chimera team. 

I don't think Chimera should fill in your postal address etc. based on what you
have typed in a form, if you want to add that data use the Address Book
application. That is what it is for, and duplicating it in Chimera is just more
bloat. 
Confirming to get the patch looked at.
Status: UNCONFIRMED → NEW
Ever confirmed: true
The patch (110501) implements an algorithm in which the typed text is first
compared with the properties of the "Me" person and if there is no match, it
searches all the other Address Book entries. I am asking myself if it wouldn't
be less confusing to search only through the data of the "Me" person.
Although the patch does search through all the other Address Book entries, it
only searches for their email addresses. Like I said, I imagined this would be
useful for webmail applications (eg. hotmail.com or webmail.mac.com). 
Is there anything holding up this patch? Forgive me if I seem too pushy (I know
those who can commit this are busy). 
->pink

Calum: it would help if you could describe here what the patch does in some
detail, and how autocomplete behaves with the patch. That will help us to decide
whether it is the correct approach.

When working on any big patch like this, it's best to keep us in the loop, to
reduce the big-patch-brainprint at the end.
Assignee: saari → pinkerton
The patch listens for form changed events - the main class I added
(CHFormTextListener)  is a subclass of nsIDOMFormListener. 

When a text field in a form changes (that is not a password field), the value of
the text field is used to search for values in the AddressBook 'me' record. Some
of the properties of the record are ignored, such as the kABUIDProperty and
kABCreationDateProperty because the user probably never wants to see them. 

If no results are found in the 'me' record, the email addresses of the other
records are searched (I am not sure about the performance impact of searching
through all of the properties in a very large Address Book). 

When a result is found, the extra text is appended to the end of the text field
and selected, e.g.:

"Cal"
-> search for values beginning with Cal (case insensitive)
"Calum Robinson"
"calumr@mac.com"
etc...
-> First result is used for completion
->"um Robinson" appended to text field
-> "um Robinson" selected in text field

If the search for matching values returns nothing, then a flag is set that stops
CHFormTextListener from trying to autocomplete values (until the user deletes
some text or the text field loses focus). 
#import <AddressBook/AddressBook.h>

what does all this do on 10.1? just fail gracefully or does it require linkage
to a separate addressbook framework that will cause us to not start up?
Woops - I forgot about that. It looks like this code would have to be in a
separate executable/bundle (in order to prevent a crash on 10.1):

http://cocoa.mamasam.com/MACOSXDEV/2002/08/2/43027.php

The bundle would then have to be conditionally loaded. Does this rule out this
patch then? 
Comment on attachment 110501 [details]
Proposed patch

correct. we want a patch that works on 10.1 and .2 (fwiw, that's the whole
reason we've been holding off on doing this work ourselves).
Attachment #110501 - Flags: review-
Would it be acceptable to have a separate executable (there would have to be a
new target/subproject created for this) that gets called if the user is running
>10.2? Or do you want to wait until requiring 10.2 becomes more feasible? 

I would be happy to implement the separate executable idea. 
We don't want to separate executables.
Sorry - what I meant was a separate file in the Chimera.app/Contents/MacOS/
directory called AddressBookHandler (or whatever) that only gets loaded if the
user is running >10.2, containing the AddressBook code. I didn't mean 2 versions
of Chimera :o
There a runtime test in Cocoa to detect system version, that can be used in if
statements and the like. No need to load bundles. However, ugly code may result.
No - you can't do that. If an application links to the Address Book framework it
will fail to launch on 10.1 and below. You can't just do if (10.2) { blah... }. 
I think I'm out of synch (did we have mail about this on the list?) but I think
that even frameworks can be treated as bundles, so instead of linking it in the
project, it can be dynamically loaded at runtime with NSBundle methods.
I just tried Longship which contains the autofill patch and I must admit it's
very handy. Innitially I was looking for the form autofill button or menu item
like Safari v67 uses, but the way this patch works is even more elegent and
usefull. Although I do think the various parts of this patch should at least be
enabled or disabled via a preference (pane?). So we would be able to specify if
we want to autofill/autocomplete from me/autocomplete from other data/all at the
same time.


I believe what might have to be done is use NSFileManager to check for the
existence of the AddressBook.framework.  If it does indeed exist, use NSBundle
to load it.  This should let it run in both 10.1 and 10.2.  

Then, someone just needs to create a prefPane that allows you to enter in custom
values.  That prefPane should also have a checkbox that allows you to use your
address book entry.  That checkbox should be disabled (or maybe not even appear)
for 10.1 users.
Component: General → OS Integration
QA Contact: winnie → petersen
i think we should be able to take this once we put 10.1 support to bed, probably
after 0.8.
Target Milestone: --- → Camino0.9
Since David made and Addressboke mebeding for the new bookmareks system, would
it be possible to use that in this feature?
Calum,

Could you please update, or anyone else for that matter. Camino wil stop
supporting 10.1 after 0.8 so it might be interesting to get this patch ready for
that. It would be nice to have any kind of autofill active as soon as possible

Another thing is that the new Bookmarks manager ( Bug 212630 ) that will be
checked in soon also uses the Address Book API, specially for this there was a
AddressBookManager.bundle created. Perhaps this patch could be modified to use
that bundle? Just a question form a non programmer.


Note that the 0.8 branch are now the only camino builds that have to work on
10.1.  We could now accept a patch that relies on the address book framework (at
least the bits of it supported by 10.2).
I hate to do do this, but we need to release

target 0.9 -> 1.0
Target Milestone: Camino0.9 → Camino1.0
I'm sorry, hasn't this been checked in? I thought form fill was checked in a
while ago and that it drew information from the address book. Mike?
Nope this is another kidn of feature. What this does is autocomplete anything
you type in a form input field, the way the url bar does, using your addresbook
entries aswwell as email adddress from other people. 2 years ago I once had a
build contianing this and it was awesome. To bad nobody cares.
Jasper: Do you know which build it was?

If this feature existed, how hard is it to reimplement and clean up?
Scratch that. Is the patch still valid?
Target Milestone: Camino1.0 → Camino1.2
*** Bug 308225 has been marked as a duplicate of this bug. ***
Attached patch Working Patch — — Splinter Review 
I got this up to date and running, reformatting the code to comply with our coding guidelines.  I tested, and it seems to work well as expected.  I'm happy to make any stylistic or specific changes, but I can't claim to understand everything that's going on here, so any large conceptual changes should wait for follow-up bugs (or anybody else is welcome to make them if they have the desire).  This is a commonly requested feature (and it's awesome in practice), so I think we should try to go ahead with this.
Assignee: mikepinkerton → stridey
Attachment #110501 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #228162 - Flags: review?(stuart.morgan)
Sorry, forgot to mention.  I have Xcode 2.3, so somebody else is going to have to diff the project file (I just put the header in the headers folder, and the mm file in Classes -> Form Fill).
Comment on attachment 228162 [details] [diff] [review]
Working Patch

This code is way too inefficient for something that's going to be called as often as this is.  It looks like most of this is a self-rolled version of what searchElementForProperty:label:key:value:comparison:/searchElementForConjunction:children: do, but I'm sure it's done *way* more efficiently in AB (i.e., not linear in the number of records, and not repeatedly using string comparisons that create and destroy temporary objects).
Attachment #228162 - Flags: review?(stuart.morgan) → review-
Assignee: stridey → nobody
Status: ASSIGNED → NEW
QA Contact: chrispetersen → os.integration
Blocks: 319792
Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.
Target Milestone: Camino1.6 → ---
Ian, are you planning to finish this one? I am very interested in implementing this, just thought I'd ask first.

However, I think the search should be case sensitive. For example, I would like to type "m" for my e-mail adress and "M" for my name. Otherwise, I would have to type seven characters ("markus." or "Markus ") to differentiate between these two cases. This is probably a common case as lots of people have an e-mail adress starting with their name.
Be my guest.  I think case sensitivity would probably just confuse people though.
Attached patch WIP1 (obsolete) — — Splinter Review 
Here's a new approach loosely based on the previous patch by Calum/Ian. Notes:

* Case sensitive, which in my opinion is more useful (see comment #36)
* Only matches against properties with an obvious string values (like strings and numbers)
* Uses a small subset of specific properties when searching, and these are the same for "me" and other contacts
* TODO: Performance can probably be improved
* TODO: Should autocomplete full name when a match for first name is found
* I have minimal experience with C++
Attachment #296780 - Flags: review?(hwaara)
--> Markus
Assignee: nobody → markus.magnuson
Comment on attachment 296780 [details] [diff] [review]
WIP1

>Index: camino/src/embedding/CHBrowserView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/embedding/CHBrowserView.mm,v
>retrieving revision 1.99
>diff -u -8 -r1.99 CHBrowserView.mm
>--- camino/src/embedding/CHBrowserView.mm	3 Jan 2008 19:26:18 -0000	1.99
>+++ camino/src/embedding/CHBrowserView.mm	13 Jan 2008 02:27:05 -0000
>@@ -36,16 +36,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #import "NSString+Gecko.h"
> #import "NSPasteboard+Utils.h"
> #import "NSDate+Utils.h"
> 
> #import "CHSelectHandler.h"
>+#import "CHFormTextListener.h"
> 
> #include "nsCWebBrowser.h"
> #include "nsIBaseWindow.h"
> #include "nsIWebNavigation.h"
> #include "nsIWebBrowserSetup.h"
> #include "nsIWebProgressListener.h"
> #include "nsComponentManagerUtils.h"
> #include "nsIDocCharset.h"
>@@ -242,23 +243,29 @@
>     PRBool tempBool = PR_TRUE;
>     pref->GetBoolPref("print.use_global_printsettings", &tempBool);
>     mUseGlobalPrintSettings = tempBool;
> 
>     // hookup the listener for creating our own native menus on <SELECTS>
>     CHSelectHandler* selectHandler = new CHSelectHandler();
>     if (!selectHandler)
>       return nil;

So, I just noticed this... Are we leaking selectHandler for each new CHBrowserView??? It looks like we are. I think it needs to go in an instance variable that we delete in destroyWebBrowser. Can someone (Mento? Stuart?) confirm this?

>-
>+    
>+    // hookup the listener for autocomplete in form fields
>+    CHFormTextListener* formTextListener = new CHFormTextListener();
>+    if (!formTextListener)
>+      return nil;
>+    

Same here. (A 'new' but no corresponding 'delete'...)

>Index: camino/src/formfill/CHFormTextListener.mm
>===================================================================
>RCS file: camino/src/formfill/CHFormTextListener.mm
>diff -N camino/src/formfill/CHFormTextListener.mm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ camino/src/formfill/CHFormTextListener.mm	13 Jan 2008 02:27:05 -0000
>@@ -0,0 +1,265 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+* Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+*
>+* The contents of this file are subject to the Mozilla Public License Version
>+* 1.1 (the "License"); you may not use this file except in compliance with
>+* the License. You may obtain a copy of the License at
>+* http://www.mozilla.org/MPL/
>+*
>+* Software distributed under the License is distributed on an "AS IS" basis,
>+* WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+* for the specific language governing rights and limitations under the
>+* License.
>+*
>+* The Original Code is mozilla.org code.
>+*
>+* The Initial Developer of the Original Code is
>+* Netscape Communications Corporation.
>+* Portions created by the Initial Developer are Copyright (C) 2002
>+* the Initial Developer. All Rights Reserved.

Please fix this to be correct while you're here (both new files). 2008 and probably not Netscape as initial developer :-). There's a MPL boilerplate at mozilla.org you can copy if you want the latest one.

>+// Complete the string by looking at values in the user's Address Book
>+NSString* CHFormTextListener::AutocompleteString(NSString* input)
>+{
>+  // Construct an array of ABSearchElements for the properties we want to search
>+  NSMutableArray *searchElements = [NSMutableArray array];
>+  unsigned i;
>+  for (i = 0; i < [interestingProperties count]; i++) {
>+    ABSearchElement *newSearchElement =
>+    [ABPerson searchElementForProperty:[interestingProperties objectAtIndex:i]
>+                                 label:nil
>+                                   key:nil
>+                                 value:input
>+                            comparison:kABPrefixMatch];
>+    
>+    [searchElements addObject:newSearchElement];
>+  }

This is essentially a for-each, so you should use a NSEnumerator.

>+  
>+  // Combine into one search element
>+  ABSearchElement *searchConjunction = [ABSearchElement searchElementForConjunction:kABSearchOr children:searchElements];
>+  
>+  NSArray *peopleFound = [[ABAddressBook sharedAddressBook] recordsMatchingSearchElement:searchConjunction];
>+  if ([peopleFound count] > 0) {
>+    // Pick the first match and return its first matching property
>+    ABPerson *firstMatchingPerson = [peopleFound objectAtIndex:0];
>+    id currentValue = nil;
>+    for (i = 0; i < [interestingProperties count]; i++) {

for-each here too

>+      currentValue = [firstMatchingPerson valueForProperty:[interestingProperties objectAtIndex:i]];
>+      if (currentValue) {

No need for this; [currentValue isKindOfClass:] below will be false if currentValue is nil.

>+        if ([currentValue isKindOfClass:[ABMultiValue class]]) { // We got multiple values
>+          ABPropertyType currentPropertyType = [currentValue propertyType];
>+          switch (currentPropertyType) {
>+            case kABMultiStringProperty:
>+            {
>+              unsigned a;
>+              for (a = 0; a < [currentValue count]; a++) {
>+                if ([[currentValue valueAtIndex:a] hasPrefix:input])
>+                  return [currentValue valueAtIndex:a];
>+              }
>+              break;
>+            }

Is it above that you want to prioritize full name? 

One method I noticed that might or might not be interesting: ABPerson's identity message, which returns an CSIdentityRef, which has lots of interesting messages: like fullName, etc.

Oh, and for-each it!

>+              
>+            case kABMultiIntegerProperty:
>+            case kABMultiRealProperty:
>+            {
>+              unsigned a;
>+              for (a = 0; a < [currentValue count]; a++) {
>+                if ([[[currentValue valueAtIndex:a] stringValue] hasPrefix:input])
>+                  return [[currentValue valueAtIndex:a] stringValue];
>+              }
>+              break;
>+            }

When is this used? Is it for phone numbers? for-each it!

>+              
>+            case kABStringProperty:
>+            case kABIntegerProperty:
>+            case kABRealProperty:
>+            case kABErrorInProperty:
>+            case kABDateProperty:
>+            case kABArrayProperty:
>+            case kABDictionaryProperty:
>+            case kABDataProperty:
>+            case kABMultiDateProperty:
>+            case kABMultiArrayProperty:
>+            case kABMultiDictionaryProperty:
>+            case kABMultiDataProperty:
>+              break;
>+          }
>+        }
>+        else { // We got a single value
>+          if ([currentValue isKindOfClass:[NSString class]]) {
>+            if ([currentValue hasPrefix:input])

Please merge the above two if statements into one

>+              return currentValue;
>+          }
>+          else if ([currentValue respondsToSelector:@selector(stringValue)]) {
>+            if ([[currentValue stringValue] hasPrefix:input])

Same ere.

>+              return [currentValue stringValue];
>+          }
>+        }
>+      }
>+    }
>+  }
>+  
>+  return nil;
>+}

>+class CHFormTextListener : public nsIDOMFormListener
>+{
>+public:
>+  CHFormTextListener();
>+  virtual ~CHFormTextListener();
>+  NSString* CHFormTextListener::AutocompleteString(NSString* input);

'CHFormTextListener::' is not needed in the declaration.

It's not clear whether this method will return an autoreleased, or alloc'd string... this is always ambigious when mixing C++ and ObjC (we've had this problems lots of times in widget/cocoa), so I suggest changing name to AutoreleasedAutocompleteString to avoid any future bugs.

>+
>+  NS_DECL_ISUPPORTS
>+
>+  // The DOM form listener interface.  We only care about change events.
>+  NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
>+  NS_IMETHOD Submit(nsIDOMEvent* aEvent);
>+  NS_IMETHOD Reset(nsIDOMEvent* aEvent);
>+  NS_IMETHOD Change(nsIDOMEvent* aEvent);
>+  NS_IMETHOD Select(nsIDOMEvent* aEvent);
>+  NS_IMETHOD Input(nsIDOMEvent* aEvent);
>+
>+private:
>+  nsCOMPtr<nsIDOMHTMLInputElement> lastTarget;
>+  PRUint32 lastInputLength;
>+  PRBool lastSearchEmpty;
>+  NSArray *interestingProperties;

Maybe these members should have a 'm' prefix -- what's the style used in the other CH* classes?
Attachment #296780 - Flags: review?(hwaara) → review-
> So, I just noticed this... Are we leaking selectHandler for each new
> CHBrowserView??? It looks like we are. I think it needs to go in an instance
> variable that we delete in destroyWebBrowser. Can someone (Mento? Stuart?)
> confirm this?

I thought we decided in some earlier bug that the ownership was being transfered with AddEventListenerByIID, but now that we call that twice with the same handler I'm not sure how that could be true without it crashing...
(In reply to comment #41)
> > So, I just noticed this... Are we leaking selectHandler for each new
> > CHBrowserView??? It looks like we are. I think it needs to go in an instance
> > variable that we delete in destroyWebBrowser. Can someone (Mento? Stuart?)
> > confirm this?
> 
> I thought we decided in some earlier bug that the ownership was being
> transfered with AddEventListenerByIID, but now that we call that twice with the
> same handler I'm not sure how that could be true without it crashing...
> 

OK, I debugged this to convince myself that we (thankfully) aren't leaking anything. 

The event listener manager (to which we are listening) is (for each call) retaining our listener object. The event listener manager in question is basically owned by the DOM window, which is owned by our nsIWebBrowser, which is destroyed properly when the window is closed.
Attached patch WIP2 (obsolete) — — Splinter Review 
(In reply to comment #40)
> Please fix this to be correct while you're here (both new files). 2008 and
> probably not Netscape as initial developer :-). There's a MPL boilerplate at
> mozilla.org you can copy if you want the latest one.

Fixed. 2002 is the correct year since that's when the original patch is from.

> This is essentially a for-each, so you should use a NSEnumerator.

Fixed.

> for-each here too

Fixed.

> No need for this; [currentValue isKindOfClass:] below will be false if
> currentValue is nil.

Fixed.

> Is it above that you want to prioritize full name? 
> 
> One method I noticed that might or might not be interesting: ABPerson's
> identity message, which returns an CSIdentityRef, which has lots of interesting
> messages: like fullName, etc.
> 
> Oh, and for-each it!

Full name is constructed in the return clause, in the new patch. The identity method is only available in 10.5, so I am not using it.

> >+            case kABMultiIntegerProperty:
> >+            case kABMultiRealProperty:
> >+            {
> >+              unsigned a;
> >+              for (a = 0; a < [currentValue count]; a++) {
> >+                if ([[[currentValue valueAtIndex:a] stringValue] hasPrefix:input])
> >+                  return [[currentValue valueAtIndex:a] stringValue];
> >+              }
> >+              break;
> >+            }
> 
> When is this used? Is it for phone numbers? for-each it!

It is for any properties that can have multiple values, for example a contact may have multiple e-mail addresses. Such a property will then be returned by AB as an ABMultiValue, which by the way can only be accessed by index.


> Please merge the above two if statements into one

Fixed.

> Same ere.

Fixed.

> 'CHFormTextListener::' is not needed in the declaration.
> 
> It's not clear whether this method will return an autoreleased, or alloc'd
> string... this is always ambigious when mixing C++ and ObjC (we've had this
> problems lots of times in widget/cocoa), so I suggest changing name to
> AutoreleasedAutocompleteString to avoid any future bugs.

Fixed.

> Maybe these members should have a 'm' prefix -- what's the style used in the
> other CH* classes?

Fixed.
Attachment #296780 - Attachment is obsolete: true
Attachment #297141 - Flags: review?(hwaara)
Comment on attachment 297141 [details] [diff] [review]
WIP2

This looks OK code-wise to me.

It kinda sucks that the only way Cocoa (at least pre-10.5) exposes the full name of a person is for us to concatenate firstName and lastName. There are so many cultures where this assumption about "full name" is not true...
Attachment #297141 - Flags: review?(hwaara) → review+
(In reply to comment #44)
> It kinda sucks that the only way Cocoa (at least pre-10.5) exposes the full
> name of a person is for us to concatenate firstName and lastName. There are so
> many cultures where this assumption about "full name" is not true...

Yes, the only alternative would be to not complete full name, only first name and last name.
Attachment #297141 - Flags: review?(stuart.morgan)
Stuart, should I ask someone else for review? You seem to have quite some queue already.
Doesn't this plus the new password code give us two completely different systems that listen for, and potentially manage, input to text fields? What happens when you start typing in a username field, and both want to take over?

It also gives us two behaviors in the face of multiple matches; logins will have a choice, but this will just pick the first.

Can we combine these into one system that watches all typing and has consistent UI, and special-cases (or delegates out) login fields?
I planning another go at this one. For reference, bug 178607 is a starting point.
Attachment #297141 - Attachment is obsolete: true
Attachment #297141 - Flags: review?(stuart.morgan)
Assignee: markus.magnuson → nobody
Can someone clearly explain a use-case for this feature *other than* allowing Address Book integration with webmail? Because that's really the only thing I see discussed here, and I'm fairly certain most popular webmail these days allows a person to import an address book. Unless you're one of those people who's totally opposed to storing contact information in a webmail interface, I don't really see what's so awesome about this feature. What am I missing?
Hardware: PowerPC → All
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: