I should be able to use Cut/Copy/Paste/Delete for bookmarks in the sidebar.

->pinkerton

we should also add a "get info" here as well

I just checked in a 'Get Info' command on the Edit menu. We still need Cut/Copy/Paste/Undo etc.

no time

we should also do this for the history tree.

PING!!!! If anyone is still working/worried/interested in this bug let me know. I am new to Camino development and I would like to tackle this one....The Copy/Paste/Delete I mean.

For for it. Basically, I'd like to see bookmark operations use the NSUndo stuff, so that they are undoable. Once that works, copy/paste would be cool.

This bug should be fixed by the patch atached to Bug 212630.

please re-evaluate what is missing post bug 212630

well, cut/copy/paste still don't work, but probably won't any time soon.

over to Haas

For responding the the cut/copy/paste menu items this should be a matter of rigging up the appropriate methods in the bookmark tree and history views (which are the first responder at this point), and then doing the appropriate stuff. Using NSPasteBoard it should be relatively easy to register our data type and post stuff to the pasteboard using that as our first option and plain text (probably just containing the URL and the bookmark title) as a second option (so that pasting to other apps works). At the risk of looking silly I'm going to take this bug and try to get basic support along these lines implemented. Simon, I don't understand you comment on the NSUndo stuff. Perhaps when we're both in IRC you could explain this to be a bit more, and we can work out whether its part of this bug or something else entirely.

See also bug 280028, where Edit: Undo/Redo are wonky with bookmarks.

Quick status update: I've got delete and copy working. When copying the bookmarks are copied to the pasteboard in our internal format and also as text. The text will probably just be a newline separated list of the URLs when multiple bookmarks are copied, unless people can suggest better alternatives. Will hopefully get paste to work next time I get to spend some quality time with the code, and will post patches here once that's done.

This rough patch adds support for the Delete, Copy and Paste menu items in the bookmarks view. They all work pretty much as expected. I have a few final clean-up tasks to do: - Add support for pasting URLs and plain texts (based on our drag and drop code) - Tidy up enabling of the "paste" menu item to follow changes to acceptable formats - Minor code tidy up etc. Once I've done those bits I'll post a patch here for review.

Full patch addresses remaining issues from the rough patch: - paste menu item is enabled when any format we understand for pasting exists on the general pasteboard. - NSURL and NSString types can be pasted - Minor code tidy ups, removal of logging code. (Note that the bracing standard is different across the two source files I've patched - I've maintained local standards in each one).

Revised patch following discussions with Simon: - Make the controller rather than the view do most of the work - Add skeleton clipboard function support to ExtendedObjectView - Minor additional tidy ups I'm seeking review of this patch as is. However I intend to make two further changes once I have approval in principle for this patch: - s/@"MozURLType"/kCaminoURLAndTitlePboardType/g - s/@"MozBookmarkType"/kCaminoBookmarkListPboardType/g (throughout the rest of the source tree, trivial change but it makes the patch longer and harder to review) - Rename the deleteBookmarks: selector in BookmarkViewController to delete: This will avoid the need for the forwarding function in BookmarkOutlineView and make the code marginally tidier. However it will require a change to the nib (for the context menu) so I haven't done this change until I have approval in principle, to reduce the time I have to keep locally modified nibs in sync)

Comment on attachment 179099 [details] [diff] [review] Revised patch > +// Actions for the edit menu > +-(BOOL) validateMenuItem:(id)aMenuItem; > +-(IBAction) delete:(id)aSender; > + Please use follow our more common spacing rule of: - (BOOL)validateMenuItem:(id)aMenuItem; here are for the other new methods added. > Index: camino/src/bookmarks/BookmarkOutlineView.mm > =================================================================== > > +// > +// Override implementation in ExtendedOutlineView so we can check whether an > +// item is selected or whether appropriate data is available on the clipboard. > +// > +-(BOOL) validateMenuItem:(id)aMenuItem > +{ > + SEL action = [aMenuItem action]; > + if (action == @selector(delete:)) > + return [[self delegate] numberOfSelectedRows] > 0; > + else if (action == @selector(copy:)) > + return [super validateMenuItem:aMenuItem] && [[self delegate] numberOfSelectedRows] > 0; Shouldn't that be [self numberOfSelectedRows]? > + else if (action == @selector(paste:)) > + return [super validateMenuItem:aMenuItem] && [[self delegate] canPasteFromPasteboard:[NSPasteboard generalPasteboard]]; > + else if (action == @selector(cut:)) > + return NO; > + > + return YES; > +} Avoid "return else". > Index: camino/src/bookmarks/BookmarkViewController.mm > =================================================================== > > +-(void)pasteBookmarks:(NSPasteboard*)aPasteboard intoFolder:(BookmarkFolder *)dropFolder index:(int)index copying:(BOOL)isCopy; > +-(void)pasteMozBookmark:(NSPasteboard*)aPasteboard intoFolder:(BookmarkFolder*)dropFolder index:(int)index; > +-(void)pasteBookmarkFromURL:(NSPasteboard*)aPasteboard intoFolder:(BookmarkFolder*)dropFolder index:(int)index; > +-(BOOL)pasteBookmarkFromText:(NSPasteboard*)aPasteboard intoFolder:(BookmarkFolder*)dropFolder index:(int)index; I wonder if these methods should really be on the BookmarksManager (although we'd have to figure out how to get the new items selected). > +-(void)pasteMozBookmark:(NSPasteboard*)pasteboard intoFolder:(BookmarkFolder*)dropFolder index:(int)index > +{ > + NSDictionary* proxy = [pasteboard propertyListForType: kCaminoURLAndTitlePBoardType]; Could the kCaminoURLAndTitlePBoardType data ever be an array of items? In general, I wonder if we can make the code more flexible in terms of dealing with both single items, and arrays of items. > +- (IBAction)copy:(id)aSender > +{ > + // Get the list of bookmark items that are selected > + NSMutableArray *bookmarkItemsToCopy = [[NSMutableArray alloc] init]; Better to make bookmarkItemsToCopy autoreleased from the start (to protect against leakage in the case of early returns: NSMutableArray *bookmarkItemsToCopy = [NSMutableArray array]; > + NSEnumerator* selRows = [mBookmarksOutlineView selectedRowEnumerator]; > + id curSelectedRow = [selRows nextObject]; > + while (curSelectedRow) { > + [bookmarkItemsToCopy addObject: [mBookmarksOutlineView itemAtRow:[curSelectedRow intValue]]]; > + curSelectedRow = [selRows nextObject]; > + } This is usually written like: id curSelectedRow; while ((curSelectedRow = [selRows nextObject])) { [bookmarkItemsToCopy addObject: [mBookmarksOutlineView itemAtRow:[curSelectedRow intValue]]]; } > + // Copy these items to the general pasteboard as an internal list so we can > + // paste back to ourselves with no information loss > + NSArray *bookmarkUUIDArray = [BookmarkManager serializableArrayWithBookmarkItems:bookmarkItemsToCopy]; > + [[NSPasteboard generalPasteboard] declareTypes:[NSArray arrayWithObject:kCaminoBookmarkListPBoardType] owner:self]; > + [[NSPasteboard generalPasteboard] setPropertyList:bookmarkUUIDArray forType:kCaminoBookmarkListPBoardType]; > + > + // Now add copies in formats useful to other applications. For a single bookmark > + // use the full set of formats, otherwise just use a \n separated list of URLs > + if ([bookmarkItemsToCopy count] == 1) { > + BookmarkItem* bookmarkItem = [bookmarkItemsToCopy objectAtIndex:0]; > + if ([bookmarkItem isKindOfClass:[Bookmark class]]) { > + Bookmark* bookmark = (Bookmark*) bookmarkItem; > + [[NSPasteboard generalPasteboard] setDataForURL:[bookmark url] title:[bookmark title]]; > + } > + } else { > + NSMutableString* urlList = [[[NSMutableString alloc] init] autorelease]; Use: NSMutableString* urlList = [NSMutableString string]; > + for ( unsigned int i = 0; i < [bookmarkItemsToCopy count]; ++i ) { > + BookmarkItem* bookmarkItem = [bookmarkItemsToCopy objectAtIndex:i]; > + if ([bookmarkItem isKindOfClass:[Bookmark class]]) { > + Bookmark* bookmark = (Bookmark*) bookmarkItem; > + [urlList appendString:[bookmark url]]; > + [urlList appendString:@"\n"]; > + } > + } > + [[NSPasteboard generalPasteboard] setString:urlList forType:NSStringPboardType]; > + } > + > + [bookmarkItemsToCopy release]; > +} > +-(IBAction) paste:(id)aSender > +{ > + NSArray* types = [[NSPasteboard generalPasteboard] types]; > + > + int pasteDestinationIndex = 0; > + BookmarkFolder* pasteDestinationFolder = nil; > + > + // Work out what the selected item is and therefore where to paste the bookmark(s) > + NSEnumerator* selRows = [mBookmarksOutlineView selectedRowEnumerator]; > + id curSelectedRow = [selRows nextObject]; > + > + while (curSelectedRow) { > + BookmarkItem* item = [mBookmarksOutlineView itemAtRow:[curSelectedRow intValue]]; > + if ([item isKindOfClass:[BookmarkFolder class]]) { > + pasteDestinationFolder = (BookmarkFolder*) item; > + pasteDestinationIndex = [pasteDestinationFolder count]; > + } else if ([item isKindOfClass:[Bookmark class]]) { > + pasteDestinationFolder = (BookmarkFolder*) [item parent]; > + pasteDestinationIndex = [pasteDestinationFolder indexOfObject:item] + 1; > + } > + break; > + } Why the 'while' if you always break after the first? > Index: camino/src/extensions/ExtendedOutlineView.mm > =================================================================== > +-(IBAction) copy:(id)aSender > +{ > + [[self delegate] copy:aSender]; > +} > + > +-(IBAction) paste:(id)aSender > +{ > + [[self delegate] paste:aSender]; > +} > + > +-(IBAction) delete:(id)aSender > +{ > + [[self delegate] delete:aSender]; > +} > + > +-(IBAction) cut:(id)aSender > +{ > + [[self delegate] cut:aSender]; > +} I think each of these needs to do a [[self delegate] respondsToSelector:] check.

Revised patch that addresses all Simon's review comments except moving code to BookmarkManager. Discussed on IRC, not thought to be worth doing. MozURLType can only contain a single URL and title. A plan for simplifying this code is contained in bug 287118. To keep this patch a reasonable size (for the convinience of reviewers and whoever checks it in for me) I haven't done a sweep for @"MozURLType" and @"MozBookmarkType" in files that haven't been touched by this bug. This work can be done gradually as the code is touched.

Comment on attachment 179214 [details] [diff] [review] smfr's review comments addressed Clearing the review flags on this. It looks like the patch on bug 287118 (which includes this change, but changes the pasteboard formats and tidies up the code) will supercede this. Jasper and I are currently testing builds with the patch for that bug, and will request review of that patch once happy with the results of that testing.

Hey Bruce with your path I'm no longer able to drag and drop a bookmark bar folder on the webview! Now, I have no problem with that but I'm pretty sure that's aregression.

I checked in the patch from bug 287118 with three changes: Change CHBrowserView to no longer accept bookmark types (kCaminoBookmarkListPBoardType, kWebURLsWithTitlesPboardType), because gecko can't handle them. Instead, a parent view will now handle the drop (and correctly load tabs etc). NSPastboard+Utils -containsURLData changed to test for a scheme on the NSURL. Otherwise, any random text would be accepted as url data. BookmarkViewController: -pasteBookmarks:intoFolder:index:copying fixed to keep an array of newly created bookmark items when copying, so we can correctly reveal those new items. Let's keep this bug open for the implementation of "Cut".

There are some regressions from these changes: * Can't drag one or more .webloc files from the Finder to the tab bar to load them * Cant' drag .webloc files to the bookmarks toolbar * test dragging .webloc files to the bookmarks outliner These bugs appear to be because drags from the Finder contain URL data (the url to the file being dragged), but we need to check for NSFilenamesPboardType first.

This patch fixes the regressions noted in the previous comment. It introduces a dependency from NSPasteboard+Utils to MainController, which doesn't seem like a particularly smart idea. I'm not asking for reviews, but any thoughts on what to do about said dependency are welcome.

(In reply to comment #24) > Created an attachment (id=180846) [edit] > Patch to address regressions > > This patch fixes the regressions noted in the previous comment. > > It introduces a dependency from NSPasteboard+Utils to MainController, which > doesn't seem like a particularly smart idea. I'm not asking for reviews, but > any thoughts on what to do about said dependency are welcome. > I don't think this is the right way to do it. What I believe *should* happen is that we should be checking for NSFilenamesPboardType before hasUrlData (or whatever) in the views themselves. It's just if block swaps in a few places.

FWIW, we did some if-block swapping in BrowserTabView.mm earlier because of a similar issue.

There was only one place (left) in the code where we checked for NSFilenamePboardType - in BrowserTabView.mm, which as you point out would just need the order of the if's swapping. However, the point of doing it in the category is to ensure that we have a consistent approach to dealing with URL data from pasteboards. This reduces the clutter in other bits of code and reduces the scope for small bugs/feature requests along the lines of "I can drop .weblocs onto the tab view, but not the bookmark manager" etc. Personally, apart from the odd dependency which I'm going to investigate solutions to, I think this patch probably is the way forward.

You're right, from a simplicity standpoint, at least. If it works and doesn't break anything, I guess I have no problem with it. (It looks a lot better to me today than it did 3 days ago ;) ).

Revised patch (new files to follow separately as I can't do cvs add)

These files are just moving code out of MainController so that stuff in extensions (which should be generally useful) doesn't depend on Camino specific code. The only change is to make them return NSURL* rather than NSString*.

(In reply to comment #30) > Created an attachment (id=182097) [edit] > New files for above patch > > These files are just moving code out of MainController so that stuff in > extensions (which should be generally useful) doesn't depend on Camino specific > code. The only change is to make them return NSURL* rather than NSString*. Note for you: Remeber to add me to the "contributors" section of those files! ;)

Revised version of these new files that just copies across the contributors from MainController as (apart from trivial comment text changes) I haven't contributed anything to these!

r+

Comment on attachment 182096 [details] [diff] [review] Updated patch sr=pink.

landed regression fixes. marking fixed.

Reopening for the [eventual] implementation of "Cut"

I'd like to see this for 0.9, but it is not a blocker.

Bruce, have you had time to look at this?

let's not zombie bugs. if cut isn't implemented, it should be a separate bug. This one is done, code was committed. We need to move on. Am I missing something?