28769 - Auto-complete server names based on e-mail address

Status: ASSIGNED

(SeaMonkey :: MailNews: Account Configuration, enhancement)

Description

[Matthew T (active 1999-2002)]

This broke out of bug 28589.

When a user is entering the incoming and outgoing server names in the account 
manager, the fields should auto-complete based on the domain part of their e-mail 
address (which they have just entered in the previous page of the wizard).

For example, if I have entered my e-mail address as [e-mail], and I then start 
typing `imaphost.f' in the incoming mail server field, Mozilla should
auto-complete with `oo.bar'.

Comment 1

[Matthew T (active 1999-2002)]

[Changing helpwanted flag to keyword, as per bug 28345.]

Comment 2

[James Simonsen]

Attachment: Implemented via onkeyup()

Adds a function that is triggered by keyup and autocompletes if possible.

Comment 3

[James Simonsen]

Attachment: Corrected patch

Only autocompletes on first char or after a '.' (correctly now).

Please make old patch obsolete.

Comment 4

[Ian Neal]

Attachment: Updated patch against current trunk

Updated against current trunk but for some reason doesn't work from chrome but
does from file

Comment 5

[Ian Neal]

Current patch has some diagnostic dumps in but they don't seem to generate
anything :-(

Comment 6

[Ian Neal]

Attachment: Patch v1.1

Patch against AccountWizard.xul rather than aw-server.xul

Comment 7

[Ian Neal]

Reassigning to me

Comment 8

[Ian Neal]

Accepting

Comment 9

[[e-mail]]

Comment on attachment 121826 [details] [diff] [review]
Patch v1.1

>+  // only autocomplete on characters
>+  if (keyCode < 48)
>+    return true;
Better to test for a nonzero charCode.

>+  // only autocomplete on first char or after a '.'
Actually this code doesn't (and perhaps should not) complete on the first char,
since it only autocompletes if a '.' is found. You could use a do loop to test
the whole servername first.
>+
>+  var shortname = servername;
>+  var j = shortname.indexOf('.') + 1;
>+  while (j > 0 && j < shortname.length)
Perhaps you are trying not to autocomplete after a '.' is typed... even if
that's the intent you should still check charCode above.

>+  {
>+    shortname = shortname.substring(j, shortname.length);
Don't need 2nd parameter to substring here or elsewhere when all you want is
the rest of the string

>+    if (emailArray[1].substring(0, shortname.length) == shortname) {
Should this be a case-insensitive match? (lowercase the email address and
shortname before the loop)

>+      servernameElement.select();
Don't need this, you're setting the selection again

Comment 10

[Ian Neal]

If I was to check for non zero charCode I couldn't use onkeyup as that only
seems to generate zero charCodes I'd have to use onkeypress instead. That would
introduce it's own problems in that at the time of that event the character will
not have been added to the textbox. Updated function, keeping with onkeyup,
would look something like this:

function serverKeyup(keyCode, name)
{
  // only autocomplete on characters
  if (keyCode < 33 || keyCode > 126 || keyCode == 46)
    return true;

  var servernameElement = document.getElementById(name);
  var servername = servernameElement.value.toLowerCase();

  var pageData = parent.GetPageData();
  var emailArray = pageData.identity.email.value.split('@');
  var email = emailArray[1].toLowerCase();

  // only autocomplete on first char after a '.'

  var shortname = servername;
  var j = shortname.indexOf('.') + 1;
  while (j > 0 && j < shortname.length)
  {
    shortname = shortname.substring(j);
    if (email.substring(0, shortname.length) == shortname) {
      servernameElement.value = servername + email.substring(shortname.length);
      servernameElement.selectionStart = servername.length;
      servernameElement.selectionEnd = servernameElement.value.length;
      return true;
    }
    j = shortname.indexOf('.') + 1;
  }

  return true;
}

Comment 11

[Ian Neal]

Attachment: Patch v1.2 using function as described above

Comment 12

[Ian Neal]

Attachment: Patch v1.3

Goes back to the do loop structure of James' original patch

Comment 13

[[e-mail]]

Comment on attachment 122515 [details] [diff] [review]
Patch v1.3

I'm not sure you don't want to autocomplete after a "." - say my email was
[e-mail] then after I'd typed mail.bbc. I'd expect to see "co.uk"
autocompleted.

I've peeked into how "regular" autocomplete does it, it uses an onkeypress to
save the last key and an oninput to do the dirty work.

Also, I don't see the point of passing an id to serverKeyup when you can pass
the element itself (in the inline event handler the "this" keyword refers to
it).

Comment 14

[Ian Neal]

this seems to be [object ChromeWindow] when called from onkeypress, is that right?
Autocomplete seems to work as you suggested already.

Comment 15

[[e-mail]]

I was wondering why you tested for key == 46 (I haven't had time to try it).

The meaning of "this" depends on context - in <element
onkeyup="doSomething(this);"> it's the element, but in function doSomething(arg)
{ dump(arg); dump(this); } it's the window.

Comment 16

[[e-mail]]

Comment on attachment 122515 [details] [diff] [review]
Patch v1.3

>+  // only autocomplete on characters
>+  if (keyCode < 33 || keyCode > 126 || keyCode == 46)
>+    return true;
>+
>+  var servernameElement = document.getElementById(name);
>+  var servername = servernameElement.value.toLowerCase();
>+
>+  var pageData = parent.GetPageData();
>+  var emailArray = pageData.identity.email.value.split('@');
>+  var email = emailArray[1].toLowerCase();
>+
>+  // only autocomplete on first char or after a '.'
Except you don't autocomplete after a '.' because you exclude it above :-(

Comment 17

[Alec Flett]

+  // only autocomplete on characters
+  if (keyCode < 33 || keyCode > 126 || keyCode == 46)
+    return true;

these are just magic numbers to me. What do they refer to? Looks like non-ascii
characters, so what do you do for non-ASCII based languages? What happens in
japanese? 
(and yes, hostnames can be internationalized, a la iDNS and such, which we support)

Comment 18

[Ian Neal]

Attachment: Patch 1.3a

Element is now passed using 'this' and keycodes less than 33 are the only ones
ignored.

Comment 19

[Alec Flett]

what special signifigance does "33" hold? I'm not doubting the signifigance of
control characters, but your code ought to be self-documenting here - offhand I
know that "32" is a space character, but 33 is just a magic number to me. You
shoudl use some of the DOM constants if you're going to be checking this kind of
thing.

Comment 20

[Ian Neal]

Looking at changing those lines to:

+  // don't autocomplete when non character keys (e.g. tab or delete) are pressed
+  if (keyCode <= KeyEvent.DOM_VK_DELETE || keyCode == KeyEvent.DOM_VK_META ||
+     (keyCode >= KeyEvent.DOM_VK_F1 && keyCode <= KeyEvent.DOM_VK_SCROLL_LOCK))
+    return true;

Less magic numbery?

Comment 21

[Alec Flett]

yes, this is exactly what I was referring to! thanks.

Comment 22

[Ian Neal]

Attachment: Patch v1.3b

Revised patch checking for DOM_VK rather than 'magic' numbers.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 122957 [details] [diff] [review]
Patch v1.3b

sr=[e-mail]

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 122957 [details] [diff] [review]
Patch v1.3b

since this is mail, I'd prefer to give the a (and test this.)

I want to test, because I'm a little nervous about doing home grown
autocomplete here.

for example, it isn't on a timer, which leads to a bad experience.

let me test and review before approval, ok?

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 122957 [details] [diff] [review]
Patch v1.3b

in addition to dmose's review, I'd like to review as well.

so making an official request.

Comment 26

[Serge Gautherie (:sgautherie)]

Ian, sspitzer,
Are you still working on this ?

Comment 27

[Ian Neal]

Waiting for some sort of super-review, I suspect at some point I'll be asked to tweak the patch.

Comment 28

[Wayne Mery (:wsmwk)]

Iann comment #27
> Waiting for some sort of super-review, I suspect at some point I'll be asked to
> tweak the patch.

sr from spitzer?