Location Bar doesn't appear as a normal text field

RESOLVED FIXED

Status

Camino Graveyard
Location Bar & Autocomplete
--
enhancement
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Nathan Hamblen, Assigned: Simon Fraser)

Tracking

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030307 Chimera/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030307 Chimera/0.7+

Camino's location bar's edges are solid gray and it does not show focus with a
glow. This is because (in BrowserWindow.nib) the location bar is drawn by a
parent view with a gray border and the textfield is instructed not to draw its
borders. I'm not sure why this was done, perhaps to facilitate placing the the
favicon inside the border. I don't think that the favicon *needs* to be inside
the border though; I think it looks just as nice outside of it. Even if it did
need to go in there, I'm sure we could make the textfield cooperate.

I've hacked the nib to let the textfield draw itself and I'll attach a pdf
screenshot. If anyone is interested I can attach the nib too. The autocomplete
is a little out of line when it pops up but that would be easy to fix.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Reporter)

Comment 1

15 years ago
Created attachment 117640 [details]
Look, pretty location bar!
the favicon really needs to be inside the url bar. it's like that on every other
browser, and helps to reinforce the idea that it's a proxy for the page url.

Comment 3

15 years ago
Is this WONTFIX, then?

Comment 4

15 years ago
I'd like to see the glow work, but I agree with Mike, the site icon should stay
inside the box.
(Reporter)

Comment 5

15 years ago
Please leave this open. I will figure out how to do it the way people want at
some point.
(Reporter)

Comment 6

15 years ago
Created attachment 120297 [details]
Screenshot of the NSTextField based location bar in action  

Looks just smashing next to the new search toolbar I think.
Attachment #117640 - Attachment is obsolete: true
(Reporter)

Comment 7

15 years ago
Created attachment 120298 [details] [diff] [review]
Patches autocomplete text field to use NSTextView for display 

The icon's in the box! Pretty straightforward, except for having to force the
textview to accept the custom text field cell.
(Reporter)

Comment 8

15 years ago
Created attachment 120299 [details]
This nib update is also required

By the way, since this nib doesn't reference the LocationBar containing view
anymore, LocationBar.h and LocationBar.mm could be removed from the build.
(Reporter)

Updated

15 years ago
Attachment #120298 - Flags: review?(pinkerton)
(Assignee)

Comment 9

15 years ago
Looks like a good thing to have.
Status: UNCONFIRMED → NEW
Ever confirmed: true
simon said he was in the middle of cleanup related to this.
Assignee: pinkerton → sfraser

Comment 11

15 years ago
Perhaps we could use this patch instead of the one submitted by Simon? Because
the new text field (build 20030415) still doesn't look like a normal cocoa
NSTextView.
(Assignee)

Comment 12

15 years ago
I agree that we should replace what I did with this patch. It might just take
some time to get to it.
Status: NEW → ASSIGNED
(Reporter)

Updated

15 years ago
Attachment #120298 - Flags: review?(pinkerton) → review?(sfraser)
(Assignee)

Comment 13

15 years ago
Comment on attachment 120298 [details] [diff] [review]
Patches autocomplete text field to use NSTextView for display 

>Index: AutoCompleteTextField.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/AutoCompleteTextField.mm,v
>retrieving revision 1.19
>diff -u -r1.19 AutoCompleteTextField.mm
>--- AutoCompleteTextField.mm	26 Mar 2003 23:14:51 -0000	1.19
>+++ AutoCompleteTextField.mm	12 Apr 2003 09:40:38 -0000
>@@ -49,6 +49,21 @@
> }
> @end
> 
>+// Text cell subclass used to make room for the proxy icon inside the textview
>+@interface AutoCompleteTextCell : NSTextFieldCell
>+@end
>+
>+@implementation AutoCompleteTextCell
>+
>+- (NSRect)drawingRectForBounds:(NSRect)theRect
>+{
>+  float offset = 19.0;
>+  theRect.origin.x += offset;
>+  theRect.size.width -= offset;
>+  return [super drawingRectForBounds:theRect];
>+}
>+@end
>+

This isn't really an AutoCompleteTextCell, it's a image-with-text cell
(and don't we already have several image-with-text cell classes? Can
we use one?).

>@@ -83,6 +98,28 @@
> 
> @implementation AutoCompleteTextField
> 
>++ (Class) cellClass
>+{
>+  NSLog(@"Called cellClass");
>+  return [AutoCompleteTextCell class];
>+}

Remove the NSLog.

>+// This method shouldn't be necessary according to the docs. The superclasses'
>+// constructors should read in the cellClass and build a properly configured
>+// instance on their own. Instead they ignore cellClass and build a NSTextFieldCell.
>+- (id)initWithCoder:(NSCoder *)coder
>+{
>+  [super initWithCoder:coder];
>+  AutoCompleteTextCell* cell = [[AutoCompleteTextCell alloc] initTextCell:@""];
>+  [cell setEditable:[self isEditable]];
>+  [cell setDrawsBackground:[self drawsBackground]];
>+  [cell setBordered:[self isBordered]];
>+  [cell setBezeled:[self isBezeled]];
>+  [cell setScrollable:YES];
>+  [self setCell:cell];

Looks like you're leaking 'cell' here.

>+  return self;
>+}
>+
> - (void) awakeFromNib
> {
>   NSTableColumn *column;
>@@ -119,8 +156,15 @@
>   [mTableView setTarget:self];
>   [mTableView setAction:@selector(onRowClicked:)];
>   
>-  // Create the icon column if we have a proxy icon
>+  // if we have a proxy icon
>   if (mProxyIcon) {
>+    // place the proxy icon on top of this view so we can see it
>+    [mProxyIcon retain];
>+    [mProxyIcon removeFromSuperviewWithoutNeedingDisplay];
>+    [self addSubview:mProxyIcon];
>+    [mProxyIcon release];
>+    [mProxyIcon setFrameOrigin: NSMakePoint(3, 4)];
>+    // Create the icon column
>     column = [[[NSTableColumn alloc] initWithIdentifier:@"icon"] autorelease];
>     [column setWidth:[mProxyIcon frame].origin.x + [mProxyIcon frame].size.width];
>     dataCell = [[[NSImageCell alloc] initImageCell:nil] autorelease];
>Index: BrowserWindowController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserWindowController.mm,v
>retrieving revision 1.102
>diff -u -r1.102 BrowserWindowController.mm
>--- BrowserWindowController.mm	10 Apr 2003 03:48:38 -0000	1.102
>+++ BrowserWindowController.mm	12 Apr 2003 09:40:43 -0000
>@@ -772,8 +801,8 @@
>     [toolbarItem setLabel:NSLocalizedString(@"Location", @"Location")];
>     [toolbarItem setPaletteLabel:NSLocalizedString(@"Location", @"Location")];
>     [toolbarItem setView:mLocationToolbarView];
>-    [toolbarItem setMinSize:NSMakeSize(128,20)];
>-    [toolbarItem setMaxSize:NSMakeSize(2560,32)];
>+    [toolbarItem setMinSize:NSMakeSize(128,NSHeight([mLocationToolbarView frame]))];
>+    [toolbarItem setMaxSize:NSMakeSize(2560,NSHeight([mLocationToolbarView frame]))];

Spaces after the commas please.

> 
>     [menuFormRep setTarget:self];
>     [menuFormRep setAction:@selector(performAppropriateLocationAction)];
> @end
Attachment #120298 - Flags: review?(sfraser) → review-
(Reporter)

Comment 14

15 years ago
>This isn't really an AutoCompleteTextCell, it's a image-with-text cell
>(and don't we already have several image-with-text cell classes? Can
>we use one?).

Not several, just one: ImageAndTextCell, used by the boomarks code.
Interestingly, it's attributed to Applle but resides in Camino's tree. My first
thought is that it might not scroll the way we want, the icon might side-scroll
off the view. I'll play with it and see. In the event that I can't use it,
should I rename the cell view I wrote? And if so, does that mean it needs to go
in a new source file? I would argue that this cell view is pretty specific to
the location bar, and wouldn't be useful anywhere else. (AutoCompleteTextField,
whether we like it or not, is itself specific to the location bar.)

>Looks like you're leaking 'cell' here.

I'm may be leaking a cell, but I'm not sure which one. NSTextField would have to
release the cell attached to it by default, wouldn't it? Otherwise we would
always have to release it ourselves, which we aren't doing. I think I sholud
probably release the old NSTextFieldCell before I call [self setCell:cell], right?
setCell should release what it's currently holding on to and retain what you
give it. So you should more than likely autorelease the cell you're creating
before calling setCell.
(Reporter)

Comment 16

15 years ago
Created attachment 121221 [details]
Same as the prior nib but without a CVS directory
Attachment #120299 - Attachment is obsolete: true
(Reporter)

Comment 17

15 years ago
Because ImageAndTextCell works with NSImage objects rather than NSView objects,
we can not use it as our image-with-text cell, at least not without some
significant changes elsewhere. Even if it did work I suspect that it would not
horizontally scroll the way we want. For those reasons I think it's justified
that we create a new image-with-text cell class, i.e. AutoCompleteTextCell. I'd
be happy to rename it if you wish.

The other changes are ready to go, I'm just waiting for direction on the cell
class before I upload a new patch.
(Assignee)

Comment 18

15 years ago
OK, let's keeep AutoCompleteTextCell.
(Reporter)

Comment 19

15 years ago
Created attachment 121292 [details] [diff] [review]
patch of autocomplete text field, updated with review recommendations
Attachment #120298 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #121292 - Flags: review?(sfraser)
checked in with a couple superficial tweaks. thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 21

14 years ago
Comment on attachment 121292 [details] [diff] [review]
patch of autocomplete text field, updated with review recommendations

We have a new Camino request flag which can be set multiple times for a patch.
Moving old review requests to the new flag. Sorry for the spam.
Attachment #121292 - Flags: review?(sfraser) → review?
Attachment #121292 - Flags: review?
You need to log in before you can comment on or make changes to this bug.