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

Bug 312054

Summary: [preferences] UserLibraryPreferencePage: After renaming a user library the Up, Down, Remove buttons do not work until dialog is reopened
Product: [Eclipse Project] JDT Reporter: Rolf Wojtech <rolf>
Component: UIAssignee: Raksha Vasisht <raksha.vasisht>
Status: VERIFIED FIXED QA Contact:
Severity: minor    
Priority: P3 CC: daniel_megert, deepakazad
Version: 3.1   
Target Milestone: 3.7 M1   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
Patch
daniel_megert: review-
Patch_v2
daniel_megert: review-
Patch update daniel_megert: review+

Description Rolf Wojtech CLA 2010-05-07 09:06:35 EDT 
Build Identifier: 20100218-1602

When you open the user library preference page and rename a library, you won't be able to remove jar files from it (or change the order) until you close and reopen the dialog.
I am sorry that I do not have the time to dive into the code and search for a fix but I wanted to report it anyways.

Reproducible: Always

Steps to Reproduce:
1. Open or create a workspace with at least one User Library that has jar files in it
2. Go to the "User Libraries" dialog (e.g. Preferences -> Java -> Build Path -> User Libraries
3. Select a User Library, click Edit and rename it (e.g. Test -> Test2)
4. Try to use the Up/Down/Remove button on one of the jar files inside the user library. The click has no result.
Comment 1 Dani Megert CLA 2010-05-08 04:05:42 EDT 
Like that since 3.1.
Comment 2 Raksha Vasisht CLA 2010-07-18 15:30:43 EDT 
Created attachment 174584 [details]
Patch

Update the parent container of all the children of the library element to the new library element, thus when we select a jar and try to move up/down or remove it , we do the operation on the updated element and not the old element which still points to the old library as the parent container and hence we do not see the changes being reflected in the page.
Comment 3 Dani Megert CLA 2010-07-19 10:34:28 EDT 
The patch works but it doesn't fix the root cause:
    CPUserLibraryElement.CPUserLibraryElement(String, boolean, CPListElement[])
is broken since it allows to create invalid user library elements. We should adjust the parent of the element in there. This also allows us to keep the new setter package visible. Please also add some Javadoc to the constructor which says that the parent will be adjusted.
Comment 4 Raksha Vasisht CLA 2010-07-20 04:04:50 EDT 
(In reply to comment #3)
> The patch works but it doesn't fix the root cause:
>     CPUserLibraryElement.CPUserLibraryElement(String, boolean, CPListElement[])
> is broken since it allows to create invalid user library elements. We should
> adjust the parent of the element in there. This also allows us to keep the new
> setter package visible. Please also add some Javadoc to the constructor which
> says that the parent will be adjusted.

The fix cant be done there as we need to set the parent for the CPListElement as the newly created Library element id, because that is how we store it in the fLibraryList when we replace the old element with the new one. If we just do a setName() on the parent container in  CPUserLibraryElement.CPUserLibraryElement(String, boolean, CPListElement[]) for all the CPListElement[] elements, that would be different id from the new library created. 

The other place we could fix it is in 
CPUserLibraryElement org.eclipse.jdt.internal.ui.preferences.UserLibraryPreferencePage.LibraryNameDialog.getNewLibrary()
where we obtain the new library created and then assign it to the children.
Comment 5 Dani Megert CLA 2010-07-20 04:21:02 EDT 
Please read comment 3 again.
Comment 6 Raksha Vasisht CLA 2010-07-20 04:49:14 EDT 
Created attachment 174715 [details]
Patch_v2

(In reply to comment #4)

Oops, the fix can be made inside the constructor itself using 'this' as the parent container afterall! Attaching the fix along with the javadoc for the constructor.
Comment 7 Dani Megert CLA 2010-07-20 04:57:54 EDT 
Please read comment 3 again, e.g.
>This also allows us to keep the new setter package visible.

Also, why do you make all those checks in the constructor? This would still result in illegal/wrong user library element.
Comment 8 Raksha Vasisht CLA 2010-07-20 08:19:16 EDT 
Created attachment 174736 [details]
Patch update

(In reply to comment #7)
> Please read comment 3 again, e.g.
> >This also allows us to keep the new setter package visible.
> 
> Also, why do you make all those checks in the constructor? This would still
> result in illegal/wrong user library element.

Made the setter default and not protected.  The checks were made because the parent container returned as an object from the getParentContainer() and as a safety check I put in a instanceof check. Removed them as they are not needed and we can directly set the parent as the new library element for all the children elements.
Comment 9 Dani Megert CLA 2010-07-20 08:54:09 EDT 
Patch is good.

	 * Creates the new library element with the given name and children elements, also sets itself as the parent
	 * container to all the given children elements.
==>
	 * Creates a new user library element with the given name and children and sets itself as the parent container to each given child element.
Comment 10 Raksha Vasisht CLA 2010-07-21 02:30:40 EDT 
(In reply to comment #9)
> Patch is good.
Committed to HEAD with the javadoc change.
Comment 11 Deepak Azad CLA 2010-08-03 14:14:47 EDT 
Verified with I20100802-1800 on Ubuntu.
Comment 12 Deepak Azad CLA 2010-08-03 14:15:04 EDT 
.