159606 - Undo after autocomplete garbles URL

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Mike Pinkerton (not reading bugmail)]

- go to www.mozilla.org
- in url bar type "www.mo" and hit tab to autocomplete
- hit cmd-z (undo)

expected:
- url should say www.mo or www.mozilla.org

actual:
- urlbar says "www.mozilla.orgzilla.org"

it appears to be undoing based on what the selection was before the
autocomplete, not just restoring the text.

Comment 1

[Mike Pinkerton (not reading bugmail)]

oops, make that:

"http://www.mozilla.org/zilla.org/"

Comment 2

[David Haas (gone, not reading mail)]

Attachment: fix

This looks to me like it fixes the garbled stuff without introducing any new
bugs.  I could be wrong on the last part, though.

Comment 3

[David Haas (gone, not reading mail)]

You can have the bug back to check out/check in the patch.

Comment 4

[Mike Pinkerton (not reading bugmail)]

-  if (range.location == [[self string] length])
-    [self startSearch:[self string] complete:!mBackspaced];
-  else
+  if (range.location == [[self string] length]) {
+    NSString *searchString = [[self string] copyWithZone:nil];
+    [self startSearch:searchString complete:!mBackspaced];
+    [searchString release];
+  }

is the reason you did this to ensure that the string doesn't change out from
under us when the user changes the value of the text view? if so, we should add
a comment.

Comment 5

[Mike Pinkerton (not reading bugmail)]

two undesired side effects:

1) now when you select items in the autocomplete area, they no longer change the
url text. i don't like this.

2) if you get suggestions, down arrow to select one, then up arrow back into the
url bar, the cursor is at the _start_ of the url bar which makes adding text to
the end pretty difficult.

Comment 6

[Mike Pinkerton (not reading bugmail)]

cc'ing [e-mail] so he sees my comments.

Comment 7

[David Haas (gone, not reading mail)]

Attachment: updated for comments

Just like above patch, but I added a comment in the code as per comment #4 and
to put the cursor at the end of the URL bar as in comment #5 point 2.

pink can figure out comment #5 point 1, since the url text changes for
everybody but him ;)

Comment 8

[Mike Pinkerton (not reading bugmail)]

i pulled a fresh tree on a different machine (my powerbook) and the patch still
doesn't put the text into the url bar when i pick an item in the autocomplete list.

*punt*

Comment 9

[David Haas (gone, not reading mail)]

OK, I'm going to blow away the entire directory & rebuild everything from
scratch tonight to try this again, but before I do that . . . . you're not
running something funky like a 10.2 seed, are you?

Comment 10

[Mike Pinkerton (not reading bugmail)]

nope, normal 10.1.5 with the latest shipping (not seed) dev tools.

Comment 11

[David Haas (gone, not reading mail)]

I blew away my entire mozilla directory and my Chimera profile, pulled from CVS
(around 1 pm today) & rebuilt from scratch, then applied this patch.  It worked.

We've got to be talking about different things.  

Comment 12

[Max Horn]

I can reproduce the problems Mike describes in comment 5.  Although for 1), the URL is 
displayed, but only after a brief delay.

I wonder, when you click on an item in the autocomplete menu, it currently immediatly 
starts to load that page, while in IE it simply selects the item. Which behaviour is better I 
wonder?

And for 2): yes, when I use the arrow keys to select items from the autocomple menu, the 
URL bar stays unchanged - it only shows the partial URL I typed in, nothing else. IMHO it 
should behave more like IE. I dunno if that is what Dave is seeing or simply a feature that 
is (not yet?) implemented, though.

Comment 13

[David Haas (gone, not reading mail)]

Well, after reading Max's comments, I've figured out the problem:

you're both trying to drive me insane ;)

Just for grins, I'm re-installing the Dec 2001 dev tools & I'm gonna recompile,
although I don't have a clue why that would matter.  But on my machine, when I
arrow down or up in the selection list, the URL bar gets filled in. 

Do you have autocomplete while typing set?  Some other URL bar preference that I
could check on? Can I zip up a copy of my debug build (it's 100+ MB) and have
one of you try it on your machine to see if it works there?  Are you building
with static libraries or not (I'm not). If you can't tell, I'm grasping at
straws here.

As for the "immediately load on click" vs. "just fill in the URL bar on click"
I'm indifferent.  I think it's a 1 line change.  Whatever people want is OK.

Comment 14

[Max Horn]

I have a default non-static debug build straight after the build guide.

I wasn't aware that there are any autocomplete prefs, where could I view/change those?

Comment 15

[Max Horn]

I am really surprised that this works for you, Dave.The problem is that excpetions are 
thrown, because of "out of bounds" NSString access. For example, if I type in "so" over 
here, then many pages are shown in the auto complete list that contains so *anywhere* 
in the URL (that's how rangeOfString works after all, if you don't specify any options). So 
e.g. the match is at position 9. Which in completeResult leads to a call to 

      [self setStringUndoably:result fromLocation:(endPoint)];

where endPoint in my example is 9. And now inside of setStringUndoably, this code is 
run:

  NSRange aRange = NSMakeRange(aLocation,[[self string] length] - aLocation);

Guess what, length = 2, aLocation = 9, -> differentce is -7, but since the members of a 
NSRange are unsigned, this amounts to a huge positive number (4 billion something). 
And quite correctly NSTextView fires an exception.

A possible fix is to use this code in setStringUndoably to calculate the range (instead of 
the above line):
  unsigned len = [[self string] length];
  NSRange aRange;
  if (aLocation > len)
    aRange = NSMakeRange(len, 0);
  else
    aRange = NSMakeRange(aLocation, len - aLocation);

Comment 16

[David Haas (gone, not reading mail)]

Attachment: updated again

From IRC & Max's comment #15, I now understand why it only worked for me.
This patch should make it work for everybody else, too.  

As for comment #12: I left in the immediate load on click.  It is a 1 line
change (in onRowClicked, unsurprisingly) to give behavior like IE.  Anyone have
a strong opinion about this?

Comment 17

[David Haas (gone, not reading mail)]

Patch in bug 161621 changes textview back to a text field & should fix this.

Comment 18

[Mike Pinkerton (not reading bugmail)]

this is still an issues after 161621 landed, but much less so and it's hard to
get into this scenario. basically you can get it to a point where you undo back
to a previous url, but the completion popup is still up (but not applicable) and
selecting anything from it will replace parts of the non-applicable url.

Comment 19

[David Haas (gone, not reading mail)]

Attachment: fixed

I had this fix in for TextViews, then when we went back to TextFields I
couldn't figure out why I had it in the first place so took it out.  Whoops.

It closes the popup window if we undo or redo.	I think that's a sensible thing
to do.

It also changes the URL bar font back to what it was, but I'm hoping nobody
notices that.  Whoops better erase this last line before I sub

Comment 20

[Mike Pinkerton (not reading bugmail)]

landed.

Comment 21

[Chris Petersen]

Verified in the 2002-08-21-05 NB.