This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Test case will be attached. Product Version = NetBeans IDE Dev (Build 100514-3a3dbb12a89f) Operating System = Windows 7 version 6.1 running on x86 Java; VM; Vendor = 1.6.0_19 Runtime = Java HotSpot(TM) Client VM 16.2-b04
Run the project, open it in the new IDE and right-click RewriteInCommentBug -> Show removeComment bug. It doesn't work. In the original IDE, switch the removeComment(Tree,Name) implementation (comment the current impl and uncomment the following block) and Reload in Target platform. It doesn't work either. IMHO, it's a bad idea requiring the tree to be touched because it will lead to reformatting in method bodies, for instance, which generates unnecessary diffs.
Created attachment 99020 [details] test case
The RemoveCommentBugAction itself is buggy: for (int i = size - 1; i > 0; i--) { will not remove anything for size == 1 (i = size - 1 = 0 !> 0). When I fix this (>= 0), the second variant works on the provided file. The first variant cannot work, as no change to trees (rewrite) actually happens. I agree this is not very nice, but this certainly does not qualify as P1/P2 defect (I think there was a bug filled for this, but do not have it offhand). The only solution is that {add|remove}Comment would need to return a Tree as all other altering methods do. The second variant in the action is currently the recommended way to alter comments. It should not cause any reformatting, etc., provided the "semantic" of the tree does not change (note that if just changing the class name would cause reformatting of the body of the class, rename refactoring would cause such reformatting too - if you have a particular case where this happens, please send me steps to reproduce). I have noticed a problem when there are multiple comments and one of them is removed, which I am trying to solve now.
(Sorry if the previous comment sounds harsh - it was not the intent. I appreciate the test case that shows exactly what is being done.)
>> The RemoveCommentBugAction itself is buggy: Yes, really sorry for that. I was trying to demonstrate a problem in production code (which removes comments conditionally), but it seems to be more complicated than that. I will try to post a new one. > The first variant cannot work, as no change to trees (rewrite) actually happens. > I agree this is not very nice, but this certainly does not qualify as P1/P2 > defect (I think there was a bug filled for this, but do not have it offhand). Well, I respectfully disagree. Since the Javadoc for the method doesn't mention that, it's either a documentation or an implementation bug. In this case, it would be a documentation bug, but (keep reading). > The second variant in the action is currently the recommended way > to alter comments. It should not cause any reformatting, etc., provided the > "semantic" of the tree does not change (note that if just changing the class > name would cause reformatting of the body of the class, rename refactoring > would cause such reformatting too - if you have a particular case where this > happens, please send me steps to reproduce). Yes, this reformatting happens inside some methods. Blank lines are removed. Currently, "Extract method" does the same bad reformatting I am talking about here. The bad thing is it just happens for some methods, not all, as most bugs I've been facing with the API these days... > I have noticed a problem when there are multiple comments and one of them is > removed, which I am trying to solve now. Good, so I didn't simply waste your time here. :-)
(In reply to comment #5) > > The second variant in the action is currently the recommended way > > to alter comments. It should not cause any reformatting, etc., provided the > > "semantic" of the tree does not change (note that if just changing the class > > name would cause reformatting of the body of the class, rename refactoring > > would cause such reformatting too - if you have a particular case where this > > happens, please send me steps to reproduce). > > Yes, this reformatting happens inside some methods. Blank lines are removed. > Currently, "Extract method" does the same bad reformatting I am talking about > here. If you mean the standard introduce method hint, note that it does much more than a dummy change to a tree. It actually moves statements into a different method, which is very different from what needs to be done for the comments to work. If there is no change to some part of a tree, the intent is not to change the corresponding text. I have a patch that partially solves the introduce method problem (for existing trees, it "simply" copies the code from one location to another and adjusts indent), but I was not courageous enough to put it in just before release.
To clarify: the comment about refactoring was meant with regard to rename refactoring, as that relates to the dummy changes that are currently needed to make comments work.
I know Extract method / Introduce method does more than a dummy change to a tree. The problem is some methods are, for some reason, rewritten the same way Introduce method would mess them. I really would like to be able to isolate the code, but I can't.
Hopefully this new test case isn't buggy. The problem happens when one tries to use removeComment and addComment at the same time.
Created attachment 99026 [details] updated test case
Created attachment 99057 [details] Binary patch. Binary patch - to test it place it into: ${NETBEANS_INSTALL_DIR}/java/modules/patches/org-netbeans-modules-java-source/186176.jar
Seems that the comment handling is broken more than I thought. I have a patch that should fix the problem from the last test case and also two other problems I noticed before. I have attached a binary patch, could you please test it? I am a bit afraid to put it into 6.9 - we are very close to final branching and there probably wouldn't be enough time to fix possible regressions (all java.hints and generator tests of course pass, but that does not guarantee that there are not regressions). Jiri, do you know if a patch release is planned? Thanks.
There will be at least one patch release after 6.9.
Integrated into 'main-golden', will be available in build *201005192201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/17cb391e32e9 User: Jan Lahoda <jlahoda@netbeans.org> Log: #186176: improving comment handling.
Integrated in post-6.9 trunk. Could you please check whether the patch helps? Thanks.
Hi Jan, Your patch worked but had a bad side-effect in my workaround code :-( I will try to come up with a use case for what is broken now, but here is a description of what the code did: - It added annotations to method by just rewriting its ModifierTree; - Then it would process the method to see, by business logic, whether comments should be retained. If some should be touched and some removed, I would create a new MethodTree and use setLabel to modify it and then add an entry to a Map<ElementHandle,List<Comment>>. ElementHandle was created based on the old MethodTree, since trying to use the new one even after it has been written leads to a NPE; - The ModificationTask would end and if the map mentioned above was not empty, I would start a second ModificationTask that would readd the "right" comments. This used to work, but now comments are added in the middle of source code, what seemed to be the original MethodTree position in source code. I used MethodTree in this description, but my code does the same for ClassTrees as well.
Sorry, but it is not clear to me what is wrong from the description - some clear steps to reproduce will be needed. Anyway, this is probably a different problem than the original report.