Bug 316654

Summary: ITypeHierarchyChangedListener receive spurious callbacks
Product: [Eclipse Project] JDT Reporter: Jason Parekh <jasonparekh>
Component: CoreAssignee: Jay Arthanareeswaran <jarthana>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: Olivier_Thomann, satyam.kandula, srikanth_sankaran, tparker
Version: 3.7Flags: srikanth_sankaran: review+
Target Milestone: 3.7 M1   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Proposed patch
none
Updated patch
none
Patch with updated tests
none
Updated pat none

Description Jason Parekh CLA 2010-06-11 16:27:52 EDT
Build Identifier: 20100218-1602 

I've noticed my impl of this interface receives typeHierarchyChanged callbacks even though the hierarchy has not changed.

TypeHierarchy.elementChanged receives an event whose toString() is:
org.eclipse.jdt.core.ElementChangedEvent[source=Java Model[*]: {CHILDREN}
	myProject[*]: {CHILDREN | CONTENT}
		src_0/java[*]: {CHILDREN | CONTENT}
			com.myproject.some.package[*]: {CONTENT}
				ResourceDelta(/myProject/src_0/java/com/myproject/some/package/simplefile.txt)[*]
			ResourceDelta(/myProject/src_0/java/com)[*]
		ResourceDelta(/myProject/src_0)[*]]

where:
- myProject is my project name
- src_0/java is a source folder
- /myProject/src_0/java/com/myproject/some/package/simplefile.txt is a text file (non-Java resource in a package fragment)

It seems like TypeHierarchy.isAffectedByPackageFragmentRoot() is returning true incorrectly.  In the comments, it mentions for JAR content changes, it will return true if one of the packages match.  However, the corresponding logic never checks for a JAR package fragment root, and just returns true if the package matches one from the packageRegion.  Seems like a fix is to ensure the package fragment root is a JAR package fragment root.

Reproducible: Always

Steps to Reproduce:
1. Create a simple plugin that listens for changes on a TypeHierarchy for some type.
2. Debug Eclipse w/ the plugin
3. Create a new java project
4. In the java project, create a subclass of the type whose TypeHierarchy is being listened to
5. In the source folder directory, create a dummy text file, save it
6. You should have seen a callback to your TypeHierarchy listener
Comment 1 Jay Arthanareeswaran CLA 2010-06-14 03:12:45 EDT
I am seeing the problem. I will investigate.
Comment 2 Jay Arthanareeswaran CLA 2010-06-15 07:45:12 EDT
Created attachment 171912 [details]
Proposed patch

Proposed patch + tests. More tests might be added.
Comment 3 Jay Arthanareeswaran CLA 2010-06-28 09:04:28 EDT
Created attachment 172902 [details]
Updated patch

Added a new tests in TypeHierarchyNotificationTests.testAddNonJavaToPackageFragmentRoot_b(). However this tests is failing because the code that processes the fine grained changes seems to be broken. Will investigate further and raise a new bug if required.
Comment 4 Jay Arthanareeswaran CLA 2010-07-02 06:20:03 EDT
Created attachment 173286 [details]
Patch with updated tests

The failing test has been fixed. All tests pass.
Comment 5 Jay Arthanareeswaran CLA 2010-07-02 06:23:01 EDT
Srikanth, would you have some time to review this patch? Thanks!
Comment 6 Srikanth Sankaran CLA 2010-07-05 02:53:12 EDT
(In reply to comment #5)
> Srikanth, would you have some time to review this patch? Thanks!

I think the patch is on the right track, however

(1) I don't think the tests are testing the right thing: You can
back out the source change and run the new tests and 2/3 pass. This
indicates that the tests are not precise enough.

(2) Should org.eclipse.jdt.core.tests.model.TypeHierarchyNotificationTests.testBug316654()
cleanup the txt file that is being created ? 

(3) For
org.eclipse.jdt.core.tests.model.TypeHierarchyNotificationTests.testBug316654_b()    
the javadoc says it can throw CoreException, but the method throw spec has
Exception in it -- Is this intentional ?
Comment 7 Jay Arthanareeswaran CLA 2010-07-05 05:05:05 EDT
Created attachment 173395 [details]
Updated pat

Patch incorporates comments (2) and (3).

About (1) - Those are regression tests that should pass with and without the fix. Since I didn't find any existing tests for those scenarios, I added them anyway. Test TypeHierarchyNotification#testBug316654_b() is particularly important because it is directly related to the code that is being changed in this patch.
Comment 8 Jay Arthanareeswaran CLA 2010-07-06 06:08:29 EDT
Added additional test in TypeHierarchyNotificationTests.testBug316654_c(), which is similar to testBug316654_b() except that it tests a change to an internal JAR.

Released in HEAD for 3.7M1.
Comment 9 Satyam Kandula CLA 2010-08-04 07:24:26 EDT
Verified for 3.7M1 using build I20100802-1800