230340 - support dragging multiple items in history view

Status: RESOLVED FIXED

(Camino Graveyard :: History, defect)

Description

[S Woodside]

this is a follow on to bug 227618. Dragging multiple items in the history
outline view should be supported.

Comment 1

[Julian Pellico]

Hi,

THe main complication here is that you can't really provide multiple items of
the same type on a pasteboard, right? Hmm. I'll think about this for a bit. ;-)
If you have any ideas Mike, let me know.

Comment 2

[Julian Pellico]

I can see the usefulness of being able to drag multiple items from the history
to for example the bookmark view. What about things like the location bar and
tab views? It only makes sense to be able to drag one item. Should we have some
default behavior like use the first row in the selection, or implement a new
drag type that only some of the objects respond to?

Comment 3

[Julian Pellico]

as discussed with Pinkerton, the history will behave like the bookmark manager
with respect to where drops can occur.

Comment 4

[Julian Pellico]

Attachment: Use MozURLType with an array as plist

Comment 5

[Julian Pellico]

Attachment: same as above, but with slight beahvior tweak

So I talked to Geoff about what the behavior should be when you drag
bookmars/history items onto the tab bar. We decided if you drag into the bar on
an area unoccupied by tabs, it should ADD the items as new tabs, not overwrite
current tabs. It appears the BM overwrites the tabs, and I did the same in my
first patch. SO i just changed that.

The BM can also be changed easily by changing YES to NO on
BrowserTabView.mm:380.

Comment 6

[Julian Pellico]

Comment on attachment 160811 [details] [diff] [review]
same as above, but with slight beahvior tweak

randomly choosing SImon for review...

Comment 7

[Julian Pellico]

Comment on attachment 160811 [details] [diff] [review]
same as above, but with slight beahvior tweak

I hear S. Woodside isn't so active...

Comment 8

[Julian Pellico]

I got it

Comment 9

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 160811 [details] [diff] [review]
same as above, but with slight beahvior tweak

Two things I noticed:

1. I can't drag multiple items from history like I can from bookmarks with thsi
patch, which appears (unless I misunderstand) to be part of the purpose of this
patch.

2:
     [self registerForDraggedTypes: [NSArray arrayWithObjects:
-	       @"MozURLType", NSStringPboardType, NSURLPboardType,
NSFilenamesPboardType, nil]];
+	       NSStringPboardType, NSURLPboardType, NSFilenamesPboardType,
nil]];

Wh don't we want the browser view registered for MozURLType? ISTM we do...

Comment 10

[Julian Pellico]

Did I miss the most important file? Hmm, let me check.

I'm not familiar with "ISTM." To avoid removing MozURLType in that file, I'd
make a new drag type.

Comment 11

[Julian Pellico]

Attachment: patch that should actually work

Comment 12

[Geoff "temporarily off bugmail" Beier]

CC'ing pinkerton and josh so they'll see and perhaps voice an opinion about my
question.

Comment 13

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #11)
> Created an attachment (id=161626)
> patch that should actually work
> 
Two questions:

1. When multiple history items are dragged to the content area, it is not
replaced, but rather all new tabs are opened. Is this what we want to do? The
behavior is the same whether the command key is down or not.

2. Do we really want to *not* register CHBrowserView for MozURLType? This will
negatively impact anyone other than Camino who embeds CHBrowserView, as they
will be obligated to either
   - modify it
   - subclass it
   - have its superview handle MozURLType drags
This does not seem to be what we want to do.

Is there a particular reason why you're not creating your drag as
MozBookmarkType? If you did that (unless I misunderstand somehting) it looks
like your patch would be much smaller and would not risk unintended impact on
other cocoa embedders, if those actually exist :-).

Mike and Josh, please correct me if I've got the reason for CHBrowserView
handling MozURLType all wrong!!

Comment 14

[Julian Pellico]

As I think I said, the MozBookmarkType expects elements of the array to be
Bookmark objects. Changing all the places that handle MozBookmarkType would be
as much work.

I wasn't aware that CHBrowserView is intended to be droppable into other
applications. I can make a new MozMultiURLType if that's what everyone prefers.

Comment 15

[Julian Pellico]

Attachment: patch that should actually work and meet people's demands

Comment 16

[Julian Pellico]

Comment on attachment 162318 [details] [diff] [review]
patch that should actually work and meet people's demands

review again =)

Comment 17

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 162318 [details] [diff] [review]
patch that should actually work and meet people's demands

There are three issues that need to be addressed before I'm comfortable marking
this r+.

1. This patch doesn't apply cleanly against current CVS. This is because
BrowserTabView.mm has changed since this was generated. It needs to be respun
against current CVS.

2. Is the current behavior that I asked about above what's desired?

3. It now uses a new drag type, MozMultiURLType, which seems OK to me and
certainly better than having CHBrowserView *not* handle MozURLType. However, is
this really necessary? It would seem like a good idea to me if we were to
create bookmark objects when we do this drag and just use MozBookmarkType. The
main reason I'd favor that approach is that the other classes already handle
multiple dragged MozBookmarkTypes appropriately and you'd therefore need to
make fewer changes. What do other folks think about this.

Anyway, (1) needs to be addressed before this can be properly reviewed, and I'd
like to get some feedback from others on (2) and (3) before I'm comfortable
marking this r+.

Comment 18

[Julian Pellico]

Attachment: gah - again

does anyone have any suggestions for checking out a fresh camino folder to test
patches against? It looks like camino isn't a module in CVS, ie

cvs co -Pd camino

doesn't work.

Comment 19

[Julian Pellico]

Comment on attachment 162448 [details] [diff] [review]
gah - again

I'm confident that this applies against CVS now -- I have a system for testing
:)

Geoff, regarding the behavior you ask about, I changed it so it does the same
thing as bookmark drags.

Comment 20

[Mike Pinkerton (not reading bugmail)]

+      int i;
+      NSArray* mozURLArray = (NSArray*)plist;
+      for (i = 0; i < [mozURLArray count]; i++) {

move the declaration of |i| into the for decl, eg:

for (int i = 0; ....)

happens in several places.

Comment 21

[Julian Pellico]

Attachment: address Mike's comment

CVS is a moving target :( argh
Plus I fixed what Pink said in the last comment.

Comment 22

[Julian Pellico]

Comment on attachment 164033 [details] [diff] [review]
address Mike's comment

Can we get moving on this again? :)

Comment 23

[Julian Pellico]

Comment on attachment 164033 [details] [diff] [review]
address Mike's comment

Josh too ;)

Comment 24

[Josh Aas]

Why do you make 2 arrays and then pass them to a new function where they are
combined into a dictionary which you put on the pasteboard? Why not just make
the dictionary in the first place and put it on the pasteboard?

There does not seem to be any reason to make a new function in
NSPasteboard+Utils at all.

Comment 25

[Geoff "temporarily off bugmail" Beier]

Some observations:

1. If I drag multiple items onto one tab, all my other tabs are replaced
regardless of the status of my command key. Is this correct behavior?

2. With this patch applied, my history view groups are labelled
"0","1","2","3","4","5","6","6" instead of "Today","Yesterday",etc.

3. It still seems to me we should be dragging bookmarks here instead of creating
a new type. See Comment #17.

(2) is a big deal. (1) and (3) need some consensus to be reached I think, before
this patch can be integrated.

Comment 26

[Josh Aas]

The history day names showing up as digits happens to me all the time without
this patch. I don't know what the rhyme or reason is for when it comes and goes.
I doubt this patch has anything to do with it.

Comment 27

[Julian Pellico]

(In reply to comment #24)
> Why do you make 2 arrays and then pass them to a new function where they are
> combined into a dictionary which you put on the pasteboard? Why not just make
> the dictionary in the first place and put it on the pasteboard?
> 
> There does not seem to be any reason to make a new function in
> NSPasteboard+Utils at all.

That's a valid point.
Also, from Geoff,
> 1. If I drag multiple items onto one tab, all my other tabs are replaced
> regardless of the status of my command key. Is this correct behavior?

I guess I'll check what bookmarks do.

Comment 28

[Julian Pellico]

Attachment: patch 7

accomodating Josh
now I just need to take down Geoff :)
re comment #25
1) same behavior as dragging Bookmarks.
2) this doesn't happen to me, same as Josh
3) Mike or Josh, please either second Geoff or side with me ;)

Comment 29

[Josh Aas]

I think I'm going to side with making bookmark objects and dragging those. New
types are messy and we already have lots of functionality for the bookmark type.

Comment 30

[Josh Aas]

At end of patch...

- return NO;

Not a good idea to leave a code path that doesn't return a value (even if it
probably can't ever be called that way, since it's tricky to drag 0 things).

Comment 31

[Julian Pellico]

It's extremely nonobvious to set the HistoryDataSource to post Bookmarks onto
the pasteboard and have it work. Not only does the following code not work in
that the history items aren't dragged successfully, it also causes a runtime
error if you leave the History and go back to it:
2004-11-18 22:35:22.218 Camino[535] *** -[HistoryItem icon]: selector not recognized
2004-11-18 22:35:22.219 Camino[535] Exception raised during posting of
notification.  Ignored.  exception: *** -[HistoryItem icon]: selector not recognized


if( count > 1 ) {
    NSMutableArray* bookmarks = [[NSMutableArray alloc] initWithCapacity:count];
    for (int i = 0; i < count; i++) {
      HistoryItem* historyItem = [toDrag objectAtIndex:i];
      Bookmark* bookmarkItem = [[[Bookmark alloc] init] retain];
      [bookmarkItem setTitle:[[historyItem name]
stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet]
withString:@" "]];
      [bookmarkItem setKeyword:[NSString string]];
      [bookmarkItem setUrl:[historyItem url]];
      [bookmarkItem setDescription:[NSString string]];
      [bookmarkItem setLastVisit:[NSDate date]];
      [bookmarkItem setStatus:kBookmarkOKStatus];
      [bookmarkItem setIsSeparator:NO];
      [bookmarks addObject:bookmarkItem];
    }
    NSArray* ptrArray = [NSArray
dataArrayFromPointerArrayForMozBookmarkDrop:bookmarks];
    [pboard declareTypes:[NSArray arrayWithObject:@"MozBookmarkType"] owner:self];
    [pboard setPropertyList:ptrArray forType:@"MozBookmarkType"];
    [bookmarks release];
    return YES;
  }

Not to mention the memory management headache, because the Bookmarks are
expected to have a lifetime beyond the drag operation.

If someone else wants to pursue the bug along this path, I suggest they take it
from me.

Comment 32

[Simon Fraser [no longer active]]

I think we should have a common protocol for history items and bookmarks, and
use that to unify the drag and drop code.

Note that I changed the multiple bookmarks dragging code recently.

Taking.

Comment 33

[Bruce Davidson]

The whole MozURLType seems a bit weird, its actually only used by Camino and
limiting it to a single URL seems a bit artificial. In bug 287118 I'm
considering implementing the WebURLsWithTitlesPboardType that Safari uses. This
is similar to MozURLType but contains a list of URLs and list of titles.

If we implemented this type we can then copy both bookmarks and history in the
same way, without having to touch MozBookmarkType. We'd also get drag and drop
to Safari, which would be pretty cool.

Comment 34

[Bruce Davidson]

Taking this bug of smfr's plate. Once bug 287118 is in I'll post a patch against
that. The patch should be simpler than patch 7 because lots of the changes for
pasting multiple bookmarks have already gone in.

Comment 35

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

Ping.

Bruce: Status update? I saw you said you were going to post a new patch after
287118 was in (which it is). Any more work done?

Comment 36

[Josh Aas]

This should not be an 0.9 blocker, though it would be nice to have. Reassigning to Simon for tracking as 
Bruce doesn't appear to be on it.

Comment 37

[Simon Fraser [no longer active]]

Fixed (but not using the patches here).