Another Bug Fix Part 2

This part describes my process for fixing the bug.

As I mentioned in part 1 I believed that the problem simply came down to refreshing the UI after the parts list was updated. So the questions I needed to answer were: where could I find the UI refresh method and where could I call it that would make the most sense?

Answering the first question was simple enough. After some light searching I came across the ExtensionDetailsViewer.java class. This class was responsible for generating the UI for the Extension Details section. In this class was a refresh() method. Perfect.

As for the answer to the second question, this was also easy enough. I simply needed to find out where the "Specify Parts" dialog was being initiated and include a call to refresh() after that.

After looking at the code for awhile trying to understand it, I came across a few lines that interested me located within a method called createButtonControl().


Button button = new Button(composite, SWT.NONE);
GridData gridData = new GridData();
gridData.heightHint = 17;
button.setLayoutData(gridData);
button.addSelectionListener(internalControlListener);
button.setData(ITEM_DATA, item);
button.setData(EDITOR_CONFIGURATION_DATA, configuration);


I thought this might be the button I was looking for (the one that opens up the Specify Parts dialog). I tested it by setting the button's text to something different and sure enough it changed, so I knew that this was the right button.

I took note of the addSelectionListener() call, as the listener that was passed was most likely what opens the dialog. The internalControlListener was an instance of the InternalControlListener class, the contents of which were actually defined inside ExtensionDetailsViewer.java. Looking at the class, I came across a method called widgetSelected()


public void widgetSelected(SelectionEvent e)
{
   // for button controls we handle selection events
   //
   Object item = e.widget.getData(EDITOR_CONFIGURATION_DATA);
   if (item == null)
      item = e.widget.getData(ITEM_DATA);
   if (item instanceof DialogNodeEditorConfiguration)
   {
      DialogNodeEditorConfiguration dialogNodeEditorConfiguration = (DialogNodeEditorConfiguration)item;
      dialogNodeEditorConfiguration.invokeDialog();
   }
   else if (item instanceof ExtensionItem)
   {
      applyEdit((ExtensionItem)item, e.widget);
   }
}


I took notice of the if statement that checks if the item is an instanceof DialogNodeEditorConfiguration. I suspected that it was here that the Specify Parts dialog was called. So I put a breakpoint on the invokeDialog() call and traced it. Sure enough, when I clicked on the "..." button I was stopped at this call. I stepped through it and ended up at a class called SOAPSelectPartsDialog.java which was responsible for creating the Specify Parts UI. So I knew then for sure that this line is what started the whole process. To test my original theory I inserted a call to refresh() right below the invokeDialog() call and ran the application. As I suspected, the UI was now fixed! After updating the parts and clicking OK in the Specify Parts dialog, the text in the parts field changed without me having to change focus.

You can find my patch on Bugzilla.

Another Bug Fix Part 1

I had mentioned earlier that this bug seemed a little more complicated. As it turns out, the fix was equally as simple as the previous bug.

Since I haven't given much detail on this bug yet I'll divide this post into 2 parts. The first part will be about re-creating the bug, the second part will describe how I fixed it.

As you may recall from my previous posts, the last bug I worked on dealt with the extensions tab of the WSDL editor. This bug deals with the same area of the UI, however unlike the previous bug this one is related to the Extension Details section rather than the Extensions tree list.

The problem in this section related to one attribute known as "parts." Parts seems to be a list of objects that makes up the soap:body extension. As you can see in the image below, the soap:body of the example WSDL I created has 2 parts, "parameters" and "NewPart"



At first nothing seems wrong with this scenario. However, to find the bug we have to click on the Button next to the parts field labeled "..." which pops up the "Specify Parts" dialog.

Next, I unchecked NewPart and clicked OK. Now we can see the problem. In the image below, notice that the parts field still displays "parameters NewPart". The NewPart is still included as a part according to the UI.

Now, as you can see the focus has been lost from the soap:body extension. I re-clicked it, and the results are below.

Now the parts field simply displays "parameters". The UI is now behaving correctly. The fact that there wasn't any actual problem updating the parts list told me that the problem was simply the UI not updating after the parts list was updated. The fact that the UI fixed itself after I changed focus back to soap:body told me that the UI perhaps just needed to be refreshed. I'll describe my fix in Part 2.

Update, New Bug

It's been awhile since my last update. Unfortunately some other courses of mine have taken up much of my time and I've had to put Eclipse on the backburner.

My previous bug still hasn't been reviewed it seems, so I'm still waiting on that and I hope I get some feedback soon.

In the meantime, I figured it didn't make alot of sense to end my semester with one bug completed a few weeks early, so I decided to take a look at Bug 235381 to see if there was any work I could do on that.

Although I didn't find a lot of time to look at it in-depth, I quickly noticed that the code for the "Parts" attribute seems quite a bit more complicated than the Delete button from the previous bug. The code to populate, modify and display this list spans across a bunch of files and it was hard to determine where exactly the problem was. While at first glance it simply seems like a UI refresh problem, it might actually be more complicated than that. I will go into further detail early next week and hopefully make some sort of progress on this.

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.

0.2 Release

Well, I believe I'm ready to release 0.2. If you read my last blog, I was currently evaluating what changes I need to make between two files (AddExtensionsComponentDialog.java, AbstractExtensionsSection.java). I figured out that AbstractExtensionsSection.java was the file I was looking for.

I ran a find for "removeButton.setEnabled" (the method which enables and disables buttons) within this file and I found two lines, one of which was located within a method called refresh(). The name alone suggested that this might be relevant. It would make sense for the section to be refreshed on a consistent basis. Here is the code:


public void refresh()
{
setListenerEnabled(false);
if (input != null)
{
if ( prevInput == input)
return;
else
prevInput = input;

Tree tree = extensionTreeViewer.getTree();
extensionDetailsViewer.setInput(null);
tree.removeAll();

addButton.setEnabled(!isReadOnly);

extensionTreeViewer.setInput(input);
if (tree.getSelectionCount() == 0 && tree.getItemCount() > 0)
{
TreeItem treeItem = tree.getItem(0);
extensionTreeViewer.setSelection(new StructuredSelection(treeItem.getData()));
}

removeButton.setEnabled(tree.getSelectionCount() > 0 && !isReadOnly);

// Bugzilla 197315. Make this bulletproof for maintenance release.
Control detailsViewerControl = extensionDetailsViewer.getControl();

if (detailsViewerControl != null && !detailsViewerControl.isDisposed())
{
detailsViewerControl.setEnabled(!isReadOnly);
}
}
setListenerEnabled(true);
}


The instance of Tree represents the list of extensions, which is highlighted in the photo below:



And as I mentioned before, removeButton refers to the Delete button which I am trying to fix. At first I believed that perhaps refresh() simply wasn't getting called when switching tabs e.g. clicking on Extensions, then clicking Documentation, then clicking Extensions again.

So I inserted a breakpoint at the beginning of the method and ran a stack trace. Turns out refresh() actually DOES get called whenever the tabs are switched. So why is the button not being re-disabled? This isn't even the real issue, which I'll explain later.

I then realized that even though refresh() was being called after every switch, the method was being exited early. If we look at this code:


if ( prevInput == input)
return;
else
prevInput = input;


Notice here that if "prevInput" is not equal to "input" then the method should return out. I realized that prevInput refers to the previous status of the Extensions frame. It was being compared to the current status of the frame, and because switching tabs wasn't changing the actual contents of the frame, the method was exiting early. To change this, I changed the if statement to look like this:


if ( prevInput != input)
prevInput = input;


When I ran the Eclipse Application, the button behaved properly!



Note that I switched tabs before switching back to the Extensions tab in this picture. When I clicked on an object in the WSDL editor the buttons kept their proper state. It would appear that the problem is solved.

However, to me the real issue is: Why does refresh() need to be called every time to persist the state of the buttons? Why don't the buttons remember their state? I believe that the answer isn't that the buttons are forgetting their state, I believe it's because something else is changing their state. This would suggest that the problem actually doesn't occur in refresh(). The change I made now is a bit of a cheap fix, what I would like to do is eventually find the root of this problem and fix it, which is why I don't plan on submitting a patch yet.

Clearly the developers wanted to make sure that the code executed in refresh() wasn't done every time a tab was switched. Thus, the problem with my solution is that by cutting the old if statement out, I ensure that the code inside refresh() is executed on every tab switch, which is a waste of resources. So for 0.3 my current objective is to find the underlying problem as to why the Delete button's state change isn't being persisted, and that means finding out where else the button is being changed.

Its been awhile! Update

Well, first I'll briefly explain my absense. I had originally been under the assumption that I was following the DPS911 release schedule (which is once every 2 weeks), turns out after talking to Dave i was actually following the DPS909 release schedule which was a huge relief. Because of my earlier assumption I had already done alot of work on Eclipse and had put myself ahead of the rest of the class, so I decided to leave it for a couple of weeks and focus on other courses.

Now that the rest of the class is around where I am, I feel it's time to get back to work on this thing. Currently the objective is to find source code thats relevant to our bug. If you've read my post from a few weeks back you'll know that I thought that I had found some relevant code but I couldn't verify this because I had problems getting the WSDL editor to load. I found out that it was because the code I was checking out in the HEAD tree inside the CVS repo wasn't matched up with the version of WTP I was running, so I checked out code from an older tree, problem solved.

Next I needed to test whether the "removeButton" Button was the same as the Button I was looking to change. I added a line of code to set the removeButton's text to "Test".

removeButton.setText("Test");

Next I ran my Eclipse Application. Looking at the WSDL editor, my button had it's text changed to Test. Success!



However, there was a slight problem. This removeButton was referenced only one time in the W11ExtensionsSection.java file that I was working in. This basically told me that this wasn't the file I wanted to make my changes in.

Fortunately for me, I able to attend a presentation by IBM's Angel Vera last week. During the presentation, Mr. Vera showed us some techniques that we could use to find code using the "Search" feature of Eclipse. Using this, I searched for "removeButton" within the org.eclipse.wst.wsdl and org.eclipse.wst.wsdl.ui packages. To my suprise, the only reference to removeButton was the one that I had already identified, meaning that this Button was declared in a different package. Fortunately, this package was easy to identify. I simply had to mouse over the removeButton reference inside the code and it led me to the org.eclipse.wst.xsd.ui package. I checked this package out and ran a search on it, and found many references to the removeButton, mainly in 2 files (AddExtensionsComponentDialog.java, AbstractExtensionsSection.java). I now realize that I am probably going to have to make changes within one of these 2 files or maybe even both in order to fix my bug.

So, that's currently where I stand. I know where I need to change things, but I don't know yet what changes I need to make. I feel confident that I'll be able to figure things out.

Some Success, Some Roadblocks

Well firstly I believe I've located some relevant code. The page for the bug I'm working on shows that the component is wst.wsdl. Using this information I happened upon 2 packages: org.eclipse.wst.wsdl and org.eclipse.wst.wsdl.ui. I hoped that these packages would contain some code that could point me in the right direction, and fortunately I found one file that could prove interesting. Under the UI package I found another package called org.eclipse.wst.wsdl.ui.internal.properties.sections. Within this package I found a java file called W11ExtensionsSection. Success? I hope so. I feel like this could be a big break since within this file I found references to two buttons called addButton and removeButton. As my previous post explained, the section of the UI I'm focusing on has Add and Delete buttons, so these references could possibly have something to do with those 2 particular buttons.

I've been unable to check as to whether or not the code is relevant. In my previous post I showed a screenshot of the WSDL editor in my main Eclipse window. When I try to debug in a new Eclipse Application, for some reason the WSDL editor isn't there. I've decided to look to the Eclipse Webtools Newsgroup for help, so hopefully in time I'll have the answer to my problem.

I've thought about what I can submit for a 0.4 Release. I definitely won't be able to make a patch for this fix until a later release, so I'm not sure what I can create in-between. Perhaps a simple patch making a slight change to the UI? I'll have to discuss this with Dave.