Community
Participate
Working Groups
wtp 1.5.1 9/14 dtd outline view does not update when delete attribute 1. create a new dtd with the following: <!ELEMENT NewElement1 EMPTY> <!ELEMENT NewElement2 EMPTY> <!ATTLIST NewElement2 NewAttribute CDATA #IMPLIED> <!ELEMENT NewElement3 EMPTY> 2. From the outline view, rightclick "NewAttribute" and select to delete 3. notice the attribute is deleted in the editor, but the outline view is not updated
This becomes an even worse problem when you try to delete the attribute for a second time because then it'll delete text and corrupt the dtd.
Created attachment 104459 [details] Log from removing DTD attribute This is the log file I see when I try and double-delete an attribute from the Outline view of a DTD. This is actually causing refresh problems on the DTD editor and Outline view.
Created attachment 154892 [details] Fix Patch There are two problems here for the two different scenarios. For the scenario where a user right clicks on the DTD outline tree and selects delete the DTDTreeContentProvider is notified through the nodesRemoved method. Currently this method does not then update the tree viewer like its nodesAdded counter part. The second scenario is a user selecting an attribute node in the DTD file and then deleting it. In this case nodeChanged gets called but its logic for updating a node if its an AttributeList only gets executed if isShowLogicalOrder() is true. Based on the comment for this if statment "in this case, we are showing attributes under the element. refresh the element object instead" it seems that attributes may have used to only be listed under elements when "isShowLogicalOrder" is true, but it now seems to be the case that attributes are listed under elements weather or not isShowLogicalOrder is true. Thus the specialty case for if the given DTDNOde in nodeChagned is an AttributeList should be executed regardless of the value of isShowLogicalOrder. With these two fixes nodes can be removed using the outline view or by manually highlighting a node in the DTD, and as a result the outline view will be correctly updated.
Any similarities to Bug 245342, or will they both just be modifying the same sections of code?
(In reply to comment #4) > Any similarities to Bug 245342, or will they both just be modifying the same > sections of code? Just from glancing at it I am not sure. But I will check into if the patch for this bug fixes any of the scenarios in Bug 245342.
(In reply to comment #4) > Any similarities to Bug 245342, or will they both just be modifying the same > sections of code? The patches are indeed very similar just from looking at the DIFFs. Do you want me to look into merging into one awesome patch?
Created attachment 154989 [details] Fix Patch Update 1 This patch gives DTDTreeContentProvider a face lift. There was a bunch of duplicated and confusing code in there. So I moved things around to get rid of duplicated code and make a simpler code path. All of this in an effort to fix both this bug and Bug 245342. I have included SOME of the ideas that were in the patch for Bug 245342 but at this point I don't think any of them exist in their original form. This patch fixes the scenario described in this bug along with all of the scenarios discussed in the description of Bug 245342 including the problem the patch for Bug 245342 does not fix. I would write some JUnits for this but this really seems like a graphical thing because it all has to do with figuring out which parents nodes to refresh. if someone has a suggestion for a way to go about writing JUnits for this I am open to suggestions.
(In reply to comment #7) > Created an attachment (id=154989) [details] > Fix Patch Update 1 > > This patch gives DTDTreeContentProvider a face lift. There was a bunch of > duplicated and confusing code in there. So I moved things around to get rid of > duplicated code and make a simpler code path. All of this in an effort to fix > both this bug and Bug 245342. I have included SOME of the ideas that were in > the patch for Bug 245342 but at this point I don't think any of them exist in > their original form. > > This patch fixes the scenario described in this bug along with all of the > scenarios discussed in the description of Bug 245342 including the problem the > patch for Bug 245342 does not fix. > > I would write some JUnits for this but this really seems like a graphical thing > because it all has to do with figuring out which parents nodes to refresh. if > someone has a suggestion for a way to go about writing JUnits for this I am > open to suggestions. Look at SWTBot.
I'd like to see some unit tests if possible. The way the code is currently designed it is hard to test with out doing some reflection on items. The DTD component is sorely lacking unit tests, and it would help to have them to make sure we don't regress.
(In reply to comment #9) > I'd like to see some unit tests if possible. The way the code is currently > designed it is hard to test with out doing some reflection on items. The DTD > component is sorely lacking unit tests, and it would help to have them to make > sure we don't regress. As I said in comment #7 I was not sure of the best way to go about this. Where is the best place to get information on this SWTBot? I found their eclipse project page and from what I read it seems like its own stand alone thing not integrated with JUnit. Though I just glanced the project quickly. Are there any existing examples of using SWTBot for JUnit tests? Any other ideas for testing this?
(In reply to comment #10) > (In reply to comment #9) > > I'd like to see some unit tests if possible. The way the code is currently > > designed it is hard to test with out doing some reflection on items. The DTD > > component is sorely lacking unit tests, and it would help to have them to make > > sure we don't regress. > > As I said in comment #7 I was not sure of the best way to go about this. Where > is the best place to get information on this SWTBot? I found their eclipse > project page and from what I read it seems like its own stand alone thing not > integrated with JUnit. Though I just glanced the project quickly. Are there > any existing examples of using SWTBot for JUnit tests? Any other ideas for > testing this? Several different ways I can think about testing this: 1. Create a Fake or Mock of the interfaces you are using or classes that you are using. 2. Write the code so that you can test the internals with out having to use reflection. Ideally all of the classes that you are using should have their own individual unit tests. 3. For a functional test us Junit and SWTBot. Notice the SWTBot requires JUnit 4 to run not JUnit 3. For a tutorial that goes through using SWTBot, http://www.eclipsecon.org/2010/sessions/?page=sessions&id=1258 I think you could get this to the point were you can test without necessarily using SWTBot though.
Talking over with Nick the best way to test this would be to grab the Outline view and grab its nodes and compare it to what is expected based on the editor content. While I understand the desire for JUnits the time it would take to build up JUnit tests for the DTD outline view from scratch is just not something I have time for. If there were existing tests that just needed to be expanded to test the user stories this patch fixes then it might be a different matter, but with no currently existing tests writing the tests from scratch is not something I have time for. I sincerely hope you can still find a way to put in these fixes seeing as they do greatly improve the usability of the DTD outline. Possibly opening a separate bug to track writing DTD outline tests from the ground up should be opened as an enhancement.
(In reply to comment #12) > Talking over with Nick the best way to test this would be to grab the Outline > view and grab its nodes and compare it to what is expected based on the editor > content. > > While I understand the desire for JUnits the time it would take to build up > JUnit tests for the DTD outline view from scratch is just not something I have > time for. > If there were existing tests that just needed to be expanded to test the user > stories this patch fixes then it might be a different matter, but with no > currently existing tests writing the tests from scratch is not something I have > time for. Sorry...I need to be a stickler here. The existing DTD component is very sparsely tested, so we need to increase the unit test cover. A patch should always come with unit tests. The tests are for our benefit to make sure that we don't introduce any regressions in the future. > > I sincerely hope you can still find a way to put in these fixes seeing as they > do greatly improve the usability of the DTD outline. Possibly opening a > separate bug to track writing DTD outline tests from the ground up should be > opened as an enhancement. Sorry, I'm not going to open a separate bug to write unit tests. The reason is, because they tend not to get written. We are all busy but it really benefits the team as a whole if we have the unit tests along with the code the patch that it is fixing.
*** Bug 337477 has been marked as a duplicate of this bug. ***
*** Bug 245342 has been marked as a duplicate of this bug. ***
Code changes checked in.
