This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 197502 - Code removing graph listener adds it instead
Summary: Code removing graph listener adds it instead
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Graph (show other bugs)
Version: 6.x
Hardware: PC Other
: P3 normal (vote)
Assignee: elotter
URL:
Keywords: SIMPLEFIX
Depends on:
Blocks:
 
Reported: 2011-04-06 17:48 UTC by jirka_x1
Modified: 2011-10-14 22:26 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
changing addition of change listener to removal of change listener (556 bytes, patch)
2011-10-09 16:53 UTC, elotter
Details | Diff
New version of the patch, intended to be more verbose with a reference to the bug fixed (708 bytes, patch)
2011-10-09 21:46 UTC, elotter
Details | Diff
Yet another version of the patch, now also containing a unit test that tests for the bug fixed by this patch. (8.92 KB, patch)
2011-10-10 14:25 UTC, elotter
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jirka_x1 2011-04-06 17:48:56 UTC
org.netbeans.api.visual.graph.layout.GraphLayout.removeGraphLayoutListener(GraphLayoutListener) should remove a listener, but it adds it instead (as apparent from http://hg.netbeans.org/main/file/aad87726c83d/api.visual/src/org/netbeans/api/visual/graph/layout/GraphLayout.java)

IMHO the fix is simple: replace

listeners.add (listener);

with

listeners.remove (listener);
Comment 1 tomwheeler 2011-08-10 02:50:44 UTC
Adding keyword SIMPLEFIX because it's straightforward.  I will suggest it as an issue to fix in the "First Patch" program.
Comment 2 elotter 2011-10-09 16:01:02 UTC
As with Jirka's previous comment, my implementation plan is to make the obvious change from 

   public final void removeGraphLayoutListener (GraphLayoutListener<N,E> listener) {
        synchronized (listeners) {
            listeners.add (listener);
        }
    }

to

public final void removeGraphLayoutListener (GraphLayoutListener<N,E> listener) {
        synchronized (listeners) {
            listeners.remove (listener);
        }
    }

whilst also doing usage searches inside the platform to see whether some other modules depended on this, with potentially erroneous results.
Comment 3 elotter 2011-10-09 16:47:08 UTC
No usages of removeGraphLayoutListener(GraphLayoutListener<N,E> listener) inside platform sources, this should therefore not influence platform or IDE itself.
Comment 4 elotter 2011-10-09 16:53:04 UTC
Created attachment 111733 [details]
changing addition of change listener to removal of change listener
Comment 5 elotter 2011-10-09 16:55:53 UTC
The patch solely influences api.visual (Visual Library API).
Comment 6 elotter 2011-10-09 21:46:18 UTC
Created attachment 111735 [details]
New version of the patch, intended to be more verbose with a reference to the bug fixed

New version of the patch that changes addition of the change listener to removal of the change listener, adding more detailed comments about the delta.
This obsoletes the earlier version of this patch that did not contain any comments.
Comment 7 elotter 2011-10-10 14:25:32 UTC
Created attachment 111787 [details]
Yet another version of the patch, now also containing a unit test that tests for the bug fixed by this patch.

New version of patch, also adding a unit test that should fail if the fix to GraphLayout is not applied.
Comment 8 spoonerj30 2011-10-10 19:19:25 UTC
I've successfully integrated Ernest's patch into my local repository.  I first patched just the test file and ran the tests which failed expectantly. I then patched the class and ran the tests which then passed.

I'm satisfied that his patch will fix the issue and his included test does test the fix. 

Pursuant to sub-section 4 of the "Testing the fix" section in the First Patch Program directives[1] I am now verifying that this patch is OK and ready for inclusion.

[1] http://wiki.netbeans.org/First_Patch
Comment 9 tomwheeler 2011-10-12 01:50:48 UTC
I've just applied the patch and pushed the fix and corresponding test to the NetBeans core-main repository:

   http://hg.netbeans.org/core-main/rev/3d38437c07ee

Ernest, this should be part of a nightly build by Friday (and maybe even Thursday).  After that, please download a nightly build (such as the platform source ZIP file) from the following URL:

   http://bits.netbeans.org/download/trunk/nightly/latest/zip/

Then verify that your patch is included and mark this issue VERIFIED.  I'll then add you to the the credits page, as I did for the other First Patch graduates:

http://hg.netbeans.org/core-main/file/tip/ide.branding/release-toplevel/CREDITS.html

Nice work!  Also, don't forget to add a comment to Geertjan's blog and tell him that this issue is now fixed :-)
Comment 10 elotter 2011-10-12 12:44:38 UTC
Thanks Tom, I will download nightlies as they become available in the next few days - and hope to confirm the correct elimination of this bug.
Comment 11 Quality Engineering 2011-10-14 14:59:50 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/3d38437c07ee
User: Thomas W. Wheeler <tomwheeler@netbeans.org>
Log: Fix for #197502, developed by Ernest Lotter in the First Patch program
Comment 12 elotter 2011-10-14 22:26:36 UTC
Tom: it is confirmed, the nightly build of 201110140600 has this patch integrated.