223209 - [patch] New bookmark manager , bookmark view clean-up

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Ludovic Hirlimann [:Usul]]

This 
a)makes sure the bookmarks are visible in the event Camino was launched via an
apple-event
b)pops open the search result window when you do a search, in the event it was
hidden
c)preserves the order of bookmarks dropped in a mass drag & drop (current
behavior reverses their order in a folder)
d)fixes some button states appropriately depending on what folder is selected
e)gets rid of a useless method & cleans up a differnt method.

Comment 1

[Ludovic Hirlimann [:Usul]]

Attachment: david's patch

Comment 2

[S Woodside]

Comment on attachment 133833 [details] [diff] [review]
david's patch

adding review? since I assume that's what you're looking for here.

 - (float)splitView:(NSSplitView *)sender
constrainMinCoordinate:(float)proposedCoord ofSubviewAt:(int)offset
 {
-  const int kMinimumContainerSplitWidth = 150;
-  float retVal = proposedCoord;
   if ( sender == mContainersSplit )
-    retVal = kMinimumContainerSplitWidth;
-  return retVal;
+    return 150; //minimum size of collections pane
+  else
+    return proposedCoord;

Dave, I don't like that you removed the definition for the value there. I think
it would be better to keep avoid hard-coding a number like that 150 into the
code. The way it was before was maybe not ideal either, but what you did is
worse I think.

+    mRootBookmarks = [[[BookmarkManager sharedBookmarkManager] rootBookmarks]
retain];
+    if (!mRootBookmarks)
+      return;

Just return? Shouldn't you log something, complain, beep(), etc?

I've got the patch installed and it works. If you fix those two issues in the
code, I'll review+

Comment 3

[S Woodside]

Added CC for david. David, please have a look at my comment #2 
http://bugzilla.mozilla.org/show_bug.cgi?id=223209#c2

Comment 4

[David Haas (gone, not reading mail)]

Attachment: updated

updated.  didn't need the check for mBookmarkManager, so I pitched it.	And I
added back in the const int rather than just run with the 150.

Comment 5

[Ludovic Hirlimann [:Usul]]

Comment on attachment 134196 [details] [diff] [review]
updated

>+    if (searchFrame.size.height < 80) {
>+      searchFrame.size.height = 80;
>+      bookmarkFrame.size.height -= 80;

Could this 80 be #defined somwhere ?

Comment 6

[David Haas (gone, not reading mail)]

Attachment: even more updated

I #defined the 80 & 150 in the header.

Comment 7

[Mike Pinkerton (not reading bugmail)]

#define bad. const long or enum good.

Comment 8

[Mike Pinkerton (not reading bugmail)]

landed w/out #defines.