diff options
| author | snej@chromium.org <snej@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-02-04 22:13:10 +0000 |
|---|---|---|
| committer | snej@chromium.org <snej@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-02-04 22:13:10 +0000 |
| commit | 9f799fdaeff018930b6123bdefbf18e98e857c22 (patch) | |
| tree | 7b59b6e18449d5653162874f60f39511e610ac63 /chrome | |
| parent | bf97cfbd3c36b2f938f522ae97f82288ef60459e (diff) | |
| download | chromium_src-9f799fdaeff018930b6123bdefbf18e98e857c22.zip chromium_src-9f799fdaeff018930b6123bdefbf18e98e857c22.tar.gz chromium_src-9f799fdaeff018930b6123bdefbf18e98e857c22.tar.bz2 | |
Mac: fix crash deleting bookmark while bookmark editor is open.
(xib change: enable 'preserve selection' in NSOutlineView.)
BUG=33333
TEST=BookmarkEditorBaseControllerTest.SelectedFolderDeleted et al
Review URL: http://codereview.chromium.org/566005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38139 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
6 files changed, 197 insertions, 6 deletions
diff --git a/chrome/app/nibs/BookmarkEditor.xib b/chrome/app/nibs/BookmarkEditor.xib index 71e4ab9..0639ab7 100644 --- a/chrome/app/nibs/BookmarkEditor.xib +++ b/chrome/app/nibs/BookmarkEditor.xib @@ -417,8 +417,10 @@ <string>folderName</string> <string>newFolder</string> </object> + <string key="NSObjectClassName">BookmarkFolderInfo</string> <object class="_NSManagedProxy" key="_NSManagedProxy"/> <bool key="NSAvoidsEmptySelection">YES</bool> + <bool key="NSPreservesSelection">YES</bool> <string key="NSTreeContentChildrenKey">children</string> </object> </object> diff --git a/chrome/browser/cocoa/bookmark_editor_base_controller.h b/chrome/browser/cocoa/bookmark_editor_base_controller.h index 265289f..bb447b5 100644 --- a/chrome/browser/cocoa/bookmark_editor_base_controller.h +++ b/chrome/browser/cocoa/bookmark_editor_base_controller.h @@ -12,6 +12,7 @@ #include "base/scoped_nsobject.h" #include "chrome/browser/bookmarks/bookmark_editor.h" +class BookmarkEditorBaseControllerBridge; class BookmarkModel; @class BookmarkTreeBrowserCell; @@ -42,6 +43,8 @@ class BookmarkModel; // Bound to the table view giving a path to the current selections, of which // there should only ever be one. scoped_nsobject<NSArray> tableSelectionPaths_; + // C++ bridge object that observes the BookmarkModel for me. + scoped_ptr<BookmarkEditorBaseControllerBridge> observer_; } @property (copy) NSString* initialName; @@ -98,6 +101,11 @@ class BookmarkModel; // Notify the handler, if any, of a node creation. - (void)notifyHandlerCreatedNode:(const BookmarkNode*)node; +// Notifications called when the BookmarkModel changes out from under me. +- (void)nodeRemoved:(const BookmarkNode*)node + fromParent:(const BookmarkNode*)parent; +- (void)modelChanged; + // Accessors - (BookmarkModel*)bookmarkModel; - (const BookmarkNode*)parentNode; diff --git a/chrome/browser/cocoa/bookmark_editor_base_controller.mm b/chrome/browser/cocoa/bookmark_editor_base_controller.mm index 59b8913..c467e09 100644 --- a/chrome/browser/cocoa/bookmark_editor_base_controller.mm +++ b/chrome/browser/cocoa/bookmark_editor_base_controller.mm @@ -17,13 +17,23 @@ #include "chrome/browser/profile.h" #include "grit/generated_resources.h" -@interface BookmarkEditorBaseController (Private) +@interface BookmarkEditorBaseController () @property (retain, readwrite) NSArray* folderTreeArray; // Return the folder tree object for the given path. - (BookmarkFolderInfo*)folderForIndexPath:(NSIndexPath*)path; +// (Re)build the folder tree from the BookmarkModel's current state. +- (void)buildFolderTree; + +// Notifies the controller that the bookmark model has changed. +- (void)modelChanged; + +// Notifies the controller that a node has been removed. +- (void)nodeRemoved:(const BookmarkNode*)node + fromParent:(const BookmarkNode*)parent; + // Given a folder node, collect an array containing BookmarkFolderInfos // describing its subchildren which are also folders. - (NSMutableArray*)addChildFoldersFromNode:(const BookmarkNode*)node; @@ -66,6 +76,79 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, [controller runAsModalSheet]; } +// Adapter to tell BookmarkEditorBaseController when bookmarks change. +class BookmarkEditorBaseControllerBridge : public BookmarkModelObserver { + public: + BookmarkEditorBaseControllerBridge(BookmarkEditorBaseController* controller) + : controller_(controller), + importing_(false) + { } + + virtual void Loaded(BookmarkModel* model) { + [controller_ modelChanged]; + } + + virtual void BookmarkNodeMoved(BookmarkModel* model, + const BookmarkNode* old_parent, + int old_index, + const BookmarkNode* new_parent, + int new_index) { + if (!importing_ && new_parent->GetChild(new_index)->is_folder()) + [controller_ modelChanged]; + } + + virtual void BookmarkNodeAdded(BookmarkModel* model, + const BookmarkNode* parent, + int index) { + if (!importing_ && parent->GetChild(index)->is_folder()) + [controller_ modelChanged]; + } + + virtual void BookmarkNodeRemoved(BookmarkModel* model, + const BookmarkNode* parent, + int old_index, + const BookmarkNode* node) { + [controller_ nodeRemoved:node fromParent:parent]; + if (node->is_folder()) + [controller_ modelChanged]; + } + + virtual void BookmarkNodeChanged(BookmarkModel* model, + const BookmarkNode* node) { + if (!importing_ && node->is_folder()) + [controller_ modelChanged]; + } + + virtual void BookmarkNodeChildrenReordered(BookmarkModel* model, + const BookmarkNode* node) { + if (!importing_) + [controller_ modelChanged]; + } + + virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model, + const BookmarkNode* node) { + // I care nothing for these 'favicons': I only show folders. + } + + virtual void BookmarkImportBeginning(BookmarkModel* model) { + importing_ = true; + } + + // Invoked after a batch import finishes. This tells observers to update + // themselves if they were waiting for the update to finish. + virtual void BookmarkImportEnding(BookmarkModel* model) { + importing_ = false; + [controller_ modelChanged]; + } + + private: + BookmarkEditorBaseController* controller_; // weak + bool importing_; +}; + + +#pragma mark - + @implementation BookmarkEditorBaseController @synthesize initialName = initialName_; @@ -88,11 +171,14 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, configuration_ = configuration; handler_.reset(handler); initialName_ = [@"" retain]; + observer_.reset(new BookmarkEditorBaseControllerBridge(self)); + [self bookmarkModel]->AddObserver(observer_.get()); } return self; } - (void)dealloc { + [self bookmarkModel]->RemoveObserver(observer_.get()); [initialName_ release]; [displayName_ release]; [super dealloc]; @@ -115,11 +201,7 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, } // Build up a tree of the current folder configuration. - BookmarkModel* model = profile_->GetBookmarkModel(); - const BookmarkNode* rootNode = model->root_node(); - NSMutableArray* baseArray = [self addChildFoldersFromNode:rootNode]; - DCHECK(baseArray); - [self setFolderTreeArray:baseArray]; + [self buildFolderTree]; } - (void)windowDidLoad { @@ -284,6 +366,7 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, const BookmarkNode* rootNode = model->root_node(); const BookmarkNode* node = desiredNode; while (node != rootNode) { + DCHECK(node); nodeStack.push(node); node = node->GetParent(); } @@ -332,6 +415,42 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, return childFolders; } +- (void)buildFolderTree { + // Build up a tree of the current folder configuration. + BookmarkModel* model = profile_->GetBookmarkModel(); + const BookmarkNode* rootNode = model->root_node(); + NSMutableArray* baseArray = [self addChildFoldersFromNode:rootNode]; + DCHECK(baseArray); + [self setFolderTreeArray:baseArray]; +} + +- (void)modelChanged { + const BookmarkNode* selectedNode = [self selectedNode]; + [self buildFolderTree]; + if (selectedNode && configuration_ == BookmarkEditor::SHOW_TREE) + [self selectNodeInBrowser:selectedNode]; +} + +- (void)nodeRemoved:(const BookmarkNode*)node + fromParent:(const BookmarkNode*)parent { + if (node->is_folder()) { + if (parentNode_ == node || parentNode_->HasAncestor(node)) { + parentNode_ = [self bookmarkModel]->GetBookmarkBarNode(); + if (configuration_ != BookmarkEditor::SHOW_TREE) { + // The user can't select a different folder, so just close up shop. + [self cancel:self]; + return; + } + } + + if (configuration_ == BookmarkEditor::SHOW_TREE) { + // For safety's sake, in case deleted node was an ancestor of selection, + // go back to a known safe place. + [self selectNodeInBrowser:parentNode_]; + } + } +} + #pragma mark New Folder Handler - (void)createNewFoldersForFolder:(BookmarkFolderInfo*)folderInfo { @@ -464,4 +583,11 @@ void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd, [super dealloc]; } +// Implementing isEqual: allows the NSTreeController to preserve the selection +// and open/shut state of outline items when the data changes. +- (BOOL)isEqual:(id)other { + return [other isKindOfClass:[BookmarkFolderInfo class]] && + folderNode_ == [(BookmarkFolderInfo*)other folderNode]; +} + @end diff --git a/chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm b/chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm index 698276e..7446357 100644 --- a/chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm +++ b/chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm @@ -162,6 +162,43 @@ TEST_F(BookmarkEditorBaseControllerTest, CreateFolder) { [controller_ cancel:nil]; } +TEST_F(BookmarkEditorBaseControllerTest, SelectedFolderDeleted) { + BookmarkModel& model(*(browser_helper_.profile()->GetBookmarkModel())); + [controller_ selectTestNodeInBrowser:group_b_3_]; + EXPECT_EQ(group_b_3_, [controller_ selectedNode]); + + // Delete the selected node, and verify it's no longer selected: + model.Remove(group_b_, 3); + EXPECT_NE(group_b_3_, [controller_ selectedNode]); + + [controller_ cancel:nil]; +} + +TEST_F(BookmarkEditorBaseControllerTest, SelectedFoldersParentDeleted) { + BookmarkModel& model(*(browser_helper_.profile()->GetBookmarkModel())); + const BookmarkNode* root = model.GetBookmarkBarNode(); + [controller_ selectTestNodeInBrowser:group_b_3_]; + EXPECT_EQ(group_b_3_, [controller_ selectedNode]); + + // Delete the selected node's parent, and verify it's no longer selected: + model.Remove(root, 1); + EXPECT_NE(group_b_3_, [controller_ selectedNode]); + + [controller_ cancel:nil]; +} + +TEST_F(BookmarkEditorBaseControllerTest, FolderAdded) { + BookmarkModel& model(*(browser_helper_.profile()->GetBookmarkModel())); + const BookmarkNode* root = model.GetBookmarkBarNode(); + + // Add a group node to the model, and verify it can be selected in the tree: + const BookmarkNode* group_added = model.AddGroup(root, 0, L"added"); + [controller_ selectTestNodeInBrowser:group_added]; + EXPECT_EQ(group_added, [controller_ selectedNode]); + + [controller_ cancel:nil]; +} + class BookmarkFolderInfoTest : public CocoaTest { }; diff --git a/chrome/browser/cocoa/bookmark_editor_controller.mm b/chrome/browser/cocoa/bookmark_editor_controller.mm index f471f1f..364feec 100644 --- a/chrome/browser/cocoa/bookmark_editor_controller.mm +++ b/chrome/browser/cocoa/bookmark_editor_controller.mm @@ -60,6 +60,16 @@ [super awakeFromNib]; } +- (void)nodeRemoved:(const BookmarkNode*)node + fromParent:(const BookmarkNode*)parent +{ + if (node_ == node || node_->HasAncestor(node)) { + // The node I'm editing is being deleted. Just close. + node_ = NULL; + [self cancel:self]; + } +} + #pragma mark Bookmark Editing // If possible, return a valid GURL from the URL text field. diff --git a/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm b/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm index 5e9f947..8aac9ab 100644 --- a/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm +++ b/chrome/browser/cocoa/bookmark_editor_controller_unittest.mm @@ -81,6 +81,14 @@ TEST_F(BookmarkEditorControllerTest, EditAndFixPrefix) { EXPECT_TRUE(child->GetURL().is_valid()); } +TEST_F(BookmarkEditorControllerTest, NodeDeleted) { + // Delete the bookmark being edited and verify the sheet cancels itself: + ASSERT_TRUE([test_window() attachedSheet]); + BookmarkModel* model = browser_helper_.profile()->GetBookmarkModel(); + model->Remove(default_parent_, 0); + ASSERT_FALSE([test_window() attachedSheet]); +} + TEST_F(BookmarkEditorControllerTest, EditAndConfirmOKButton) { // Confirm OK button enabled/disabled as appropriate: // First test the URL. |