Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 231842

Summary: DelayedDirectEditHelper does unregister control listeners when aborted
Product: [Tools] GEF Reporter: Liam Asher Segel-Brown <liamsb>
Component: GEF-Legacy GEF (MVC)Assignee: Anthony Hunter <ahunter.eclipse>
Status: RESOLVED FIXED QA Contact:
Severity: trivial    
Priority: P3 CC: ahunter.eclipse, hudsonr, mlehmannm, nyssen
Version: 3.4   
Target Milestone: 3.5.0 (Galileo) M7   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Patch that fixes the described problem for me, please review. nyssen: iplog+

Description Liam Asher Segel-Brown CLA 2008-05-13 11:48:53 EDT 
Build ID: I20080502-0100

Steps To Reproduce:
1. Create an EditPart using a DIRECT_EDIT_ROLE policy.
2. Active the direct edit, by clicking on the EditPart.
3. Cause the DelayedDirectEditHelper to be aborted before it runs. One way to do this is to put a breakpoint on line 61 in the constructor, and then change focus to another window when execution stops at this breakpoint. This causes a focus lost event which aborts the DelayedDirectEditHelper prior to the DelayedDirectEditHelper being run.
4. Listeners are not unregistered from the control (viewer.getControl()). This can be observed by placing a breakpoint in the abort method and repeating step 3. The abort method will be called an extra time for each set of listeners that have not been removed.

More information:
Solution:

Move the remove listener calls on lines 105-107 outside the if statement beginning on line 101. This will cause the listeners to be removed from the control of the used DelayedDirectEditHelper regardless of if the helper was aborted or not.
Comment 1 Marco Lehmann-Mörz CLA 2008-05-20 05:18:05 EDT 
This will probably fix a problem that sometimes the listener won't unregister, which results in memory leaks, because the DelayedDirectEditHelper holds a reference to the source edit part.
Comment 2 Marco Lehmann-Mörz CLA 2008-05-20 13:26:55 EDT 
Created attachment 101105 [details]
Patch that fixes the described problem for me, please review.
Comment 3 Randy Hudson CLA 2008-05-20 22:52:12 EDT 
> Created an attachment (id=101105) [details]
> Patch that fixes the described problem for me, please review.

The patch looks safer than the original suggestion of just moving the code around ;-).  Calling remove listener would sometimes throw a widget is disposed exception.

While we're in this class, I noticed that SWT has added getDoubleClickTime() to Display. We originally had to guess at this, which is why we have a magic number (400) in the code.
Comment 4 Anthony Hunter CLA 2009-04-02 13:51:01 EDT 
(In reply to comment #3)
> While we're in this class, I noticed that SWT has added getDoubleClickTime() to
> Display. We originally had to guess at this, which is why we have a magic
> number (400) in the code.

Bug 271021
Comment 5 Anthony Hunter CLA 2009-04-03 14:53:59 EDT 
Committed to HEAD for 3.5.