User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030328 Chimera/0.7+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030328 Chimera/0.7+ The window title of the in-window bookmark manager is inherited from the window previously displayed. Reproducible: Always Steps to Reproduce: 1. Select "Bookmarks...Manage Bookmarks" 2. In-window bookmark manager opens. 3. Actual Results: Bookmark manager window and window menu retains prior window's title. Expected Results: Titlebar of in-window bookmark manager and window menu list should state "Bookmarks"

Shouldn't the window title continue to reflect the URL bar, which won't change when bookmarks are being managed? Also, Safari behaves the same way.

Perhaps if the url bar said something like "about:bookmarks" until the bookmark manager session is completed and the user returns to the previous page.

->pink

sure whatever

I'm with Greg on this one, the title should not be changed while the manager is visible. There are two reasons for this: 1) The user can easily know that the browser session has not been lost and that the currently open page is still open, and 2) The user can easily identify an open window through the Dock menu or through the Window menu. Imagine having two windows (with individual session histories) with the bookmarks manager open -- there'd be no quick and easy way of telling which window is which if the title bars had the same text. In other words, just do what Safari does -- in other words, do what Camino currently does.

Lauri, re: statement (1), when the user selects the bookmark manager, the current page is replaced, just as if they had selected an ordinary bookmark, and the user must hit the 'back' command to return to the prior content. I think users can grasp this, and it would lessen confusion if the title reflected current context rather than prior content. Re: statement (2), say you navigate to site "foo," select Bookmark Manager, and put the manager in the background (with title "foo"). Browse in a foreground window to site "foo" again. Go to the Window Menu and "foo" is duplicated, but one instance the content is "foo," the second instance the content is bookmark manager. This defeats the Window Menu. How does the user know what to select?

A more obvious case of this bug appears when all windows are closed and the "Bookmarks...Manage Bookmarks" menu item is selected. The Bookmark Manager appears with the user's homepage in the window title, the homepage URL in the location box, and the "Back" button active. Hitting the "Back" button results in a blank page, not the homepage. Also, the icon in the location box is generic, not a URL "globe."

Updating status at Pink's request. Camino Build ID: 2003081810 - Bookmark manager continues to inherit the title of the preceding page. When bookmark manager is launched with no windows open, it fills the homepage URL in the address box and activates the 'back' toolbar button, but the 'back' function does not work.

1) patch does not set the backbutton when no URL is loaded. 2) Changes the window title to BookmarkManager while managing bookmarks. 3) Changes the window's title to it's original name when preesing the back button.

What this for ? + if ( [mContentView isBookmarkManagerVisible] && ![[[self getBrowserWrapper] getCurrentURLSpec] isEqualToString:@"about:blank"] ) I don't like the title "BookmarksManager" I think it should be "Bookmarks Manager" with a space. It should be localizable.

First line is here so when I trigger the bookmark view I can't press the back button, if I did not load any page in the tabe before. will work on localizing the string.

i think it's important to always have the back button enabled when the bm manager is displayed, regardless of whether there's a webpage to go back to. mostly for consistentcy.

The title sting is now localizeable. Took care of comment #10 and #12.

Comment on attachment 135558 [details] [diff] [review] Patch v1.0 r+

crash log

Comment on attachment 135558 [details] [diff] [review] Patch v1.0 This contains a crasher. I can reproduce consistently: I have this bookmarked: http://www.smartmobs.com/index.html Enter bm manager, open that bookmark. open manager bookmarks again, open that bookmark again, it crashes.

The window's title needs to be resstored only when the back button is hit.

Patch now handles the bookmark manager and the History view. Previous patch would call the history window bookmark manager.

Comment on attachment 135649 [details] [diff] [review] V 2.0 did you test this?! It still crashes!

Simon: How do you crash it ? I've been playing a lot with it before uploading the new patch.

reproduce crash: 1. I have the following bookmarks <some folders> http://www.scottmccloud.com/ [A] http://www.biglist.com/lists/xsl-list/archives/200009/msg00763.html [B] http://www.comminit.com/ctrends2003/sld-7642.html [C] <other bookmarks> 2. Launch camino 3. Cmd-B, double click on A (wait for it to load) 4. Cmd-B, double-click on B (wait for it to load) 5. Cmd-B, double-click on C (wait for it to load) 6. Close the window 7. crashes Also, in the patch it says "Bookmark Manger" should be Manager. BTW, I believe that Localizable.strings is a UTF-16 encoded text file.

I should learn to read comments. Fixed the simon's crash. Fixede the "manger" string issues. Copying/Pasting is not always a good thing.

OK, it doesn't crash any more :) When I click on the toolbar button to manage bookmarks (it still says toggle sidebar) the title doesn't change. For example, If I Cmd-B and then click the tb button, the title stays as "Bookmark Manager".

fixing the sidebar icon issue. Is there anything else I've missed ?

I see some problems. 1) mSavedTitle. You're saving it in one method then using it in another without having retained it. What's keeping it from being released out from under you? This seems like a crash waiting to happen. 2) Let's say you hit CMD-Y to open up the history panel. Then you click on the "Bookmark Toolbar" collection. Won't the title still say "History Manager"? Or if you open up the bookmark manager then click on the history folder, the window title will still be "Bookmark Manager". That's no good. I say, pick one name for the thing and stick with it - "History Manager", "Bookmark Manager", "Various Things", or something else entirely - one name, and that's it.

Hmm... maybe "Collections Manager" ? It would kind of suggest the idea that it's lists of collections of links of various sorts and sizes.

Or what about "Link manager"? Bookmarks and Hitory instances are all links!

I've setled for "Link Managment".

Addressing issues of comment #27.

I think that the two lines that look like this: + [[self window] setTitle:mSavedTitle]; are leaking .. they should be + [[self window] setTitle:[mSavedTitle autorelease]]; (since you retained it earlier you must release it) BTW You don't need to retain strings created with @"" construction so those ones are OK.

I think "Links Manager" is awkward. How about we just call the whole thing "Bookmarks Manager" even if they use Cmd-Y ... ?

so the following patch will work.

No, this still seems bad. mSavedTitle is going to be saved but occasionally invalid, which is just asking for trouble. Make 2 methods: -(NSString *)savedTitle and -(void)setSavedTitle:(NSString *)aTitle. savedTitle returns mSavedTitle. setSavedTitle replaces mSavedTitle with aTitle. Handle retain/release of mSavedTitle in setSavedTitle. The end.

Taking into acount dave's comment. And simon's tips on memory management.

Comment on attachment 136827 [details] [diff] [review] v2.6 buggy version.

Comment on attachment 136827 [details] [diff] [review] v2.6 >+- (void)setSavedTitle:(NSString *)aTitle; >+{ >+ id old = mSavedTitle; >+ mSavedTitle=[aTitle retain]; >+ [old retain]; >+} You've retained "old" when you meant to release it. The classic pattern (non-threadsafe, but that's OK) is: - (void)setSavedTitle:(NSString *)aTitle; { [aTitle retain]; [mSavedTitle release]; mSavedTitle = aTitle; }

Comment on attachment 136849 [details] [diff] [review] should this time r+

This patch just plain doesn't work for me. When I switch to the bookmark manager, it shows the bookmark manager title but when I switch back by hitting the toolbar button it never restores the correct title. Here is some other stuff: 1. You need to update Localizable.strings with the key/value "Bookmark Manager" 2. You need to end the Localizable.string entry with a semicolon 3. When you write statements like "mSavedTitle=nil;" please put a space to the left and right of the "=" operator 4. Nit - the open block bracket should not be on its own line for "if" statements (e.g. "if (...) {" not "if (...) \n...{" 5. Maybe point out somewhere in the comments that pageloading is halted with the bm manager is open. Otherwise this patch might restore an incorrect title if the page redirected while the bm manager was open. 6. In your patch here: +// save the window title before showing +// bookmark manager or History manager +- (void)setSavedTitle:(NSString *)aTitle; +{ + id old = mSavedTitle; Don't end the method signature with a semicolon. 7. Same thing as in point 3 - spaces around the "=" operator here: mSavedTitle=[aTitle retain]; 8. Here: +// save the window title before showing +// bookmark manager or History manager +- (void)setSavedTitle:(NSString *)aTitle; +{ + id old = mSavedTitle; + mSavedTitle=[aTitle retain]; + [old release]; +} + Maybe just use autorelease, like this: +// save the window title before showing +// bookmark manager or History manager +- (void)setSavedTitle:(NSString *)aTitle; +{ + [mSavedTitle autorelease]; + mSavedTitle = [aTitle retain]; +} + You implementation is <maybe> slightly faster but is more confusing at a glance.

Ludovic - you didn't do anything about comments 2 and 6. Other than that r=josha@mac.com Mike - here's the best way to land this: use Ludo's patch, but just fix my comment 6 quickly after you apply. Then don't bother with the localizable.string patch (this will avoid conflicts and my comment #2 problem), just add these lines to your localizable.strings: /* Bookmark View History View */ "Bookmark Management" = "Bookmark Management";

[self toggleBookmarkManager:self]; + [[self window] setTitle:[self savedTitle]]; why isn't the |-setTitle:| part of |-toggleBookmarkManager:|?

This is a new patch that fixes all of my nits and gets rid of all the duplicate work being done. Apply this patch and add the lines described in my comment above to Localizable.strings.

I just realized my patch leaks mSavedTitle when the browser window closes. I'll update it soon.

Comment on attachment 138652 [details] [diff] [review] fix memory leak Never mind... I need more time with this patch

Seriously - this one works :)

Comment on attachment 138703 [details] [diff] [review] hopefully the last revision sr=pink

landed

I was getting ready to verify fixed and discovered one nit related to comment #7: Close all windows, Select "Bookmarks...Show All Bookmarks," Bookmark Manager opens with homepage URL in address bar and Back button active, but Back button does not take user to the homepage URL as expected. File separate bug or handle here?

please file a new bug, thanks.

OK, I'll mark Verified Fixed.