143094 - [chimera] Better error checking on drag&drop

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Mike Pinkerton (not reading bugmail)]

patch from [e-mail], coming up

Comment 1

[Mike Pinkerton (not reading bugmail)]

Attachment: some better error handling

haven't looked at the patch at all.

Comment 2

[Joe Hewitt (gone)]

Big thanks to [e-mail] for cleaning up my slop. Some quick comments
after scanning over the patch:

* Looks like you have tabs turned on in Project Builder (the default
setting).  Could you please turn off tabs and replace all the tabs in
your patch with 2-space indentation?

* In response to your comment about mouseUp and dragging - dragging must start
while the mouse is down - try dragging anything in the OS and you'll see.

* That O(n^2) operation in writeItems could be mightly slow if you are dragging
a lot of bookmarks.  I am curious if it perceptibly affects the start of a drag
if you have a large bookmarks file and try to drag a bunch of it around.

* Some of your method names start with lowercase, some uppercase.  Could you try
to fit with the convention of the class? I guess it's confusing since Apple uses
lowercase but some of Hyatt's classes use uppercase (like BookmarksService).

Comment 3

[Richard Schreyer]

I've fixed all the issues except for the o(n^2) thing.

I'm not sure if I'm going to fix it, or just cut out that functionality.

What that operation does, is it removes a node from the drag if that node's 
parent is also being dragged.   So, for example, if you expanded an item 
in the list, and then dragged that item, and several of it's children, this 
ensures that when you drop, the children will still be inside of that folder.

If I removed that line, the children would be placed out on the same level 
as the folder.

The current slow implementation is a little more forgiving when it comes 
to preserving any existing orginization, while the latter is arguably a bit 
more powerful for the user.  Any opinions on how it should work?

Comment 4

[Mike Pinkerton (not reading bugmail)]

how does it work in other apps, ie, what is the expected behavior from the user
perspective?

i would argue that the current behavior is more common and most expected.
Usually whenever i've written dnd code in the past, i remove children from the
list if the parent is dragged too.

Comment 5

[Richard Schreyer]

Attachment: updates to the bookmark dnd patch

fixed the issues brought up in the bug comments:
indenting and tabs fixed
method capitalization fixed
commented out o(n^2) code.  this should get fixed in the future, but it's not
too important right now
added new argument to BookmarksService::BookmarkAdded/Changed/Remote to
determine if it should flush the bookmarks.  There were LOTS of extraneous
flushes when drag-and-dropping a large number of bookmarks.  Drops should be
much faster now.

Comment 6

[Mike Pinkerton (not reading bugmail)]

i noticed there are parts of this patch that are now outdated and have been
removed from the relevant files. can i get an updated patch?

also, how does this patch relate to bug 149920? does it address it at all?

Comment 7

[Richard Schreyer]

Attachment: updated bookmarks patch

The patch has indeed bitrotted.  An updated version is attached.

Regarding bug 149920, that brings up two issues that can cause bookmarks to be
lost, and then two crashes that can result afterwards.	Both of those issues
are fixed in this patch.

Comment 8

[Kathleen :Brade]

looking at the patch, I see a couple blocks that are commented out with /* */
can these be removed or converted to #ifdef WHEN_WE_HAVE_A_PERFORMANCE_FIX or
something like that?

Comment 9

[Kathleen :Brade]

Attachment: cleaned up patch (QI, #ifdef, etc)

Comment 10

[Kathleen :Brade]

Attachment: above with with -w

Comment 11

[Kathleen :Brade]

*** Bug 149920 has been marked as a duplicate of this bug. ***

Comment 12

[Mike Pinkerton (not reading bugmail)]

richard, i noticed this patch disables dragging items on the personal toolbar to
re-arrange them. was this intended, or an oversight?

i'm about to land this and would like to know.

Comment 13

[Richard Schreyer]

As I recall, when I wrote this code, dragging from the toolbar hadn't even been implemented yet, so this probably needs to get fixed before it's added to the tree.

Comment 14

[Mike Pinkerton (not reading bugmail)]

over to brade. 

richard: in the patch you removed the method that the personal toolbar relied on
to start a drag. why? 

upping to blocker status, it fixes several dataloss bugs

Comment 15

[Richard Schreyer]

This patch was initially written almost a month and a half ago, before Chimera had any 
bookmark d&d at all.  That effort was duplicated (someone else beat me to it), so I 
merged what I liked from the existing implementation (which had several major 
oversights in it, the dataloss bugs) into my own, and submitted it.  All of the functionality in 
the removed methods (at that time) was covered elsewhere.

Unfortunatly, I haven't tracked how the codebase has changed since then, and what 
might rely on those methods.

My general reasoning was that the BookmarksService class was serving as a data 
model, so I moved anything having to do with drag&drop specifically out of it, and into the 
BookmarksDataSource and CHBookmarksToolbar classes.  Only functions having 
specifically to do with editing the bookmarks database were left in the BookmarksService 
class.  

I'll have a look at how dragging from the toolbar works today and see what can be done.

Comment 16

[Joe Hewitt (gone)]

I didn't hook up dragging from the toolbar because of quirky event issues with
the cocoa buttons on the toolbar.  The drag events are never fired on these
buttons, because they are killed off by the buttons' own handling of the mouse
down event.  The only solution I can think of is to stop using cocoa buttons and
make up our own button class.

Comment 17

[Mike Pinkerton (not reading bugmail)]

ohhhhhh, i see. so the bugs that had to do with dragging things onto themselves
from the personal toolbar are drags from the bookmarks pane. ah-ha!

i'll try to land this one today, then.

Comment 18

[Mike Pinkerton (not reading bugmail)]

landed.

Comment 19

[Terri Preston]

Verfied Project X trunk build 6/28/02