Wednesday, March 25, 2009

Problem Solved

In my last post I talked about my initial fix for Bug 209289 which I thought was a cheap fix since it didn't get to the real root of the problem which was solving why the refresh() method needed to run through every time in order for the delete button to be disabled. Why didn't the state of the button persist even if the refresh() method wasn't called? Clearly there was another piece of code modifying the button's state.

I tried debugging for awhile, running multiple stack traces to try to find the point where the button's state was being changed, with no luck. Along the way though, I came to understand the code more and more.

It hit me that AbstractExtensionsSection.java was a subclass of AbstractSection.java, so I decided to try finding something in that file. Unfortunately I found nothing, but I realized that the name "AbstractExtensionsSection" implies that there is probably a class that subclasses it. I ran a search and I came back upon W11ExtensionsSection.java, the file I originally referenced in my January 29th post and subsequently dismissed on my February 24th post because it only called the setEnabled() method on addButton and removeButton. Originally I figured that this couldn't have been where the problem was, that it had to be wherever the button was created and it's state was set. However now I had a better understanding of the code.

Looking at the method that the code was inside:


public void setInput(IWorkbenchPart part, ISelection selection)
{
super.setInput(part, selection);
isReadOnly = true;
ExtensibleElement extensibleElement = getExtensibleElement(input);
if (extensibleElement != null)
{
Element element = extensibleElement.getElement();
if (element instanceof ElementImpl)
{
isReadOnly = false;
modelAdapter = WSDLModelAdapter.lookupOrCreateModelAdapter(element.getOwnerDocument());
modelAdapter.getModelReconcileAdapter().addListener(internalNodeAdapter);
}
}
addButton.setEnabled(!isReadOnly);
removeButton.setEnabled(!isReadOnly);
}

I realized that enabling the two buttons here didn't make any sense. The logic for this is handled in the refresh() method in AbstractExtensionsSection, and if the state changes refresh() will run through again and adjust the state accordingly. So why does this method need to handle the buttons' state? It doesn't.

So, I removed these two lines from the method and ran the application. To my delight, the changes worked, and the results I saw using my cheap fix a couple of weeks ago were the same changes I was seeing now. This time, I did it without having to change refresh() execute every time the extensions section was returned to, which is a waste of resources. Instead my change simply leaves the handling of the buttons' state to the refresh() method, which I feel alot more comfortable with.

So now I need to take this solution to Bugzilla. Hopefully I'll be able to get my patch into the tree. Also, my next step is to find another bug to work on, as I have no intention of sitting around for the final few weeks of the semester.

3 comments:

Bliss said...

Very good Jesse!

Could you please post your findings on Bugzilla?

Thank you,
Jordan

pliu said...

Hi, Jesse.

Did you post your findings and the FIX to Bugzilla? It will be a valuable contribution to the WTP community.

Peter.

Bliss said...

Very good Jesse.

Let us hope that we will get soon a review and it will be a good one.

Jordan