155484 - Edit menu should apply to bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Simon Fraser [no longer active]]

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

Comment 1

[saari (gone)]

->pinkerton

Comment 2

[Mike Pinkerton (not reading bugmail)]

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

Comment 3

[Simon Fraser [no longer active]]

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

Comment 4

[Mike Pinkerton (not reading bugmail)]

no time

Comment 5

[Mike Pinkerton (not reading bugmail)]

we should also do this for the history tree.

Comment 6

[Nathan Wertman]

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.

Comment 7

[Simon Fraser [no longer active]]

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.

Comment 8

[Jasper]

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

Comment 9

[Chris Casciano]

please re-evaluate what is missing post bug 212630

Comment 10

[Mike Pinkerton (not reading bugmail)]

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

Comment 11

[Josh Aas]

over to Haas

Comment 12

[Bruce Davidson]

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.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 14

[Bruce Davidson]

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.

Comment 15

[Bruce Davidson]

Attachment: First rough patch

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.

Comment 16

[Bruce Davidson]

Attachment: Full patch

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

Comment 17

[Bruce Davidson]

Attachment: Revised patch

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 18

[Simon Fraser [no longer active]]

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.

Comment 19

[Bruce Davidson]

Attachment: smfr's review comments addressed

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 20

[Bruce Davidson]

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.

Comment 21

[Jasper]

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.

Comment 22

[Simon Fraser [no longer active]]

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

Comment 23

[Simon Fraser [no longer active]]

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.

Comment 24

[Bruce Davidson]

Attachment: 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.

Comment 25

[Wevah (Nate Weaver)]

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

Comment 26

[Wevah (Nate Weaver)]

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

Comment 27

[Bruce Davidson]

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.

Comment 28

[Wevah (Nate Weaver)]

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 ;) ).

Comment 29

[Bruce Davidson]

Attachment: Updated patch

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

Comment 30

[Bruce Davidson]

Attachment: 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*.

Comment 31

[Wevah (Nate Weaver)]

(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! ;)

Comment 32

[Bruce Davidson]

Attachment: Revised versions of new 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!

Comment 33

[Wevah (Nate Weaver)]

r+

Comment 34

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 182096 [details] [diff] [review]
Updated patch

sr=pink.

Comment 35

[Mike Pinkerton (not reading bugmail)]

landed regression fixes. marking fixed.

Comment 36

[Bruce Davidson]

Reopening for the [eventual] implementation of "Cut"

Comment 37

[Josh Aas]

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

Comment 38

[Samuel Sidler (old account; do not CC)]

Bruce, have you had time to look at this?

Comment 39

[Mike Pinkerton (not reading bugmail)]

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?