Closed Bug 235782 Opened 21 years ago Closed 20 years ago

A new, more flexible tabbed browsing control for Camino

Categories

(Camino Graveyard :: Tabbed Browsing, enhancement)

Product:
Camino Graveyard
Component:
Tabbed Browsing
Platform:
PowerPC
macOS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino0.9

People

(Reporter: me, Assigned: mikepinkerton)

Details

Attachments

(9 files, 17 obsolete files)

obligatory screenshot
75.02 KB, image/png
Details
The screen shot when opening tab of the maximum number(16 tabs)
183.04 KB, image/png
Details
the screenshot
107.75 KB, image/pdf
Details
the screenshot
145.39 KB, image/png
Details
screenshot using Aqua interface
119.15 KB, image/png
Details
screenshot using Graphite interface
125.06 KB, image/png
Details
screenshot of Mail.app
19.73 KB, image/png
Details
Crash log
30.25 KB, text/plain
Details
All files required to patch HEAD to contain this feature
39.28 KB, application/x-gzip
jaas
: review+
Usul
: review+
Details
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.
Attached file archive of the NIB file (obsolete) —
Attached file images for the tab chrome (obsolete) —
Confirming as an RFE.
Status: UNCONFIRMED → NEW
Component: General → Tabbed Browsing
Ever confirmed: true
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 ?
Attached file archive of the NIB file (obsolete) —
Got the wrong BrowserWindow.nib earlier
Attachment #142408 - Attachment is obsolete: true
minor resource tweaks
Attachment #142409 - Attachment is obsolete: true
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...
jerome, bugzilla has nothing against tiff files. can you put them in a zip and attach that if that's easier for you?
(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 :-)
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?
I think the active tab should be clearly highlighted against the inactive ones and inactive ones should be dimmed.
Attached image the screenshot —
I tried to replace active_tab_bg with active_tab_bg. The active tab is clearly highlighted.
Attached image the screenshot —
Sorry, I've mistaken. I tried to replace active_tab_bg with active_tab_bg. The active tab is clearly highlighted.
Attachment #143101 - Attachment mime type: application/pdf → image/pdf
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.
(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?
> 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
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.
In the same concept I tried Graphite interface.
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.
Attached image 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.
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.
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.
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 ;-)
(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.
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.
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.
Attached file 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.
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.
(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.
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
Target Milestone: --- → Camino0.9
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.
(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.
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.
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.
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?
Geoff, Any chance we can get this checked into the trunk. The more eyes we can get to bang on this feature the better...
(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
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
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.
Geoff - status update?
(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.
sounds good :) thanks
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
Attachment #142406 - Attachment is obsolete: true
Attached file additional source files (obsolete) —
Attachment #142407 - Attachment is obsolete: true
Any known issues?
(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.
Attached file Archive of the .nib file (obsolete) —
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.
Attachment #142974 - Attachment is obsolete: true
(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.
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.
Attachment #155663 - Attachment is obsolete: true
Attached file additional source files (obsolete) —
The new classes, reflecting the same tweaks as the patch, uploaded separately because I couldn't get the cvs diff to include them.
Attachment #155665 - Attachment is obsolete: true
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?
Ugh. I meant no browser with tabs puts the favicon on the right. sheesh
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.
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 :)
(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.
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.
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.
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.
Attachment #142975 - Attachment is obsolete: true
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.
Attachment #156010 - Attachment is obsolete: true
Attached file additional source files (obsolete) —
Goes with the latest revision of the patch...
Attachment #156011 - Attachment is obsolete: true
(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 :-))
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.
(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.
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.
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!
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!
(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.
(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.
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?
Attachment #155819 - Attachment is obsolete: true
Attachment #156192 - Attachment is obsolete: true
Attachment #156193 - Attachment is obsolete: true
Attachment #156194 - Attachment is obsolete: true
Attachment #156316 - Flags: review?
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.
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
Another nit in the licensing... * Adapted from BrowserTabViewItem.mm by Geoff Beier * Simon Fraser <sfraser@netscape.com> 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?
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.
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 <sfraser@netscape.com> > > 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 <sfraser@netscape.com> * 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!
(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.
> * Original Contributor: > * Simon Fraser <sfraser@netscape.com> > * Adapted from BrowserTabViewItem.mm by Geoff Beier > Is that clearer? No - its hard to word correctly. How about: Contributors: Geoff Beier <me@mollyandgeoff.com> Based on BrowserTabViewItem.mm by Simon Fraser <sfraser@netscape.com>
Attachment #156316 - Flags: review?
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.
Attachment #156316 - Attachment is obsolete: true
Attachment #157382 - Flags: review?(joshmoz)
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.
(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
Ugh - I must have forgotten to run the touch command on the nib...
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 :)
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.
yes, #define bad. const good. eg: const short kFoo = 8;
(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.)
(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.
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.
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.
Attachment #157382 - Attachment is obsolete: true
Attachment #157382 - Flags: review?(joshmoz)
Attachment #157618 - Flags: review?(joshmoz)
Attachment #157618 - Flags: review?(qa-mozilla)
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.
Attachment #157618 - Flags: review?(qa-mozilla) → review+
This is identical to the last one except that, per qa-mozilla@hirlimann.net'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
Attachment #157618 - Attachment is obsolete: true
Attachment #157643 - Flags: superreview?(pinkerton)
Attachment #157643 - Flags: review?(joshmoz)
Attachment #157618 - Flags: review?(joshmoz)
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
Attachment #157643 - Flags: review?(joshmoz) → review+
is there a time estimate on when this new tabs code will be adopted by the nightly builds?
patch has landed
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #157643 - Flags: superreview?(pinkerton)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: