Closed Bug 43189 Opened 20 years ago Closed 19 years ago

new autocomplete widget and hookup to global history

Categories

(SeaMonkey :: Location Bar, defect, P3, major)

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: god, Assigned: hewitt)

References

Details

Attachments

(9 files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.17pre3 i686; en-US; m16) Gecko/20000619
BuildID:    2000061920

The autocompletion feature does not autocomplete urls which were reached by
following links, as it is done in NS4.x and IE>=4.

Reproducible: Always
Steps to Reproduce:
1.click on a link
2.try typing in the link manually


Actual Results:  no autocompletion is done

Expected Results:  as in NS4, those URLs should be autocompleted too
Taking. No clue when I will be able to get to it. Not sure if we want to keep 
this Nav 4.x behavior in mozilla. cc'ing johng for comments
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: no autocompletion on urls reached by clicking links → [Feature]no autocompletion on urls reached by clicking links
Target Milestone: --- → Future
If it is an easy fix that you might get to anyway when fixing bugs in 
autocomplete, and you feel strongly about it, then go ahead and nominate 
nsbeta3.  Otherwise, ignore it and leave the milestone as "Future" - I'm 
comfortable shipping without this feature.
Summary: [Feature]no autocompletion on urls reached by clicking links → [RFE]no autocompletion on urls reached by clicking links
nav triage team: looked at this bug, it is not a beta stopper. bulk update of 
several such bugs. 
Keywords: nsbeta1-
moving these bugs to History: URLBar
Assignee: radha → alecf
Status: ASSIGNED → NEW
Component: History: Session → History: URLBar
I think the summary should be changed.  It seems like he's requesting that there
be no autocompletion for clicked links.  I find that this functionality would be
useful.
Also, would a way to accomplish this feature be to compare the typed text to the
urls in history?
changing summary so that it reflects the wanted feature...
Summary: [RFE]no autocompletion on urls reached by clicking links → [RFE] autocompletion on urls reached by clicking links
Added "(link url bar autocomplete to global history)" to summary which I think
correctly reflects what bug is about. Alec mentioned looking into this when we
talked yesterday, well, congrats, it's on your 0.9 plate. ;-) Marking nsbeta1+,
mozilla0.9
Keywords: nsbeta1-nsbeta1+
Summary: [RFE] autocompletion on urls reached by clicking links → [RFE] autocompletion on urls reached by clicking links (link url bar autocomplete to global history)
Target Milestone: Future → mozilla0.9
Attached patch pre-fix cleanupSplinter Review
before I do any urlbar work, I wanted to clean this file up a fair amount...
looking for reviews. 
Status: NEW → ASSIGNED
    res = nsServiceManager::GetService(kRDFServiceCID,
NS_GET_IID(nsIRDFService),
-                                                   (nsISupports
**)&gRDFService);
+                                      (nsISupports **)&gRDFService);
    res = nsServiceManager::GetService(kRDFCUtilsCID,
NS_GET_IID(nsIRDFContainerUtils),
-                                                   (nsISupports
**)&gRDFCUtils);
+                                      (nsISupports **)&gRDFCUtils);

It looks like |gRDFService| is only used in the constructor, so no need to have
it as a global static. |gRDFCUtils| is used in a few places, but why have it as
a static instead of as a member variable and make it a nsCOMPtr (and use
do_GetService)?



+   nsCOMPtr<nsIPref> prefs = do_GetService(kPrefServiceCID, &res);

Shouldn't that be NS_PREF_CONTRACTID so you can get rid of the CID?
Same for |NS_RDF_CONTRACTID "/rdf-service;1"| and |NS_RDF_CONTRACTID
"/container-utils;1"| (ugh, is there a bug on converting those to #defines?)...



+nsUrlbarHistory::SearchCache(nsAReadableString& searchStr,

and

+nsUrlbarHistory::GetHostIndex(nsAReadableString& aPath, PRInt32 * aReturn)

add a |const|. Actually, when I tried compiling with the patch, gcc complained
about this in a rather odd way:

nsUrlbarHistory.cpp: In method `nsresult nsUrlbarHistory::OnStartLookup(const
PRUnichar *, nsIAutoCompleteResults *, nsIAutoCompleteListener *)':
nsUrlbarHistory.cpp:240: no matching function for call to
`nsUrlbarHistory::SearchCache (nsLiteralString, nsCOMPtr<nsIAutoCompleteResults>
&)'
nsUrlbarHistory.h:56: candidates are: nsresult
nsUrlbarHistory::SearchCache(nsAReadableString &, nsIAutoCompleteResults *)
<near match>



+       if ((const char*)preHost) 

I don't think the cast is needed (gcc doesn't need it, anyway).



-    if (!aItem || !aArray)
+    if (!aArray)

Why not check for aItem.IsEmpty() ?



+    printf("nsUrlbarHistory::OnStopLookup()\n");

and

+#if 1
+    printf("nsUrlbarHistory::OnAutoComplete(%s)\n",
NS_ConvertUCS2toUTF8(searchString));
+#endif

shouldn't those be wrapped in #ifdef DEBUG or DEBUG_alecf ?
Oh, and that NS_ConvertUCS2toUTF8 needs a .get().



+    if (nsCRT::strcmp(aTopic, NS_LITERAL_STRING("nsPref:changed").get())==0) {

How about:

     if (nsLiteralString(aTopic).Equals(NS_LITERAL_STRING("nsPref:changed"))) {
or
     if (NS_LITERAL_STRING("nsPref:changed").Equals(aTopic)) {
Hey Alec, have you done any work on this yet? If not, I can spare you.  I've got
this working now in my tree, but I need to polish it a bit.  Need to be smart
about the way hostnames are treated, and a bunch of other little details.  I'll
have a patch by monday, hopefully.
adding hewitt so I can answer his question (shame on you for asking a question
without adding yourself to the CC :))
nope, I haven't - I'm really whacking nsUrlbarHistory.cpp though, so if you're
touching that I'd prefer you wait until I land the above patch.
As far as I can tell, all of the autocomplete-related stuff in nsURLBarHistory
is useless at this point.  If we're going to hook autocomplete up to the global
history, that will all be done in nsGlobalHistory, and at that point the only
thing you'll need nsURLBarHistory for is to add/remove items in the datasource.
well, my eventual goal with this is to have the url bar store everything in
global history, by flagging certain urls as 'typed' (as in a url that the user
has typed into the browser) - then when we do autocomplete, we first match
against the 'typed' urls, and then agains the entire history.
I've reached a point in developing the new autocomplete widget where I'm happy
enough to check in what I have now, but think in the long run there are many
problems yet to resolve.  I have a forthcoming patch which is ready to be
reviewed (gently).  There are still a few outstanding issues with autocomplete
that this patch does not address, namely:

1. don't support multiple datasources yet
2. don't support incremental rebuilding of the list
3. need a speedy way to deal with large result sets

I've spoken to dmose a bit about these issues, as he has certain needs for LDAP
autocomplete.  It would be nice if we could be smarter about the way we build
the list.  Currently, we kick off an asynchronous search each time the user
enters a new character, and that search must callback just once with the entire
list of results.  Then, all menuitems are removed and new ones are added to the
list.  This can be slow for large lists, and currently I have a bit of a hack in
place which makes it faster in some situations (see "rangeOptimize" in the patch). 

What I would really love to be able to do is use the outliner widget to solve
these problems.  Because the slowest part of autocomplete is not the actual
search, but the building of the content model, it would be swell if we could use
outliner within a popup to solve this.  I ran a test and found out that outline
doesn't like popups -- it crashes when you try to open a popup which contains an
outliner.

On the bright side, if you apply this patch you'll see autocomplete kicks so
much more ass than it did before, so it's a worthy improvement even if it's not
perfect.  I think we could get away with pushing the above issues off to a
future milestone.

As if I haven't written enough already, here's a list of the things that the
following patch accomplishes:

-- popup spans the entire textfield
-- popup stays open while you type, and updates immediately
-- a scrollbar appears if the results list is too long
-- up/down/pageup/pagedown navigation works for scrollable lists
-- hitting escape while keying through results reverts to previous typing
-- option to fill and select text as user types (used in msgcompose, but not urlbar)
-- ability to filter user-entered text before passing it to the autocomplete session
-- built in support for (optional) proxy icon
-- built in support for (optional) "history" menubutton (which spans entire
textfield)
-- the textbox context menu is shared via an XBL binding (should be done for all
textbox bindings too)
-- widget used in Navigator urlbar, Open Location Dialog (new), msg compose, and
abook mailing list dialog
-- in Navigator, hooked up urlbar autocomplete to the global history
-- in Navigator, the autocomplete results list is sorted alphabetically
-- in Navigator, search engine option appears at the bottom of the popup
-- in Navigator, clicking on a url in the list automatically loads it
-- in Navigator, search is smart about chopping off "http://" and "www." before
matching, e.g.
   typing "cnn" will match "http://www.cnn.com/news/blah.html"
-- in Navigator, the proxy icon disappears while the user is typing

wow, that is pretty kickass but one question that came up in another bug somewhere:

"-- in Navigator, clicking on a url in the list automatically loads it"

suppose I've already been to http://www.foobar.com and now i would like to get to http://
www.foobar.com/somepage.html?

Imagine foobar is some long annoying URL. When i start typing  foo... i want it to complete
but then i want to add to it. How do i do that? with the mouse? with the keyboard? Just asking
Blocks: 68890
Blocks: 41811
taking this one
Assignee: alecf → hewitt
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Summary: [RFE] autocompletion on urls reached by clicking links (link url bar autocomplete to global history) → new autocomplete widget and hookup to global history
Had a look at this.
Things look good over all.
Need to make sure you add a patch for the jar.nm
It was missing for all of your new files.

Also was not able to get the URL bar to show up in the
classic theme.  I think it was my build but can you verify that
it works in your build.

Other then that it looks good.
I've got one more patch coming up with includes the following enhancements:

1. the patch should actually apply! (thanks alec)
2. support for multiple search sessions
3. no more stupid autocomplete menubutton
4. support for mac classic theme

These are the caveats I'm aware of:
1. autocomplete won't work in navigator until you load a page (seems the history
isn't loaded until then)
2. that's it!

I'd appreciate if lots of people could apply this patch and give it a test
drive. Since the urlbar is such a sensitive area of the product, I'd hate to
bust it for a day.  Try to get over the fact that the patch is 3000+ lines long
and 34 files wide. Thanks!
I would like to add that testing message compose is also very important :-)
Blocks: 56647
Blocks: 75507
Blocks: 68388
Blocks: 38409
Blocks: 38865
Blocks: 40305
Depends on: 58837
Blocks: 59534
Blocks: 67770
Depends on: 63421
No longer depends on: 58837, 63421
Blocks: 71153
I spent my entire day today stress-testing the autocomplete widget and I found a
few cracks in it's armor.  I focused particularly on making the message compose
autocomplete work smoothly.  Little annoying things would happen if you typed
too fast, tabbed or clicked out of the widget, etc... I think I've plugged all
those holes now, though. 

Additionally, in Navigator I wrote a little more code to ensure that the
page-proxy-button is invisible and not draggable when the user is typing a url.
 It returns to it's normal state when the value of the urlbar becomes a valid
page which had been loaded successfully.

I hate to keep posting patch after patch after patch to this bug... so I'll hold
off on posting my new patch until sunday or monday in case I tweak it more over
the weekend.  If you want the latest patch sooner than that, ping me.
Another late-breaking feature -- I just enabled navigator autocomplete to
display the title of the page in addition the the url of each autocomplete result.
Wow, hewitt is laying the smackdown on this bug today.  Question: is this just
Linux?  If not, should we change the OS to "All"?
Have you tested it agains Japanese input? That was a concern when I wrote the original autocomplete widget. You 
can easely test that on Mac. Mac OS 9 CD have all you need to install Japanese input support.
Nice work!  The multiple sessions thing works!  Here are some random comments
about the existing patch:

* the sessions' OnStopLookup()s never seem to get called at all.  This causes
problems with LDAP, because it expects (reasonably, I think) that any running
search will be stopped before a new one is begun.

* For the mail compose autocomplete, we'd like to be able to complete against
multiple addressbooks of a single type at a time  This has a couple of implications:

    * Each entry in mSessions should probably be an array.

    * The autocomplete session of a particular type may no longer be a service.
      In the case of LDAP, there would be one object implementing   
      nsIAutoCompleteSession and nsILDAPAutoCompleteSession per directory.
      In order to set the LDAP-specific params, I need to QI each session
      to nsILDAPAutoCompleteSession. Since I won't be able to get the session
      just with a getService() from JS like the current compose-window addrbook
      completion code does, mSessions would need to be a public property (like
      autoCompleteSession was in the old code) so that I can iterate over the 
      different LDAP sessions.

I would be really interested in continuing to see your up-to-the-minute patches,
as I'll be hacking on this over the weekend.  Seems to me like it would be
completely reasonable to keep attaching updated patches here; but email them to
me if you'd rather.

Anyway, this is great work!  I'm amazed and pleased that I was actually able to
get a composite addrbook/ldap session working right out of the box.  Thanks!
OS: Linux → All
Hardware: PC → All
Here comes my latest patch.  I've just tested on mac and linux today and
everything looks real good.  Well... almost.  One particular problem I found is
that when I retrieve the page title from the history mork db, on mac only I'm
seeing garbage at the end of the string, like "mozilla.org0??$??".  I don't know
too much about mork, but perhaps there's a different in the way strings are
stored on mac vs. windows?



I hate to move this to 0.9.1, but I think it's for the better.  I'm going to
take one last stab at improving this widget by re-writing the drop-down list
using outliner.  This change is absolutely necessary because I have found that
the time spent building the set of menuitems is significant on old systems, and
even debug builds on new systems.
Target Milestone: mozilla0.9 → mozilla0.9.1
is there a build with this patch available to the public?  I'd really like to
try this out but I don't have the time to cvs co/patch/build/etc.  any chance
that a build can be put in ftp.mozilla.org/pub/mozilla/nightly/experimental/?
Right now I'm working on re-writing the widget so that it uses outliner.  It's
just way too slow building all those menuitems.  It might take me a few days
before I have a new patch, depending on how many outliner bugs I turn up (2
already)... 
Depends on: 76465
Just an update -- outliner is cooperating nicely, and I've got the widget about
90% re-written.  There are two dependent bugs at this point, but I've got fixes
for both in my tree which I'll try to land soon. The good news is that the
performance boost I expected to get by switching to outliner has definitely
happened.  No more ui-freezes when you get a really long result list.  
Depends on: 76297
Depends on: 76428
Great!  Even a partially working patch would be very handy at this point,
because the CVS tip has now diverged enough that the most recent patch here no
longer works.  :-(
Depends on: 76894
Working on the outliner-conversion this week has been far less pleasant than I
had expected.  I have been continually tripping over layout bugs, and am wasting
a lot of time fixing these bugs that get in my way (see the 4 dependent bugs). 
Nevertheless, I think I'm almost done!  The thing works beautifully and I've
re-implemented about 95% of the functionality of the menuitem-based widget at
this point.  I'll post a "not done yet" patch shortly for the benefit of dmose
and others who might care.  In addition to this patch, you'll need to apply
patches from the 4 dependent bugs above.
The patch I'm about to submit actually works.  Really well.  Considering what a
disaster things were yesterday, I'm surprised.  Long live outliner!  So, go to
town all you eager reviewers! :) Remember, you have to apply the patches from
the 4 blocking bugs above (76297,76428,76465,76894).
comments on the global history part of the patch:
* you're leaking in nsGlobalHistory::AutoCompleteEnumerator::IsResult - I'm not
sure why you're doing this:
nsLocalString(url.ToNewUnicode());

you should just be able to pass in "url" - if you're running into
zero-termination issue, find me on AIM/IRC and we'll figure it out.

* In ConvertToISupports() you shouldn't need to use ToNewUnicode() here either,
you should just be able to say .get()

* when you're converting the enumerator to the nsISupportsArray, you have:
+    while (NS_SUCCEEDED(enumerator->GetNext(getter_AddRefs(entry)))) {
+      nsCOMPtr<nsIAutoCompleteItem> item = do_QueryInterface(entry, &rv);
+      if (NS_SUCCEEDED(rv))
+        array->AppendElement(item);
+    }

I think you should assume, since you own all the code that creates this
enumerator, that each element is an nsIAutoCompleteItem - so I wouldn't bother
with the extra QueryInterface, just dump it into the array.
Also, I'm pretty sure you should be using HasMoreElements, just to be consistent
with the API.

- I'm not sure I'm keen on the idea of converting the enumerator ->
nsISupportsArray -> flat array -> sort -> nsISupportsArray..  at the very least,
we're doing a LOT of ADDREF/RELEASEes to do this conversion. Might I suggest you
use nsAutoVoidArray instead of the first nsISupportsArray?
- this:
+    delete items;

should be
+    delete[] items;

I'm not sure I'm so happy about the http-specific logic in here, but I'm not
sure of a better solution at the moment. I'd like to see a protocol-independant
way of deciding if two urls are a "close match"


It also seems like we really need to have a getValueConst() in
nsIAutoCompleteItem, so that we don't have to keep allocating copies of the data
in there just to compare it to another string.

that's it for now, I'll revisit this on monday.
All the patches apply nicely, but upon trying to build, autocomplete.js appears
to be missing.  :-/
Blocks: 76028
Blocks: 51637
I've got one more Happy-Fun-Patch for all you kids to enjoy.  But remember,
kids, do not taunt Happy-Fun-Patch.  Do not look directly at Happy-Fun-Patch. 
And do not attempt to ignite Happy-Fun-Patch. 

It's way too late (or, early?) and I'm tired.

Note to Alec: I addressed several of your comments in the this revision.  When
you next attempt to apply/review this latest patch, grab me on AIM so I can go
over a few things with you (provided I'm awake during your work day).
Attached patch Happy-Fun-PatchSplinter Review
Bugs I noticed in the 04/23 experimental build on Win98
(http://ftp.mozilla.org/pub/mozilla/nightly/experimental/autocomplete/)

1. Dragging the scrollbar thumb in autocomplete dropdown: the thumb thinks my 
mouse cursor is ~30 pixels from where it actually is.
2. Dropdown's size isn't an integer number of sites, and mousing over a half-
shown item in the outliner scrolls to make it completely visible (this feels 
slightly flimsy).
3. Down arrow on scrollbar moves scrollbar about .8 sites at a time instead of 
one.
4. Classic: Go button is misaligned.
5. Modern 3: clicking down arrow at right edge of url bar moves the down arrow 
(ok), but forgets to clean up a pixel of the old position of the arrow.
6. If I clear the location bar and type a hyphen (or another character that no 
previously-visited URLs start with), the "search for..." thing doesn't appear 
when I type the hyphen.
7. The Netscape logo next to "search for..." is visually distracting.
8. Typing a few random letters and then quickly pressing up or down makes those 
letters disappear.  (In this case, the "search for..." thing hasn't appeared.)
9. Location bar context menu is gone.
I would also like to add that when I type the "ftp" part of ftp.mozilla.org, the
widget only returns a "search for..." line.  In the previous autocompletes, it
would have matched up all the ftp places I've typed without needing the ".xxxx"
part.
> 1. Dragging the scrollbar....

This is because the event.clientX/Y properties are miscalculated whenever the
event target is inside of a popup. I had to file and fix bug 76297 to workaround
this problem for calculating the selected row on mousemove.

> 2. Dropdown's size isn't an integer number of sites...

Hmmm... I used to see this problem, but I thought I had fixed it. The height of
the outliner is supposed to be exactly 6 rows, and I use the newly implemented
rowHeight property on nsIOutlinerBoxObject to calculate this. Perhaps it's not
always giving back an accurate number.

> 3. Down arrow on scrollbar moves scrollbar about .8 sites at a time...

Is there a way to control how many pixels a scrollbar increments by? I'll have
to look into that.

> 5. Modern 3: clicking down arrow at right edge of url bar moves the down arrow
(ok), but forgets to clean up a pixel of the old position of the arrow.

I've seen that too. Seems to be a layout bug.

> 6. If I clear the location bar...

This problem has irritated me beyond believe.  I am definitely telling the popup
to open in that situation, however, during these cases it refuses to paint
itself.  It actually creates it's frames, they just don't paint.  I've tried
many ways to workaround this, with no success.  I'm sure this will be the first
major autocomplete bug filed after I land.

> 7. The Netscape logo next to "search for..." is visually distracting.

This isn't just the Netscape logo, it's the logo of whatever search engine you
pick in your prefs under Navigator > Internet Search.  Set it to Google like I do :)

> 8. Typing a few random letters and then quickly pressing up or down makes those
letters disappear. (In this case, the "search for..." thing hasn't appeared.)

Weird, I'll try to fix that before I land.

> 9. Location bar context menu is gone.

Fixed that already.

>> 3. Down arrow on scrollbar moves scrollbar about .8 sites at a time...

>Is there a way to control how many pixels a scrollbar increments by? I'll have
>to look into that.

FWIW, scrollbars for outliners in MailNews do scroll one line at a time.
I hope this is the right spot.  Can we have this not show every every single
file a user visits?  I personally liked how it used to only display only the
links that were typed in.  Perhaps this could be an option that can be set?

blockcipher
I agree - I find the new way too cluttered to be useful. (Not to disparage
everyone's hard work or anything!)
after talking with joe over IRC, debating lots of issues, I'm proactively
sr='ing this before his next patch.. there's still a leak which he has to track
down, but once that is tracked down, then the sr= counts (but joe, still attach
a patch here so people can see what's landing)
I thought I was noticing some leaks, just by watching memory go in the windows
task manager as I typed, but alec claims not to see this on his machine, and
after checking the leak logs I seem to be properly releasing all my objects.

I'll post a new patch here shorly which includes post-Alec-review changes.
Not sure if anyone mentions it already, but the new autocomplete in the
experimental build, like the old one, still displays URLs that lead to 404 Not
Found pages. These shouldn't be recorded, since they're incorrect links, and
servers should be sending a 404 header to the browser so Mozilla should be able
to pick that up. The problem goes with incorrect domains, at least in the old
autocomplete.
gerbilpower@yahoo.com
that's covered by another bug, however the current belief (and DO NOT argue it 
here) is that 404 urls should be included in the autocomplete widget. 

Hewitt: could you color urls (or supply a different icon) by result type? 
perhaps a broken link icon for 404, a page w/ arrow for redirects, broken globe 
for dns failures, ...
URL: any
if you don't like seeing 404's in autocomplete, that's really a global history
problem.  autocomplete just searches the global history now, so perhaps you
should file a bug saying that 404 pages shouldn't be stored in history.  I don't
know think that http response code is stored in the history db, so there's no
way for me to color code 404's.
timeless/alex, that's bug 9203, "[RFE] do not save 'dead' or incorrect url's in 
the location drop down".
crawl, walk, run folks... let's get hewitt's patch landed, and then we can deal
with other issues as bugs. hewitt and I have talked about other changes we want
to make (such as new methods for storing keywords and search queries, smarter
ordering of the matches, and so forth)
ok, I'm satisfied! sr=alecf
*** Bug 71153 has been marked as a duplicate of this bug. ***
I'd like to see autocomplete-from-history as an option -- I think a lot of
people prefer the use-only-what-you're-given behavior. Bug #77935
finally, fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
No longer blocks: 41811
Although there are still a few kinks, I think this is an overall excellent job.
 Should we make a bug to get the autocomplete faster?
yes, we should open a new bug on the speed... both hewitt and I should revisit that
I would guess that any perceived speed problems would be largely in part to the
slowness in creating and opening the popup, rather than the speed of searching
the history.
I have to correct my previous statement.  Speed is just fine for url-bar
autocomplete on my celeron 550 system.  However, name autocomplete when
composing messages seems a lot slower.   Perhaps it is because of the huge lists
created with autcollect.
msg compose autocomplete is purposely slower - it's designed to not
auto-complete until you stop typing.
Hmmm... this makes it so we absolutely have to pause and stop typing when we're
banging out an email.  I open a message, I know who I want to send it to, I type
the first few letters and hit enter, but I don't get autocomplete.  Me no like.
VERIFIED Fixed on all platforms with 20010511.
(which of course only means that the widget is there and basically functioning as advertised, by no
means have extensive tests been performed and even so would be separate bugs)
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.