Closed Bug 954941 Opened 10 years ago Closed 10 years ago

Drop target does not reappear after detaching all but one merged buddies

Categories

(Instantbird Graveyard :: Contacts window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(3 files, 2 obsolete files)

*** Original post on bio 1509 at 2012-06-12 16:04:00 UTC ***

Timestamp: 06/12/2012 05:58:26 PM
Error: this.childNodes[1] is undefined
Source File: chrome://instantbird/content/contact.xml
Line: 170

STR
- Merge a buddy by drag and drop onto the drop target of another buddy
- Note drop target disappears
- Detach the added buddy
- Drop target does not reappear
- Drag the buddy back onto the contact to merge it: error
Attached patch PatchSplinter Review
*** Original post on bio 1509 as attmnt 1609 at 2012-06-14 22:35:00 UTC ***

Checks the drop target is there before trying to remove it.
Attachment #8353366 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** Original post on bio 1509 as attmnt 1610 at 2012-06-14 22:37:00 UTC ***

Alternatively, put back the drop target immediately when detaching the last merged buddy.

This seems better in some ways, but it "feels odd" somehow when actually using it. What do you think?
Attachment #8353367 - Flags: review?(clokep)
Attached patch Alternative patch with delay (obsolete) — Splinter Review
*** Original post on bio 1509 as attmnt 1611 at 2012-06-14 22:53:00 UTC ***

Same as the alternative patch, but with a setTimeout so the detach animation finishes before the drop target is added back. Gets rid of the odd feeling of the alternative patch imho.
Attachment #8353368 - Flags: review?(clokep)
Attached patch Alternative patch with delay (obsolete) — Splinter Review
*** Original post on bio 1509 as attmnt 1612 at 2012-06-14 23:10:00 UTC ***

Forgot to trigger the insert animation for the drop target.
Attachment #8353369 - Flags: review?(clokep)
Comment on attachment 8353368 [details] [diff] [review]
Alternative patch with delay

*** Original change on bio 1509 attmnt 1611 at 2012-06-14 23:10:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353368 - Attachment is obsolete: true
Attachment #8353368 - Flags: review?(clokep)
*** Original post on bio 1509 at 2012-06-14 23:27:35 UTC ***

There is still a bug here. If, after detaching the buddy, I drop another buddy onto the remaining buddy inside the contact, I get the following error

Timestamp: 06/15/2012 01:00:47 AM
Error: observer.observe is not a function
Source File: components/imContacts.js
Line: 1091

So it seems I am missing something...
*** Original post on bio 1509 at 2012-06-14 23:34:34 UTC ***

(In reply to comment #5)
> There is still a bug here. 
To be more specific, this only applies to the alternative patches.
*** Original post on bio 1509 at 2012-06-15 00:14:31 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> > There is still a bug here. 
> To be more specific, this only applies to the alternative patches.

The problem is that buddyElt.build(aSubject, this); adds an observer. And the code is not set up to have two buddies to transition to.

I can't see how to add the insert transition animation "by hand" without causing an error somewhere down the line. 

Leaving the problematic line out, I can still produce an error
  Timestamp: 06/15/2012 02:08:47 AM
  Error: this._observers is undefined
  Source File: file://components/imContacts.js
  Line: 808
unless I remove the setTimeout call. The error occurs when dropping a buddy onto the remaining buddy after detaching the merged buddy.

So it's only the "alternative Patch", without buddyElt.build(aSubject, this), that appears to be as bug-free as the original Patch.
Comment on attachment 8353369 [details] [diff] [review]
Alternative patch with delay

*** Original change on bio 1509 attmnt 1612 at 2012-06-15 00:16:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353369 - Flags: review?(clokep) → feedback?(clokep)
*** Original post on bio 1509 as attmnt 1613 at 2012-06-15 00:33:00 UTC ***

Found a way to add the delay without (as far as I can tell) adding any bugs. 

To repeat, it would be nice if one could use the transition timer mechanism for this, but I don't see how to make this work for a drop target buddy and for transitioning to two buddies (detached buddy and drop target) rather than just one (the detached buddy).
Attachment #8353370 - Flags: review?(clokep)
Comment on attachment 8353369 [details] [diff] [review]
Alternative patch with delay

*** Original change on bio 1509 attmnt 1612 at 2012-06-15 00:33:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353369 - Attachment is obsolete: true
Attachment #8353369 - Flags: feedback?(clokep)
Comment on attachment 8353367 [details] [diff] [review]
Alternative patch

*** Original change on bio 1509 attmnt 1610 at 2012-06-15 21:39:01 UTC ***

I definitely like this one the best FWIW.
Attachment #8353367 - Flags: review?(clokep) → review+
Comment on attachment 8353366 [details] [diff] [review]
Patch

*** Original change on bio 1509 attmnt 1609 at 2012-06-15 21:39:20 UTC ***

This is pretty clearly broken if the drop target doesn't come back.
Attachment #8353366 - Flags: review?(clokep) → review-
Comment on attachment 8353370 [details] [diff] [review]
Alternative patch with delay

*** Original change on bio 1509 attmnt 1613 at 2012-06-15 21:39:44 UTC ***

I think the delay makes this one look broken...
Attachment #8353370 - Flags: review?(clokep) → review-
Whiteboard: [checkin-needed]
*** Original post on bio 1509 at 2012-06-21 00:29:18 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/8d161d706eb2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.