199790 - [patch]Bookmark manager window inherits wrong window title

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Warren TenBrook]

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"

Comment 1

[Greg K.]

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.

Comment 2

[Warren TenBrook]

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.

Comment 3

[Simon Fraser [no longer active]]

->pink

Comment 4

[Mike Pinkerton (not reading bugmail)]

sure whatever

Comment 5

[Lauri Kieksi]

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.

Comment 6

[Warren TenBrook]

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?

Comment 7

[Warren TenBrook]

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."

Comment 8

[Warren TenBrook]

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.

Comment 9

[Ludovic Hirlimann [:Usul]]

Attachment: making Bookmarkmanager the title when editing bookmarks.

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.

Comment 10

[S Woodside]

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.

Comment 11

[Ludovic Hirlimann [:Usul]]

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.

Comment 12

[Mike Pinkerton (not reading bugmail)]

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.

Comment 13

[Ludovic Hirlimann [:Usul]]

Attachment: Localized.strings can't be diffed because they are binarried by PB.

Comment 14

[Ludovic Hirlimann [:Usul]]

Attachment: Patch v1.0

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

Comment 15

[S Woodside]

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

r+

Comment 16

[S Woodside]

Attachment: crash log

crash log

Comment 17

[S Woodside]

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.

Comment 18

[Ludovic Hirlimann [:Usul]]

Attachment: Fixing comment #17 crash

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

Comment 19

[Ludovic Hirlimann [:Usul]]

Attachment: Strings to deal Bookmark and history.

Comment 20

[Ludovic Hirlimann [:Usul]]

Attachment: V 2.0

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

Comment 21

[S Woodside]

Comment on attachment 135649 [details] [diff] [review]
V 2.0

did you test this?! It still crashes!

Comment 22

[Ludovic Hirlimann [:Usul]]

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

Comment 23

[S Woodside]

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.

Comment 24

[Ludovic Hirlimann [:Usul]]

Attachment: v2.1

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

Comment 25

[S Woodside]

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".

Comment 26

[Ludovic Hirlimann [:Usul]]

Attachment: v2.2

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

Comment 27

[David Haas (gone, not reading mail)]

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.  

Comment 28

[S Woodside]

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

Comment 29

[Jasper]

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

Comment 30

[Ludovic Hirlimann [:Usul]]

Attachment: Localized.strings.gz

I've setled for "Link Managment".

Comment 31

[Ludovic Hirlimann [:Usul]]

Attachment: v2.3

Addressing issues of comment #27.

Comment 32

[S Woodside]

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.

Comment 33

[S Woodside]

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

Comment 34

[Ludovic Hirlimann [:Usul]]

Attachment: Bookmark Management

so the following patch will work.

Comment 35

[Ludovic Hirlimann [:Usul]]

Attachment: v2.4

Comment 36

[David Haas (gone, not reading mail)]

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.

Comment 37

[Ludovic Hirlimann [:Usul]]

Attachment: v2.6

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

Comment 38

[Ludovic Hirlimann [:Usul]]

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

buggy version.

Comment 39

[David Haas (gone, not reading mail)]

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 40

[Ludovic Hirlimann [:Usul]]

Attachment: should this time

Comment 41

[S Woodside]

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

r+

Comment 42

[Josh Aas]

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.

Comment 43

[Ludovic Hirlimann [:Usul]]

Attachment: Patch taking in to account josh's comment.

Comment 44

[Josh Aas]

Ludovic - you didn't do anything about comments 2 and 6. Other than that
r=[e-mail]

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";

Comment 45

[Mike Pinkerton (not reading bugmail)]

     [self toggleBookmarkManager:self];
+    [[self window] setTitle:[self savedTitle]];

why isn't the |-setTitle:| part of |-toggleBookmarkManager:|?

Comment 46

[Josh Aas]

Attachment: updated version of Ludo's patch (slimmed down)

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.

Comment 47

[Josh Aas]

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

Comment 48

[Josh Aas]

Attachment: fix memory leak

Comment 49

[Josh Aas]

Comment on attachment 138652 [details] [diff] [review]
fix memory leak

Never mind... I need more time with this patch

Comment 50

[Josh Aas]

Attachment: hopefully the last revision

Seriously - this one works :)

Comment 51

[Mike Pinkerton (not reading bugmail)]

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

sr=pink

Comment 52

[Mike Pinkerton (not reading bugmail)]

landed

Comment 53

[Warren TenBrook]

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?

Comment 54

[Mike Pinkerton (not reading bugmail)]

please file a new bug, thanks.

Comment 55

[Warren TenBrook]

OK, I'll mark Verified Fixed.