235782 - A new, more flexible tabbed browsing control for Camino

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Geoff "temporarily off bugmail" Beier]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040224 Camino/0.7+

Ever since the release of Panther, I don't like Camino's tabs as much as I used
to. Attached is a patch that uses a custom control for manipulating Camino's
tabbed browsing view and, in addition to making them a little easier to hit with
the mouse (IMO), has the following features:
1. Close button on each tab. (recently added by Mike to the standard tabs)
2. The favicon is replaced by the progress indicator when the tab is still
loading, rather than the close button.

In addition to those, it should now be possible to add drag-to-rearrange to the
tabs. I am currently working on that. I didn't have it ready for this patch, and
wanted to start getting feedback from developers on:

1. The possibility of someone liking this idea enough to commit an
implementation of it to Camino.
2. Things I'll need to fix/remove/change/do better/etc. in order to make that
happen, assuming that it's OK in principle.
3. Feedback on the look and feel.

Thanks are due to Mike for answering dumb questions on IRC and the list, and to
Jasper for providing some artwork. Any aesthetic appeal this patch holds is
thanks to him. Any ugliness is my fault. Thanks also to the Adium folks for some
ideas/inspiration.

This patch is currently packaged into 4 parts. I separated the source into added
files + a patch file, because I could not get "cvs add" to add the files to my
working directory without write access to the repository, and didn't find a way
around that. The .nib is attached as a tarball, and the graphics are in a
separate tarball. Any feedback on how to package this better would be most welcome.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Geoff "temporarily off bugmail" Beier]

Attachment: changes to files already in the tree

Comment 2

[Geoff "temporarily off bugmail" Beier]

Attachment: additional source files that I couldn't get cvs to include in the diff

Comment 3

[Geoff "temporarily off bugmail" Beier]

Attachment: archive of the NIB file

Comment 4

[Geoff "temporarily off bugmail" Beier]

Attachment: images for the tab chrome

Comment 5

[Geoff "temporarily off bugmail" Beier]

Attachment: obligatory screenshot

Comment 6

[Greg K.]

Confirming as an RFE.

Comment 7

[Jérome Foucher]

Can't get the archive of the NIB file to decompress correctly. I end up with a .nib holding just the CVS 
folder.
Can you repost it please ?

Comment 8

[Geoff "temporarily off bugmail" Beier]

Attachment: archive of the NIB file

Got the wrong BrowserWindow.nib earlier

Comment 9

[Geoff "temporarily off bugmail" Beier]

Attachment: images from camino/resources/images/chrome

minor resource tweaks

Comment 10

[Jérome Foucher]

Thanks for the new files. The NIB package is now ok.
The patch works perfectly.

The close button images are the same as the ones I provided for the patch for bug 211570 I think. I 
originally took the images from Safari, and converted them in PNG because Bugzilla did not let me 
attach the TIFF files. However, the PNG images don't look as good as the original TIFF ones. Maybe 
Japser could design some new cool images...

Comment 11

[Mike Pinkerton (not reading bugmail)]

jerome, bugzilla has nothing against tiff files. can you put them in a zip and
attach that if that's easier for you?

Comment 12

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #10)
> Maybe Japser could design some new cool images...

Jasper has something in progress... I in fact owe him an email on this very
subject :-)

Comment 13

[Hiro]

Attachment: The screen shot when opening tab of the maximum number(16 tabs)

The patch was applied and a few was used.
I think that it is good :-)

By the way, when tab of the maximum number is opened like a screen shot, each
tab shows which page or distinction is difficult.
When a mouse pointer is placed on tab, is it possible to add the function to
help distinction of expressing a page title name as a tool tip like firefox?

Comment 14

[Eiichi]

I think the active tab should be clearly highlighted against the inactive ones
and inactive ones should be dimmed.

Comment 15

[Eiichi]

Attachment: the screenshot

I tried to replace active_tab_bg with active_tab_bg.
The active tab is clearly highlighted.

Comment 16

[Eiichi]

Attachment: the screenshot

Sorry, I've mistaken.
I tried to replace active_tab_bg with active_tab_bg.
The active tab is clearly highlighted.

Comment 17

[Neil Lee [:neilio]]

Visually, this is completely backwards now. Historically things that are not active are greyed out, and 
here this the /active/ item that's darkened. It really should be reversed.

Comment 18

[Hiro]

(In reply to comment #16)
> Created an attachment (id=143103)
> the screenshot

At Camino which I built, active tab is displayed by highlite.
Are all patches applied?

Comment 19

[Eiichi]

> At Camino which I built, active tab is displayed by highlite.
> Are all patches applied?

No.
I replaced active_tab_bg with active_tab_bg

Comment 20

[Eiichi]

Attachment: screenshot using Aqua interface

I think we need not ape Safari's interface.
Safari uses Metal interface and an active tab is connected to Location field.
In Aqua interface the acive tab is highlighted in blue face and the inactive
ones remain white, so users can easily confirm where they are.

Comment 21

[Eiichi]

Attachment: screenshot using Graphite interface

In the same concept I tried Graphite interface.

Comment 22

[Jasper]

Dude you have been switching images around like a mad man. You completly
misunderstood the chrome.

The active tab is the one that has a clear white appearance, the inactive tabs
are the ones with the dimmed grey look. Why you might ask? Well because in real
life objects that go into the background are not lighter then objects in the
foreground but are darker. It's aproven metaphor by both Firefox and Safari and
real life of course. You should use that, I'm under the impression that you are
looking to much to apple's tabs which had no real appearant color logic to me,
appart from being the other way around which always seemed really odd to me.

As you may see in this iamge:
http://www.jasperhauser.nl/camino/bla/tabs_geoffpreview2.jpg

I tweaked the ui it a bit more so that inactive tabs have a subtle hollow look,
while the active tab has a direct opposite gradient. Making it look as if the
active tab is reaaly more in the foreground. I would defenitly not give the
inactive tabs a light color since it would make it look to much like the
bookmark bar, which might result in confusion amongst users. Note that the dark
color of the inactive tabs make the tab bar stand out much better then what you did.

As for color I doubt that we need it. I think that the addition of color like
the blue you used makes the interface look cluttered together with all the site
icons. The dark grey and light plastics make it look much cleaner and
proffesional. Would we want to add al kinds of extra chrome for the blue theme?
That would be a waste of space, especially if we can make one appearance that
works in both graphite and blue environments.

Comment 23

[Eiichi]

Attachment: screenshot of Mail.app

I said which interface is more friendly for users.
When we use tabs in Aqua interface, we usually confirm that darkend tab shows
the window it contains.
Even if it's old as you say, in Aqua interface we should do so like Mail.

Comment 24

[Eiichi]

In Finder view as Icons, as List and as Columns is the selected item lighter
than others?
No, it's marked as darkened.

What we need is we can distinguish explicitly which item is selected. 
The important thing is that we confirm which item is selected at a glance.
So I think the metaphor sholud have uniformity.

In Safari Apple put away uniformity designed by themselves, because they use
Metal interface.

Comment 25

[Neil Lee [:neilio]]

Yes, but those do not indicate the same information. In list view, it indicates the column that is 
currently sorting the list view.

Tabs signify completely separate window environments, not the sorting of information. It's a completely 
different task. At any rate, this is a wasted argument: we need to do what users expect, and users 
expect that tabs in the background will be darker or "greyed" out.

Comment 26

[Martin Creutziger [:MMx]]

as a user and sysadmin who is often confronted with "less qualified" users, I
agree with Neil and Jasper that the active tab is supposed to be lighter.
I think Jasper's screenshot in #22 just looks awesome (the tabs). I have seen a
lot of different tabs already and that is *definitely* the best I have seen so
far. I loved it at first sight ;-)

Comment 27

[Eiichi]

(In reply to comment #25)

Contrary to your opinion, I think most users expect that the active tab is
highlighted and inactive tabs are dimmed, because tabs are buttons to indicate
which window is active.

Comment 28

[Stuart Morgan]

Just to throw my two cents in:

- 2 or 3 or 5 or 10 people's opinions of what they think most users will expect
tabs to be like really don't amount to much. Anyone can think whatever they want
about "most users", but unless someone has actual data from usability
studies/testing, it's not an especially useful argument either way.

- Moreover, I submit that it doesn't matter either way from a usability
perspective. The selective item shouldn't be "lighter" or "darker"--it should be
*distinctive*. Given a bunch of things which are the same, and exactly one that
isn't, people can easily pick out the one that is distinctive, be it because
it's lighter, darker, another color, bigger, whatever. This is a skill that is
fundamental to the way our brains work, and one we master very early (think
Sesame Street). So long as the active tab is, to some degree, visually
distinctive, the usability task is over.

Now, if the issue were something like a dialog box with only OK and cancel,
where there are only two choices and no other clues to which is active, and
their choice is final in at least some sense, then it would be important what
people's gut feeling is ("what happens when I press return?"). But this is a
fully interactive, user-created set of tabs. They have titles, which either do
or don't match the current page, and they become active when you click them.
Even if you sat someone down with two tabs open with the same title, it would
*still* be trivial to figure out what the colors mean. So, 5-10 seconds of
acclimation, tops, for an application that will likely be used very frequently.
 Who cares?

---

Now, from a purely asthetic standpoint, I like the lighter active tab, because
it creates a visual blending with a lot of page content which appeals to me.

Comment 29

[Eiichi]

O.K. This is my last statement about this.
This shouldn't be treated as anyone's taste, but as uniformity of user's interface.

1) Safari doesn't use Aqua interface, so it adopts lighter active tab.
    But its active tab is inverted and connected to Location bar, and tab has no
URL icon.
    Consequently this had the consequence that Apple uses Metal interface.

2) If Camino adopts tabs like Safari, the least it should make it clear that the
active tab is 
    connected to the active window.

Comment 30

[Hiro]

Attachment: Crash log

It may crash, if Tab is opened further and a page is displayed, when load is
applied to Camino by displaying many pages simultaneously by Tab etc.

It does not crash, even if it carries out the same operation by official
Nightly build.

Comment 31

[Isaac Finnegan]

Actually I can get Camino to crash with any build if I have it open too many
pages at once (dragging a large bookmark folder).  I don't think it's the tabs.

Comment 32

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #30)
> It may crash, if Tab is opened further and a page is displayed, when load is
> applied to Camino by displaying many pages simultaneously by Tab etc.
> 
> It does not crash, even if it carries out the same operation by official
> Nightly build.

Thanks. I think I've got that figured out now and will soon update the patch to
fix that.

Comment 33

[Eiichi]

I'm not sure this will help TabView development, but just for reference...

Shiira, a new cocoa tab browser using Apple's Web Kit rendering engine like OmniWeb.

Shiira Project : http://hmdt-web.net/shiira/
Nightly Build 040420 : http://hmdt-web.net/shiira/build/Shiira040420.dmg
ShiiraSrc 040420 : http://hmdt-web.net/shiira/build/ShiiraSrc040420.dmg
TabBarViewSrc : http://hetima.com/temp/HTSRTabBarView040420.dmg

Comment 34

[Greg K.]

FYI, there’s a private build available with these new tab controls via MZ:
<http://forums.mozillazine.org/viewtopic.php?p=513260#513260>.

A couple of nits:

I’d like to see a pixel or two of space between the top of the tabs and the
toolbar above.

The background tabs should have rounded corners like the foreground tab.

The mouseover effect on background tabs leaves a space on the right if and only
if there is another tab to the right.

Comment 35

[ender_mb]

(In reply to comment #34)
> FYI, there’s a private build available with these new tab controls via MZ:
> <http://forums.mozillazine.org/viewtopic.php?p=513260#513260>.
> 
> A couple of nits:
> 
> I’d like to see a pixel or two of space between the top of the tabs and the
> toolbar above.

Yeah, this might be nice...

> The background tabs should have rounded corners like the foreground tab.

IMHO this would make for too much visual clutter in the tab bar.

Comment 36

[Eiichi]

Hi, Geoff.
Thank you for your efforts.
I'm using Isaac's private build.
It works fine including drag'n'drop operation.
But it sometimes seems to log "Got mouseUp!".
Except for that it works perfectly for now.

Comment 37

[Josh Aas]

Since we're about to start on the 0.9 buglist, can we get and update from Geoff
on  how things are going? This will help us decide how much effort to put into
fixing thing in our existing tab implementation over the next few months.

Comment 38

[Mike Pinkerton (not reading bugmail)]

yeah, i'd like to see where we're at with this. is it time to start looking at
code so we can get the bulk of it landed into the trunk?

Comment 39

[Chris Petersen]

Geoff,

Any chance we can get this checked into the trunk. The more eyes we can get to 
     bang on this feature the better...

Comment 40

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #39)
> Geoff,
> 
> Any chance we can get this checked into the trunk. The more eyes we can get to 
>      bang on this feature the better...

Hi Guys,

Sorry I'm slow... I've been unexpectedly swamped. I've got a patch that I think
is almost good enough to go in, and I just need to clean up a conflict against
HEAD, smoke test, and package/post it here. I'll try to do that in the next 48
hours so others can review and this can move forward!

Geoff

Comment 41

[Geoff "temporarily off bugmail" Beier]

I'm trying to produce a patch and currently running into a pretty basic (I
think) CVS problem that's blocking my progress. Does anyone know how to get 'cvs
diff' to work properly when there's a conflict? 

My patch creates a conflict in BrowserTabViewItem.mm, partly because it moves
NSTruncatingTextandImageCell out to a new file (there are other conflict areas
too...), and when I try to do a cvs diff  to produce a patch, I get conflict
markers in the diff. As you might expect, the patch will not work that way.

If anyone could post some advice, either here or via email to me, I'd very much
appreciate it. I'm stumped as to why my changes are getting conflict markers in
the cvs diff rather than getting a patch file which updates the current revision
to my local copy.

If I can get past this problem, I should be able to get a patch up here in very
short order. Due to some unexpected travel, I'll have limited access to my mac
for a few days, so worst case, I expect to get it up here Thursday.

Thanks,

Geoff

Comment 42

[Mike Pinkerton (not reading bugmail)]

you need to update your local tree to the latest version and resolve any
conflicts that creates so that your local tree builds again. Then your diff will
be clean.

make sure you do a cvs diff -N to include the new files that you've cvs add'ed. 

Comment 43

[Josh Aas]

Geoff - status update?

Comment 44

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #43)
> Geoff - status update?

It's a long, convoluted story not worth posting here :-)... the short version:

I'm now back online and just got a new build box (bonus: it's a laptop, so I can
hack when I travel!!). It's now got developer tools and a fresh camino tree and
is building, as well as a restore from the archives of my old box. As soon as it
finishes, I'm making a new patch and posting it. If that's in the next hour or
so, it may be tonight. If not, I won't go to bed tomorrow night until I have a
good patch.

Comment 45

[Josh Aas]

sounds good :) thanks

Comment 46

[Geoff "temporarily off bugmail" Beier]

Here's a patch folks can start to look at... CVS gives me the following when I attempt to add the files:
cvs [server aborted]: "add" requires write access to the repository

so cvs diff -N doesn't do what we want. I'm posting a patch as well as a tarball of the new classes. I'm 
having a funny problem with the nib file, so I'm not posting that just yet. I'll either sort it out tomorrow 
or show up on irc asking for advice :-) (Unless someone knows of a way to get IB or some utility to diff/
merge nibs)

Please let me know what you see that needs to be addressed to get this into shape to be added.

Geoff

Comment 47

[Geoff "temporarily off bugmail" Beier]

Attachment: Changes to files already in the tree

Comment 48

[Geoff "temporarily off bugmail" Beier]

Attachment: additional source files

Comment 49

[Josh Aas]

Any known issues?

Comment 50

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #49)
> Any known issues?

The main thing is that I haven't been able to get a new nib built based on the one in HEAD. I'm sure I'm 
forgetting something simple, and just need some time to look at that later today. I've been testing with 
one of my old nibs, but there appears to have been some change to the one in CVS since I made that, 
so I'm reluctant to post that here. I'll either post a good one today or come asking questions on IRC this 
evening.

The only other issue I'm aware of is that I've occasionally seen a little bit of quirkiness in the drawing 
(e.g. separator line between tabs can be drawn 1px off where it should be) and would like to see if I can 
tweak the drawing to make that go away. It's pretty subtle, though, and I have not yet been able to 
reproduce it with this patch against HEAD on my current machine.

All the functionality of the current tabs should be there and working. If anything is not that's an error, 
so please let me know.

Comment 51

[Geoff "temporarily off bugmail" Beier]

Attachment: Archive of the .nib file

Here (finally) is a working .nib to go along with it. Also posting the steps it
took to get it right, since this was a little error-prone :-)

Steps to building a working .nib:

1. Parse the following classes:
IconTabViewItem
BrowserTabView
BrowserTabViewItem
BrowserContainerView
BrowserTabBarView
RolloverTrackingCell
TabButtonCell
TruncatingTextAndImageCell

2. In the Browser Window set the BrowserTabView to tabless, borderless. Change
the custom class on the NSTabViewItem to BrowserTabViewItem.
3. Change the height of the BrowserContainerView to 456
3. Add a custom view to the BrowserContainerView. Set its size:
	-x 0
	-y 434
	-w 761
	-h 22
Set its class to "BrowserTabBarView". Connect its mTabView outlet to the
BrowserTabView.
4. Connect the mTabBar outlet on the BrowserTabView to the BrowserTabBarView.
5. Connect the mTabBar outlet on the BrowserContainerView to the
BrowserTabBarView.
6. Connect the mTabView outlet on the BrowserContainerView to the
BrowserTabView.

Comment 52

[Ludovic Hirlimann [:Usul]]

(In reply to comment #47)
> Created an attachment (id=155663)
> Changes to files already in the tree
> 

Could you post these using diff -u it's a lot easier to read.

Comment 53

[Geoff "temporarily off bugmail" Beier]

Attachment: Changes to files already in the tree

This revision fixes some visual display glitches Jasper found and prevents tabs
from giving mouseover feedback for background windows.

It's also a unified diff, as requested.

Comment 54

[Geoff "temporarily off bugmail" Beier]

Attachment: additional source files

The new classes, reflecting the same tweaks as the patch, uploaded separately
because I couldn't get the cvs diff to include them.

Comment 55

[Isaac Finnegan]

This is working great for me. I've been able to patch/add files/add nib and
build with no problem.  Love the new tabs. Great work Geoff.

P.S. I have to move the spinner/favicon to the left of the page title in the
tab. I'm sorry but no browser I've ever used puts them to the left. And to those
who have a problem with it being next to the close button: how did you ever
navigate the colored window buttons in OSX in the first place?

Comment 56

[Isaac Finnegan]

Ugh. I meant no browser with tabs puts the favicon on the right. sheesh

Comment 57

[Josh Aas]

Mike - have you looked at this patch yet? I'm going to try to build it and get
it ready to land this weekend. Wondering if you know of any reasons it can't hit
the trunk before I go through it. I'll also make a nice diff package so its easy
to apply.

Comment 58

[Josh Aas]

The patch looks good, compiles and whatnot, but the UI has some serious issues
that we should think about resolving before landing on the trunk.

1. There is no indication of which tab is currently selected
2. There is no divider between tabs - just a favicon next to a close box
3. The 16-tab limit has not been removed. We need to have the little ">>"
looking thing on the right with a pulldown menu listing tabs that overflow. I
know its not easy to do, but it is the right thing to do and shouldn't be that
hard. Geoff - if you don't want to do it I will.
4. I don't think the favicon should be on the right. People have it in their
heads that it goes on the left and the title goes to the right, and that is also
how other UI elements work (imaged menus and whatnot). I think we should just
swap the close and favicon images.

I think it would be OK to land on the branch after 1,2, and 4 are taken care of.
3 can wait a bit, but it needs to be done, sooner rather than later hopefully.

I will post a code-level review after we figure out what to do with the UI.

Thanks for the great work - I hope you're not put off by pickiness, but a tight
review process helps a lot in the end. And it makes you feel better when it does
land :)

Comment 59

[ender.mb]

(In reply to comment #58)
> The patch looks good, compiles and whatnot, but the UI has some serious issues
> that we should think about resolving before landing on the trunk.
> 
> 1. There is no indication of which tab is currently selected
> 2. There is no divider between tabs - just a favicon next to a close box

If you don't see the dividers between the tabs and the selected tab being
lighter, then something probably went wrong with your build, or some images are
missing or something..

> 4. I don't think the favicon should be on the right. People have it in their
> heads that it goes on the left and the title goes to the right, and that is also
> how other UI elements work (imaged menus and whatnot). I think we should just
> swap the close and favicon images.

I actually prefer to have both the close box and the favicon on the left--I
think people are making the usability issues there to be worse than they are;
I've never accidentally closed a tab when i meant to drag it or vice-versa.

One other thing I've noticed: dragging links from a page to the tab bar to
create a new tab only seems to work sporadically, I haven't been able to track
down the exact circumstances it works under, but it fails often enough to
warrant looking at.

Comment 60

[Josh Aas]

I had a friend who doesn't work on web browsers :) or would be considered an
"advanced" computer user look at the new tabs and he almost immediately, about
the icons, said "why don't you just put the icon next to the close button?"

As we haven't considered this before (that I know of), any thoughts? I think
having the close button on the far left and the icon immediately to the right of
it, followed by the title might not be a bad idea. Something to think about.

Comment 61

[Jasper]

Josh, you probably forgot to also download and add the chrome files. Anyway
Geoff is preparing a new patch with updated files fixing some visual issues. I
can asuree you chrome is available.

About the site icon and close icon.

We released a versions with all options to see what users liked.
1) left side site icon, right side close box. Most people liked this the most
2) left side close box, right side site icon. People expected the close box to
be on the right side, because of the way site icons and site names are displayed
everywhere else in Camino.
3) site icon and close box on the left side. People hated it, including me. Most
people thought it look cluttered and very odd. And general feeling was that it
was the perfect situation for people to eccidentaly click and close the tab. And
I agree. This is not the option to go with. With 16px icons you should never but
 functionalities to close to each other. Spread them appart. It look better and
works better period.

The reason why Geoff hasn't worked on the 16+ feature is because he wanted the
patch to be checked and tested on feature parity first to make further feature
additions easier instead of makin one huge patch that would be hard to review
and even harder to track bugs with.

And we haven't decided et on how to implement the 16+ issue as we think the
Safari way lacks certain things. I hope to descuss some ideas soon with you
guys. For now we are concentrating on getting this first patch perfect so we can
land it to make further changes much easier for Geoff.

Comment 62

[Geoff "temporarily off bugmail" Beier]

Attachment: images from camino/resources/images/chrome

This is the tab chrome. First, remove any files beginning with tab_ from your
xcode project, then untar this archive in camino/resources/images/chrome.

If it looks like there are no dividers, etc., you're probably missing these
files.

Comment 63

[Geoff "temporarily off bugmail" Beier]

Attachment: changes to files already in the tree

This revision reorganizes the drawing routines a bit to correct some glitches
uncovered by Jasper's new chrome, and generally simplify things. It also moves
the favicon to the left of the label, per Josh's most recent suggestion to give
folks a chance to try it this way.

Comment 64

[Geoff "temporarily off bugmail" Beier]

Attachment: additional source files

Goes with the latest revision of the patch...

Comment 65

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #58)
> The patch looks good, compiles and whatnot, but the UI has some serious issues
> that we should think about resolving before landing on the trunk.
> 
> 1. There is no indication of which tab is currently selected
> 2. There is no divider between tabs - just a favicon next to a close box
As Ender said, these two sound like missing chrome.

> 3. The 16-tab limit has not been removed. We need to have the little ">>"
> looking thing on the right with a pulldown menu listing tabs that overflow. I
> know its not easy to do, but it is the right thing to do and shouldn't be that
> hard. Geoff - if you don't want to do it I will.
As Jasper mentioned, we are thinking about this, and don't plan to just ignore
it. After discovering firsthand how much fun it is to maintain a large chunk of
code that lives outside the tree, I was kind of hoping I'd be able to do this
after the patch hits the trunk :-).

> 4. I don't think the favicon should be on the right. People have it in their
> heads that it goes on the left and the title goes to the right, and that is also
> how other UI elements work (imaged menus and whatnot). I think we should just
> swap the close and favicon images.
> 
I'm not attached to the current placement. Here's my logic:

1. Since 1984, the close box goes on the left all the time. I didn't want to
violate that lightly.
2. I thought the close button and the favicon together looked kind of cluttered,
so I moved it to the end.

If the consensus is that I'm full of it on either point, just let me know what
is preferred... I don't feel strongly about it at all, but had to pick a layout
for the first cut. In the most recent version of the patch, just so you can
easily see and play with the difference, I've moved the favicon.


Please continue to be picky... I want the best code possible and am not the
least bit put off by pickiness. (Ask Jasper :-))

Comment 66

[Josh Aas]

Getting rid of the 16 tab limit after this hits the trunk sounds fine.

I will apply the latest patches and whatnot and test tomorrow. Are you ready for
a code-level review? How many more revisions do you anticipate before it hits
the trunk? Just settling the favicon/close box placement? My current thinking is
left side site icon, right side close box.

Comment 67

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #66)
> Getting rid of the 16 tab limit after this hits the trunk sounds fine.
> 
> I will apply the latest patches and whatnot and test tomorrow. Are you ready for
> a code-level review? How many more revisions do you anticipate before it hits
> the trunk? Just settling the favicon/close box placement? My current thinking is
> left side site icon, right side close box.

At this point, I think I'm ready for a code level review. The only further
revisions I'm currently planning are any that you guys feel are necessary before
this hits the trunk. (Like the icon placement) 

Once this hits the trunk I'd like to add:

1. Nice handling of >16 tabs.
2. Drag-to-reorder.

I'm ready to work on these now, but would like to wait till this hits the trunk,
as it'll be much easier to maintain a patch that way :-). I also suspect the
code is easier to review in 3 chunks than in one big shot.

Comment 68

[Warren TenBrook]

We've debated close buttons on tabs in the past (e.g., bug 155292).  I'll renew
the suggestion to eliminate close buttons on tabs in favor of a unified close
widget in the tab bar, a la Firefox, or just adding the "Close Tab" toolbar
button on the far right of the toolbar default set for all users.  This would
clean up the proposed tab UI considerably.

Comment 69

[Josh Aas]

In the future, perhaps gzip one folder containing all the latest stuff for
patching the trunk. That will help keep the attachments list under control. Thanks!

Comment 70

[Josh Aas]

I just compiled with the new tab code, including chrome :)

It looks great, and works great! I actually like the close button and the
favicon being next to each other on the left. Its sort of like the browser's new
icon set - it is a bit awkward at first, but you get used to it quickly. This
solution makes sense in terms of usability rules - the close button has always
been on the left in Mac OS, and the icon has always been on the left in Mac OS.
I know lots of people object at first, but I think it just takes some getting
used to. Its nice not having anything on the right side of the tabs.

If you don't have any other patches coming up in the next day or two Geoff, I'll
start the code review and we'll get this to hit the trunk. Good work!

Comment 71

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #70)

> If you don't have any other patches coming up in the next day or two Geoff, I'll
> start the code review and we'll get this to hit the trunk. Good work!

Thanks! I'm not planning any new patches for now unless you guys see a problem
that requires one, so please go ahead with the review.

Comment 72

[Jon C. Thomason]

(In reply to comment #65)
> 1. Since 1984, the close box goes on the left all the time.
> I didn't want to violate that lightly.
> 2. I thought the close button and the favicon together
> looked kind of cluttered, so I moved it to the end.

I think these are good points.  If I may suggest, though, the question isn't so
much about the appearance of clutter.  Rather, it's about the proximity of a
destructive option next to a safe one.

It's about one's chances of accidentally closing a tab when his intention is the
opposite: to keep it permanently by dragging it somewhere safe.  (Yes, I know
the title text is a drag source but the point of the favicon-on-the-left is to
mimic established Finder behaviors which connect with a corresponding variety of
user habits.)

Leaving aesthetics and themes completely out of it (this bug is not the place
for that), one -mechanical- piece I think bug 159510 truly got right was to make
the close box smaller and harder to hit by accident.

IMHO what makes the current scheme appear cluttered is that the close box and
favicon are presented as peers.  You could fix that by scaling down the close
button, making it bubble-shaped like the window's (but not necessarily red if
you don't want to), and moving it closer to the upper left.

Then I think each element would be more clearly defined, and they'd no longer
seem to be at cross purposes.

Comment 73

[Geoff "temporarily off bugmail" Beier]

Attachment: All files required to patch HEAD to contain this feature

This is a unified patch incorporating Jasper's latest chrome and fixes for the
last couple visual glitches we identified.

I don't see any flag to check here to indicate it, but I believe this is ready
for review.

Josh, could you begin the code review?

Comment 74

[waverider]

Mr.crot uploaded flexible tabbed Camino this web site :
"Camino 1.7branch 2004/8/15" at http://www.bekkoame.ne.jp/~h-sek/

When I tested it on 10.2.8, Console show this message whenever I created new tab :
WindowServer[178]: CGXRemoveTrackingArea : Invalid tracking area

on 10.3.5, Console don't show this message.

Comment 75

[Josh Aas]

Nits:
- lots of formatting problems all over. I'll list a few examples here
- reverse logic like "nil != tab" to "tab != nil" (constant on right)
     - so with formatting changes, "while( nil != tab ) {" becomes "while (tab
!= nil) {"
- if( (nil == backgroundImage)||(nil == tabButtonDividerImage) ){ <- wrong
  if ((nil == backgroundImage) || (nil == tabButtonDividerImage)) { <- right
- be careful about how you divide lines - don't divide on operators (line 117 of
BrowserTabBarView.mm for exammple)

		[tabButtonDividerImage compositeToPoint:NSMakePoint( tabButtonFrame.origin.x 
			- [tabButtonDividerImage size].width, tabButtonFrame.origin.y ) 
operation:NSCompositeSourceOver];

Instead divide at argument points (:)

- fix indentation between lines 106 and 122 of BrowserTabBarView.mm
- use && to combine "if" statements in lines 163 and 164 of BrowserTabBarView.mm
- any object assigned to "backgroundImage" will leak in BrowerTabBarView if
"loadImages" gets called more than once. The safe thing to do is to pad it and
any other variable like it with a release if the variable is not null before
assigning
- maybe replace
  ------------
  if( !button ) {
    return nil;
  }
  return [button tabViewItem];
  ------------
with 
  ------------
  return (button) ? [button tabViewItem] : nil;
  ------------
lines 301-304 BrowserTabBarView.mm (same at line 136)
- in BrowserTabBarView, can "tabBarDefaultHeight" be constant?


Review of more serious issues later :) the formatting really needs to get
cleaned up before we can continue

Comment 76

[Josh Aas]

Another nit in the licensing...

*  Adapted from BrowserTabViewItem.mm by Geoff Beier
*  Simon Fraser <[e-mail]>

It isn't clear who wrote the file originally and who adapted it. In mathematical
terms, it needs parenthesizing. Does "by Geoff Beier" mean it was originally by
you, or that you are the one who adapted it?

Comment 77

[Josh Aas]

Sorry for so many posts in a row, but here is some more:

In RolloverTrackingCell.mm:

-(void)dealloc
{
  [super dealloc];
  if (mUserData) {
    [mUserData release];
  }
}

Always dealloc super at the end of the method. So:

-(void)dealloc
{
  if (mUserData) {
    [mUserData release];
  }
  [super dealloc];
}

Same thing in TabButtonCell.mm

Also, I don't think you need that check on mUserData. Pink? I think I recall
Pinkerton calling me on that more than once.

---------------------------------------------------

It might not matter because of the same nil checking logic I questioned above,
but the logic behind "mActiveTabButton" is a bit funny. The only place where it
gets a value assigned to it is immediately preceded by [mActiveTabButton
release], which means it must at some point be attempting to release a nil
object. Would be nice to have that cleaned up if its not too hard to do.

---------------------------------------------------

You comments indicate that the current patch puts the favicon on the right. I
have put a lot of thought into this and I really think it should be on the left,
to the right of the close button. People will get used to it, its easier to see
what is going on, and it doesn't violate any standard user interface practices.

Comment 78

[Geoff "temporarily off bugmail" Beier]

Josh,

Thanks for your patient review... as we discussed, I am currently preparing the
patch for a second pass.

(In reply to comment #76)
> Another nit in the licensing...
> 
> *  Adapted from BrowserTabViewItem.mm by Geoff Beier
> *  Simon Fraser <[e-mail]>
> 
> It isn't clear who wrote the file originally and who adapted it. In mathematical
> terms, it needs parenthesizing. Does "by Geoff Beier" mean it was originally by
> you, or that you are the one who adapted it?

That's not clear at all :-). Somehow I got those lines in the wrong order. I
meant to have

* Original Contributor:
*  Simon Fraser <[e-mail]>
*  Adapted from BrowserTabViewItem.mm by Geoff Beier

Is that clearer?

(In reply to comment #77)
> 
> Also, I don't think you need that check on mUserData. Pink? I think I recall
> Pinkerton calling me on that more than once.
> 
> ---------------------------------------------------
> 
> It might not matter because of the same nil checking logic I questioned above,
> but the logic behind "mActiveTabButton" is a bit funny. The only place where it
> gets a value assigned to it is immediately preceded by [mActiveTabButton
> release], which means it must at some point be attempting to release a nil
> object. Would be nice to have that cleaned up if its not too hard to do.
> 

Do you mean that I should check for mActiveTabButton == nil there, or should I
conclude that the nil check is unnecessary? Or do you mean something else
altogether that I've missed?


> ---------------------------------------------------
> 
> You comments indicate that the current patch puts the favicon on the right. I
> have put a lot of thought into this and I really think it should be on the left,
> to the right of the close button. People will get used to it, its easier to see
> what is going on, and it doesn't violate any standard user interface practices.
> 

Sorry. The comments are left over from the way I initially did it. I moved the
favicon as a trial so you and others could see how it would feel... since that
seems to be the preferred way, I'll leave it and update the comment accordingly.

Let me know on the questions above, and I'll fix those along with the formatting.

Thanks again for your help and patience!

Comment 79

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #78)

> Do you mean that I should check for mActiveTabButton == nil there, or should I
> conclude that the nil check is unnecessary? Or do you mean something else
> altogether that I've missed?

The obj-c runtime checks for nil on method calls and fails silently, returning
nil for the expression. you do not need to check for nil here.

Also, i agree with Josh. |if (constant == expression)| is bad form. While
technically it visually separates assignment from comparison, it's much harder
to read because it doesn't flow they way you would read it if you were writing
psuedocode. It's backwards from the way you think about comparisons in natural
language.

Comment 80

[Josh Aas]

> * Original Contributor:
> *  Simon Fraser <[e-mail]>
> *  Adapted from BrowserTabViewItem.mm by Geoff Beier

> Is that clearer?

No - its hard to word correctly. How about:

Contributors:
Geoff Beier <[e-mail]>
Based on BrowserTabViewItem.mm by Simon Fraser <[e-mail]>

Comment 81

[Geoff "temporarily off bugmail" Beier]

Attachment: All files required to patch HEAD to contain this feature

This is the second cut; it should incorporate all comments from the review thus
far plus it removes some code that was no longer necessary (and commented out
in the originally submitted version) and makes the favicon display slightly
better in the absence of the progress indicator.

Comment 82

[Josh Aas]

When I build with the latest patch, it compiles and starts up but no browser
window appears. A message in the console says:

------------------------
2004-08-30 19:37:46.295 Camino[10839] *** -[NSTabViewItem willBeRemoved:]:
selector not recognized

2004-08-30 19:37:46.296 Camino[10839] Exception raised during posting of
notification.  Ignored.  exception: *** -[NSTabViewItem willBeRemoved:]:
selector not recognized
------------------------

Waiting for a new patch before reviewing.

Comment 83

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #82)
> When I build with the latest patch, it compiles and starts up but no browser
> window appears. A message in the console says:
> 
> ------------------------
> 2004-08-30 19:37:46.295 Camino[10839] *** -[NSTabViewItem willBeRemoved:]:
> selector not recognized
> 
> 2004-08-30 19:37:46.296 Camino[10839] Exception raised during posting of
> notification.  Ignored.  exception: *** -[NSTabViewItem willBeRemoved:]:
> selector not recognized
> ------------------------


Generally that happens when the correct .nib is not copied into the final
executable. Assuming you have extracted and copied the included
BrowserWindow.nib to
mozilla/camino/resources/localized/English.lproj/BrowserWindow.nib you need to
make sure to delete Camino.app from the build directory prior to performing your
build within XCode. For some reason XCode doesn't always copy over the .nib to
the final product when it's been modified outside InterfaceBuilder like this.

Geoff

Comment 84

[Josh Aas]

Ugh - I must have forgotten to run the touch command on the nib...

Comment 85

[Josh Aas]

Is a separate BrowserContainerView file necessary for one method? Perhaps you
plan to expand that file in the future and it is easiest to land it that way?
--------------------------------------------------
Nit: There is no need for braces on conditional statements that contain only one
line, e.g.:

if (NSIntersectsRect(tabButtonFrame,rect)) {
  [tabButton drawWithFrame:tabButtonFrame inView:self];
}

Should be:

if (NSIntersectsRect(tabButtonFrame,rect))
  [tabButton drawWithFrame:tabButtonFrame inView:self];

(example is from BrowserTabBarView.mm)
--------------------------------------------------
BrowserTabBarView.mm, line 169

NSLog(@"Here's where we'd handle the drag among friends rather than the drag
manager");

This log message should not be checked in. Please comment it and its conditional
code out.
--------------------------------------------------
BrowserTabBarView.mm, line 389

  BOOL rv = [mDragDest prepareForDragOperation: sender];
  if (rv == NO) {

should be:

  BOOL rv = [mDragDest prepareForDragOperation: sender];
  if (!rv) {
--------------------------------------------------
In some places, like lines 94 and 105 of RolloverTrackingCell.mm, you are using
tabs instead of 2 spaces. You should be able to set that behavior to be correct
for you in the Xcode preferences.
--------------------------------------------------
Starting at line 43 of TabButtonCell.mm, you make a series of #define macros.
These should not be macros. Standard practice is to make those static (and
perhaps const) variables defined at the top of the file or a logically
equivalent place (in this case the same place you placed the #define macros).
--------------------------------------------------
I'm only going to review the new files tonight, and that is all I have to say
about them. With some of the changes I suggested above they are good enough to
go in. I will review the patch tomorrow.

The tabs look great. The close-box on the left with the favicon to its immediate
right looks great. Creating and closing tabs is really fast. I didn't see any
glitches.

I love this patch :)

Comment 86

[Josh Aas]

One problem that I noticed is that the contextual menu text for moving a tab to
a new window is misleading. It doesn't move the tab's actual contents - it just
closes the tab and opens a new window with the same url. This can be
demonstrated with http://time.gov. The could be a problem on pages with forms
where users accidentally terminate sessions or lose previously entered
information, expecting that the tab simply got moved to a new location.

If you can come up with better text, great. Perhaps "Open Tab URL in New
Window." Otherwise we may want to drop this feature until we can make it less
misleading.

Comment 87

[Mike Pinkerton (not reading bugmail)]

yes, #define bad. const good. eg:

const short kFoo = 8;

Comment 88

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #86)
> If you can come up with better text, great. Perhaps "Open Tab URL in New
> Window." Otherwise we may want to drop this feature until we can make it less
> misleading.

This patch does not change the menu display in any way; it simply uses the
existing contextual menu. As such, I'd prefer to make that change as a separate
patch but if you think that needs to be part of this one for it to land I'll add
that.

(In reply to comment #87)
> yes, #define bad. const good. eg:
> 
> const short kFoo = 8;

I agree. I was blindly copying BookmarksToolbar.mm for the style here :-). I'll
make that and the other changes Josh cited, bar the context menu, later today
and repost. (Unless you guys think the contextual menu change needs to be
included with this patch.)

Comment 89

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #85)
> Is a separate BrowserContainerView file necessary for one method? Perhaps you
> plan to expand that file in the future and it is easiest to land it that way?

Sorry for the spam... I missed this on the first pass. The reason I separated
BrowserContainerView out in the first place was because I was adding a couple of
other methods and needed to access them outside of BrowserContentView. If you
guys prefer, I will return this class to its original location, as neither of
these is true at this time. I don't have any specific plans to expand it, as I
have moved the logic that I originally had there elsewhere. Let me know what the
preference is here.

Comment 90

[Josh Aas]

Review of patch part of the package...
-----------------------------------
In the contex for your patch, there is the line in BrowserTabView.mm:

 // Only to be used with the 2 types of tab view which we use in Chimera.

If you respin the patch please replace the reference to Chimera with Camino.
-----------------------------------
Please visually separate comments from the end of a line of code:

NSRect  iconRect   = [self convertRect: [mLabelCell imageFrame] fromView:
nil];//NSMakeRect(0, 0, 16, 16);

Either put spaces around the "//" or put it above the line.
-----------------------------------
+  if( remove ) {

formatting. should be:

if (remove) {
-----------------------------------
None of this stuff is serious, so just do it if you respin the patch.

Comment 91

[Geoff "temporarily off bugmail" Beier]

Attachment: All files required to patch HEAD to contain this feature

This iteration should address all of Josh's comments to date. While doing that
I found and fixed a potential bug in the drag receiving code. That is included
as well.

Comment 92

[Ludovic Hirlimann [:Usul]]

Comment on attachment 157618 [details]
All files required to patch HEAD to contain this feature

r+ with comment given on IRC being taken care of :
1) nil ==
2) CVS in the tar file
3) Bad Licences headers

Nice work.

Comment 93

[Geoff "temporarily off bugmail" Beier]

Attachment: All files required to patch HEAD to contain this feature

This is identical to the last one except that, per [e-mail]'s
review:

1. License headers are corrected.
2. Missed constant is corrected.
3. CVS directory is removed from nib archive
4. browser.patch is now produced with diff -u2

Comment 94

[Josh Aas]

Comment on attachment 157643 [details]
All files required to patch HEAD to contain this feature

this review+ assuming Geoff only changed what he said he changed

Comment 95

[mike]

is there a time estimate on when this new tabs code will be adopted by the
nightly builds?

Comment 96

[Ludovic Hirlimann [:Usul]]

patch has  landed