166449 - Page title and url cropping issues in location autocomplete droplist

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[sairuh (rarely reading bugmail)]

i've noticed that when the URL gets above a certain length, it will overlap with
its title in the Location's autocomplete droplist. this is also a problem in
mozilla (see bug 127089), but sfraser mentioned that the implementation in
chimera is different. (couldn't find an existing chimera bug for this, but dup
as needed.)

screenshot coming up.

Comment 1

[sairuh (rarely reading bugmail)]

Attachment: screenshot showing url and title overlap

how important is it to have the title displayed here? (in other words, why not
remove it? just curious from a usability stance. for myself, i track things
more often by url, not by title in this droplist.) if it is important to keep
the title in the autocomplete droplist, is there a way to more neatly crop the
url? alternatively, how about cropping the title, in order to display more of
the url?

Comment 2

[Jasper]

This is still an issue in the 20030902 NB.

Comment 3

[Gabriel Radic]

Hmm, maybe I don't understand this correclty, but I don't see any overlap
problem in NB 2003082902, nor in the screenshot. It works just like that for me
and I like it that way.

Do you have a problem with the URL's getting cut-off? I believe the space ratio
for the URL should be a bit bigger than it is, say 75% of the line instead of
what it is now (60%?). 

Another enhancement would be to show the start and the end of the URL instead of
just chopping the end. Same for the title.

Comment 4

[HARUNAGA Hirotoshi]

I can't see this problem on Camino 0.8 beta.
I believe this is a dup of bug 127089.

Comment 5

[Stuart Morgan]

The issue still exists, it's just not very precisely described. There is no
overlap, just cropping (see screenshot). Revising summary (was "page title and
url overlap in autocomplete droplist").

Re comment 4, as the original description says Camino has its own implementation
of the drop-down list, so it's not a duplicate.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: screenshot with better illustration of the separator and spacing

Basically, the dead space between the URL part and the title part needs to be increased by a few pixels.  

There's a faint separator line between the two parts (you can see it in the selected entry in my screenshot), and to the right of the line, there's always a half-space or so.  To the left of the line, longer URLs run right up to the line, so there's no space on that side.  Since there's only half as much space between the URL part and the title part on long URLs--and that is not much space :-)--the long, cropped URL appears to overlap the title.

Comment 7

[Samuel Sidler (old account; do not CC)]

Is this hard to fix? Adding spacing doesn't seem too hard and if it's not, it'd be a great little polish...

Comment 8

[Simon Fraser [no longer active]]

That's a table view in there. We'd have to do text truncation, or mess with cells. Not too hard, but more than a nib change.

Comment 9

[Håkan Waara]

Attachment: WIP patch

Ok, this will be my first Camino patch (and first code in Cocoa) so bear with me.

First of all, how do we want the truncation? In the middle of the URI and then at the end of the title? Or maybe ellipsis at the end of both URI and title? (Note that truncation (ellipsis) in the middle of a string is only available from 10.3 and above.)

This patch actually kind of works. It spews out runtime errors, because I changed the getResult() method of autocomplete to return a NSAttributedString from NSString. This is because we want to set the style info. But maybe we can do this later on in the flow? 

I also think I have to release the static info pointer somewhere. ;-)

All comments welcome. I will continue on this patch probably tomorrow.

Comment 10

[Håkan Waara]

Attachment: Patch

Ok, so this patch makes our autocomplete text styled. I've made the URI column truncate in the middle like in Safari, and the title column truncate at the end.

Comment 11

[Stuart Morgan]

Comment on attachment 206223 [details] [diff] [review]
Patch

+static NSDictionary *sURIColumnStyle;
+static NSDictionary *sTitleColumnStyle;

These should be declare inside the functions that use them.

+    [truncateInMiddle setLineBreakMode:NSLineBreakByTruncatingMiddle];

What exactly happens on 10.2?  Does it fail gracefully and behave the way it currently does?

+    sURIColumnStyle = [[[NSDictionary alloc] initWithObjectsAndKeys:truncateInMiddle, NSParagraphStyleAttributeName, nil] retain];

alloc already gives you a retain count of 1; you never want to retain something you alloc.

+    sTitleColumnStyle = [[[NSDictionary alloc] initWithObjectsAndKeys:truncateAtEnd, NSParagraphStyleAttributeName, nil] retain];


Same here.

 -(void)dealloc
 {
+  [sURIColumnStyle release];
+  [sTitleColumnStyle release];

It's not safe to release static members at instance deallocation--you are leaving all other instances with a dangling pointer.  This will cause strange behavior and/or crashes on every use of these values once an AutoCompleteDataSource has been dealloced.

Comment 12

[Håkan Waara]

Attachment: Better patch

Thanks for the review!

So, the reason I put retain there was that it was indeed crashing, and I thought something was freeing it before I wanted to. Indeed you can't release static objects in the instance dealloc :-)

So, since these static objects are around during the lifetime of our app, I'll just have them alive all the time.

Added runtime check for 10.3+

Comment 13

[Håkan Waara]

There's a typo in there "return nil on <= 10.3" should be "return nil on < 10.3".

Comment 14

[Simon Fraser [no longer active]]

Rather than doing it this way, did you think about setting the table column data cell to some custom class that handles the truncation? We have TruncatingTextFieldCell, and TruncatingImageAndTextCell; the former is probably closest to what you want. That way you don't need the 10.3 tests.

Comment 15

[Håkan Waara]

Attachment: Patch using TruncatingTextFieldCell

So, this simple patch is what I think should work.

It simply makes us use the existing TruncatingTextFieldCell class. However, for some reason the menuitems aren't updated (that is, they stay with the text I gave them on init). 

I haven't been able to track down the source of this - am I missing something obvious?

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass-reassign of bugs still assigned to pinkerton to nobody; filter on "NoMoPinkBugsInCamino".

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think Dan's rewrite in bug 495496 might fix this.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FIXED by Dan's rewrite in bug 495496 or some subsequent follow-up.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Moving these bugs to the 2.1 milestone to make it easier for them to fall out of the rough "all bugs fixed for 2.0" query.