Community
Participate
Working Groups
It seems that the "toggle comment" action (ctrl + shift + c) always uses "<!-- -->" style comments, even in Java sections (in a JSP). The action should be content type/partition aware to use the appropriate style of comments.
*** Bug 104849 has been marked as a duplicate of this bug. ***
Amy ... this is actually a new and exciting case we haven't really dealt with before, I don't think. The appropriate action depend on location of start and end points of selection, and also what the selection spans. Please development a brief design document to address these issues in the context of this specific use case. I suspect it would apply to some other cases too, besides comments ... such as "surround with try/catch".
Also .. see related bug 104843 ... don't be fooled by the incorrect behavior of CA and validation.
Yes, this is really a problem for jsp writers.
reassigning to inbox
Currently working on a patch that over halls the commenting for all structured documents. It depends on Bug 282189
Created attachment 140629 [details] The Long Awaited Commenting Fix THE PROBLEM: This patch completely over hauls the current commenting handler implementation for structured documents. Currently there is only one handler in the wst.xml.ui plugin that handles all of the commenting for XML/HTML/JSP. As many bugs have noticed (this one apparently being the first) this means that no matter what you comment in any of these files it gets commented with <!-- -->, this is obviously not the desired outcome. THE SOLUTION: My solution was to create separate handlers for XML, HTML, and JSP files but all steaming from one new class AbstractStructuredCommentHandler that I put in the wst.see.ui plugin seeing as all of the other effected plugins rely on that one. This parent class has almost all (see ISSUES) of the shared functionality between all of the comment handlers for the three file types. Then each file type has its own abstract handler class that inherits from the shared one that implements more specific implementation to the given file type. Then lastly for each file type there is 3 different concrete handlers for toggling, adding block and removing block (see ISSUES, for issues with this). Then each plugin, xml.ui, jsp.ui and html.ui now have extensions to plug their own handlers in for their file content type. The magic of this is with the way it is written it is rather simple to allow for different type of comment prefixes and suffices depending on the partition type/s of the user selected regions in a given file type (see ISSUES for issues with partitions). This means that depending on the user selected partitions the correct enclosing comment prefixes and/or suffixes can be chosen to comment or un-comment the selected code. In sum, this bug is now fixed along with others that I have marked as depending on this one, and the comment handling code is now "easier" to maintain should there be a need for dealing with some new type of partition type in any given file type. ISSUES WITH SOLUTION: 1. I have a pet peeve with my solution in that the Toggler***Handler, Add***BlockHandler, and Remove***BlockHandler for the three file types are esentialy copies of eachother except for which Abstract***CommentHandler class they extend from. This would be a perfict place to use multiple inheritence if java would allow it to take the shared functionality from the Abstract***CommentHandler and the shared functionality between all Toggle***Hanlder (or Add*** or Remove***) and get them in one class to be the specific handler for a specific file type. But allas there is no multiple inheritence so I as far as I could tell I either had to duplicate code that is the same for a given file type or the same for a given comment handler type (toggle, add, remove) and seeing as the comment handler type specific code was smaller I decide to duplicate that instead of the latter. If anyone has any suggests as how to better structure this (it may help to download the patch and see the hierarchy for yourself) I am MORE then open to suggestions to help get rid of this duplicate code. I have marked all copied classes as such so HOPEFULLY if it does has to stay as duplicate code and anyone has to update it later they will see its duplicate code and can be kind enough to pass the updates along to the other copies. But again some other solution to this problem is more then welcome. 2. The other problem is with partition types. Currently there are no specific partition types for code commented out in <script></scirpt> or <% %> regions. And because of this the Remove***BlockCommentHandler is unable to find the comment prefixes and suffixes surrounding a user highlighted selection to remove said prefixes and suffixes. I have opened Bug 282213 and Bug 282214 to address this issue. Once those have been fixed the abstract handler classes for HTML and JSP files can be easily updated to handle the new partition types. TESTING: I have attempted to test every possible commenting permutation in the three file types (XML, HTML, JSP) to be sure it all works. It is more then likely I did not think of every strange permutation but I am confident that I thought of at least the common ones and they all work. Much time was testing highlighting different types of partitions in JSP files to be certain it would commented the entire selection with the appropriate comment type, especially because the appropriate comment type can change as multiple lines are commented out. Such as if a <% is commented out then all further lines might have originally been commented out with // but now need to be commented out as html with <!-- --> It would not be a bad idea if others could test this patch and let me know if they find any scenarios that do not work as expected. I did my best co copy the functionality of the way the Java comment handlers appear to work. FUTURE WORK: With this new easily extensible handler framework commenting for CSS files should be implemented I would think. I only held of on doing it now in case my solution gets changed around at all to fix ISSUE #1 (duplicated code) OTHER: All comments, suggestions, questions, etc are more then welcome and asked for, thanks!
Created attachment 140906 [details] Patch with no duplicated code This patch eliminates my pet peeve described in my first patch by moving the shared algorithms for add comment block, remove comment block, and toggle comment to 3 static methods in sse.ui. With this patch each of the handler implementations now call these static methods passing themselves as the handler to use. This eliminates the duplicated code issue.
Bug 282559 was created to track implementation of this framework for CSS files.
*** Bug 261975 has been marked as a duplicate of this bug. ***
Comment on attachment 140906 [details] Patch with no duplicated code This patch still presents the time issue discussed in Bug 261975. I am working on an updated patch which will be more speedy when toggling the comments on a large number of lines.
Created attachment 146755 [details] Patch Update 3 This patch updates the way the toggle line comment operation works. As pointed out by Bug 261975, toggling many lines has a tendency to lock things up. This update makes it so that a rewrite session is used and if toggling more then 10 lines then the operation is done by displaying a ProgressMontiorDialog. This dialog also has the cancel button enabled and if canceled will stop toggling lines, but the lines that have been toggled thus far will stay toggled. By using the rewrite session the time it takes to toggle a 1000+ lines is now only a few seconds and with the added benefit of seeing the progress bar the user knows the program has not locked up but it simply takes a bit of time to add in all of the comment tags.
(In reply to comment #12) > Created an attachment (id=146755) [details] > Patch Update 3 Did a very quick glance over the code. Some suggestions. 1) You don't need to create the class StaticStructuredCommandHandlers. Instead, move the methods into AbstractStructuredCommentHandler and make them non-static protected. You don't need to pass the handler then, you can call the handler methods directly on the AbstractStructuredCommentHandler class. This also allows subclasses to override/tweak these methods if they need to. 2) In #processToggleCommentAction() you do some readAndDispatch on the Display. This is so that the UI keeps painting while you do actions in the UI. (Multi-plexing the UI thread.) This approach is icky. There are some platform specific peculiarities that others have already tried to workaround. (See the org.eclipse.ui.internal.dialogs.EventLoopProgressMonitor for some docs on the hacks they use.) Also, you are not handling any exceptions that might result from readAndDispatch(). (You should ignore them and log them, but if you log them, you get the blame for them - but readAndDispatch can be running anybody's arbitrary code. You can't mimick the exact handling what workbench does because it's ExceptionHandler.getInstance() is internal. For these reasons, I'm suggesting a much cleaner alternative. There is a much better solution in the form of PlatformUI.getWorkbench().getProgressService.busyCursorWhile(IRunnableWithProgress). This method will fork a Thread to run the runnable. For the first 800ms, no dialog will be shown (just a busy cursor), then it is replaced with a ProgressJobsDialog. So, for fast actions, the user will just see a busy cursor, if they take longer, they see a dialog with the option to cancel. You don't need to anticipate the "magic number" of how many lines you need to exceed before opening a dialog, as this method will do that for you.
Not sure if it matters, but commenting out across partitions is a little odd. Maybe you could commenting out the lines that contain partitions or something like that? i.e. nice <% int a = 0; %> If you select 3 lines (not 4), you'll get this <!--nice--> <%--<%--%> <!-- int a = 0;--> %> This comments out the partition, leading to errors. Maybe, better yet, would be to somehow make the commenting structural-aware. i.e. <a>nice </a> If you just try to comment the first line, you'll get this: <!--<a>nice--> </a> It might be more desirable to comment out the entire element... i.e. <!--<a>nice </a>-->
(In reply to comment #14) I certainly think that sounds like an interesting idea but I have some reservations about it. I know for myself if I am saying I want to toggle the comments on some specific lines then that's what I want to do, I want to comment comments on some SPECIFIC lines. If the tooling went and commented out other lines IT thought I wanted commented I would be annoyed. Maybe having another key-combination/menu option for commenting out the containing region of the cursor or selection would be a good idea as future work.
Created attachment 147138 [details] Patch Update 4 (In reply to comment #13) > (In reply to comment #12) > > Created an attachment (id=146755) [details] [details] > > Patch Update 3 > > Did a very quick glance over the code. Some suggestions. > > 1) > You don't need to create the class StaticStructuredCommandHandlers. Instead, > move the methods into AbstractStructuredCommentHandler and make them non-static > protected. You don't need to pass the handler then, you can call the handler > methods directly on the AbstractStructuredCommentHandler class. This also > allows subclasses to override/tweak these methods if they need to. Good idea. Don't know how I missed that one, guess I have gotten to close to the code. Changes included in attached patch. > 2) > There is a much better solution in the form of > PlatformUI.getWorkbench().getProgressService.busyCursorWhile(IRunnableWithProgress). > This method will fork a Thread to run the runnable. For the first 800ms, no > dialog will be shown (just a busy cursor), then it is replaced with a > ProgressJobsDialog. So, for fast actions, the user will just see a busy cursor, > if they take longer, they see a dialog with the option to cancel. You don't > need to anticipate the "magic number" of how many lines you need to exceed > before opening a dialog, as this method will do that for you. as we discussed this approach unfortunately locks up the UI making the cancel button un-clickable. In an attempt to make the call to readAndDispatch safer it has not been warped in a try/catch to log most Exceptions and Errors that are not caused by the commenting code and would not effect the commenting code if they occurred.
Will be happy when the feature gets implemented, otherwise it is a pain to comment JSP files!
Hi, what's the status of this bug?
(In reply to comment #18) > Hi, > what's the status of this bug? It is waiting for review. It is a major patch and needs careful review before committing.
Comment on attachment 147138 [details] Patch Update 4 After some discussion with Nick and Nitin it was the consensus that I could write this patch more robustly now that I have a few more months of experience.
cool! Ian, does that mean you're still targetting this bug for 3.2?
(In reply to comment #21) > cool! Ian, does that mean you're still targetting this bug for 3.2? Yes.
Created attachment 159026 [details] Patch - Update 5 Tada! Here is is, the long awaited, most amazing, commenting patch new and improved with extension point. Rather then go into all the gory details the basic idea here is that there is now a org.eclipse.wst.sse.ui.add.block.comment.commentingStrategy extension point that allows you to specify different types of comments to use in different types of content types for different types of partitions. This way the plugins that know about a specific language can implement the commenting strategy for that language even for different content types. So as an example, the CSS plugin now implements the extension to implement CSS comments in CSS documents as well as HTML and JSP documents for the correct partition types. As always this code is crazily commented so to get the gory details see the patch.
*** Bug 282559 has been marked as a duplicate of this bug. ***
I forgot to mention that there are two known "bugs" with this patch, style and script comment blocks can not be removed because neither has a specific partition type for the comments. There are already bugs open to fix the script case for HTML and JSP document Bug 282213 and Bug 282214 and I will open bugs for the style region as well.
(In reply to comment #25) > I forgot to mention that there are two known "bugs" with this patch, style and > script comment blocks can not be removed because neither has a specific > partition type for the comments. There are already bugs open to fix the script > case for HTML and JSP document Bug 282213 and Bug 282214 and I will open bugs > for the style region as well. I have opened Bug 302763 and Bug 302764 to track the needed fixes for the style regions.
Created attachment 159376 [details] Patch - Update 6 The menu contributions were all messed up in the last patch. This patch makes it so the menu contributions/handlers/contexts/definitions all interact in a similar way as to the other source menu contributions.
Created attachment 160677 [details] Patch - Update 7 Old patch had become stale and would no longer apply. Updated patch to apply to latest HEAD.
Created attachment 160843 [details] Patch - Update 8 Updates previous patch to have the handlers be internal and then use the IHandlerService in StructuredTextEditor to register the handlers for the command types. Adds new context "org.eclipse.wst.sse.comments" that the menu contributions provided by the sse.ui plugin for comment handling are activated on. Thus any editor type wishing to have the commenting commands show up for their editor must have the new "org.eclipse.wst.sse.comments" context registered for their content type. All of this helped cut down on extensions and made it so the handlers could be made private instead of API.
Created attachment 160936 [details] Patch - Update 9 Nick pointed out to me offline that this patch suffered from the same issue I have run into before, child content types not picking up extensions defined for their parents. Without this functionality the commenting actions would not work on documents such as the web.xml because they are a different content type then just the plan old xml content type. To this end I have updated the code to search through the registry for all of the commenting strategies registered for a given content type and that content types parents, and then choose the strategy from these results that applies to the current regions. This allows child content types to pick up the new commenting for "free" without having to do anything*. * This is not entirety true, if the content type in question has any partition types that the parent content types commenting strategies are on aware of then the parent content types strategies will not work for thosue specific partition types in the child content type. In that case new commenting strategies to deal with those partition types for that child content type will have to be written. The end result of this is that no one should loose any commenting functionality and should either gain some, or just have the same amount they had before this patch.
Created attachment 160938 [details] Patch - Update 10 Forgot to include the JSDT fix in the last patch :(
I think we should defer this; CommentingStrategy needs another pass. The class should be more consistent in requiring either the document or model as parameters on its methods (I'm not sure why it would ever require the model). The use of a list of IDs and whether they're not all required also mimics the kind of and/or functionality provided by Core Expressions, which we should avoid duplicating. Is there a reason ToggleLinesRunnable does not fork?
Deferring until 3.3. We'll definitely want to look into simplifying/demystifying the API.
As mentioned and discussed, we'll need to keep working on smoothing out the API.
Created attachment 166769 [details] Patch - Update 11 (In reply to comment #32) > I think we should defer this; CommentingStrategy needs another pass. The class > should be more consistent in requiring either the document or model as > parameters on its methods (I'm not sure why it would ever require the model). This patch fixes the issue of passing around the model and now only uses StructuredDocument. This most likely occurred due to all the copy and pasting because of the changing from not using extension points to using extension points. > The use of a list of IDs and whether they're not all required also mimics the > kind of and/or functionality provided by Core Expressions, which we should > avoid duplicating. I still need to look into this. > Is there a reason ToggleLinesRunnable does not fork? Not sure what you mean here. If you mean where is it run in a seperate process you should take a look at the end of ToggleLineCommentHandler#processAction where there is an IF that either runs the runnable with a ProgressMonitorDialog if there are more then 10 lines to toggle or else just runs the runnable directly. 10 as a semi arbitrary number but I do feel there is no need to open a progress monitor dialog when just coping a couple lines, but once you get into bigger numbers this operation can take some time and thus the Dialog is key to keeping the user informed and allowing them to cancel.
(In reply to comment #32) > The use of a list of IDs and whether they're not all required also mimics the > kind of and/or functionality provided by Core Expressions, which we should > avoid duplicating. Are there any examples in SSE code of using Core Expressions or any recommended reading for putting them to use?
(In reply to comment #36) > (In reply to comment #32) > > The use of a list of IDs and whether they're not all required also mimics the > > kind of and/or functionality provided by Core Expressions, which we should > > avoid duplicating. > > Are there any examples in SSE code of using Core Expressions or any recommended > reading for putting them to use? I don't think any of the SSE extension points use them directly, but there are some examples of using them enabling/disabling handlers in the various menus. You can also get some more information on the framework from this wiki page: http://wiki.eclipse.org/Command_Core_Expressions
*** Bug 316068 has been marked as a duplicate of this bug. ***
Created attachment 178931 [details] Patch - Update 12 This is an update of the previous patch to work with the current 3.2.x stream. It also removes one of the fields in the extension point as per Nitin's request. I spent some time looking into the core expressions/enablement extension points and whether they would be a good fit for this use case. My findings were that they would not. Due to the complexity of determine what type of comment needs to be used based on which partition types are highlighted in a specific content type I believe that a specific solution for this problem (like the one I have provided here) is the best solution. I am all for re-using existing code and not re-inventing the wheel but in this case I do not believe I have done that. In my attempts to figure out how to use the core expressions/enablement extension points I felt like I was trying to put a square peg in a round whole, it just does not seem like the right tool for this particular job. I have VERY thoroughly documented the new extension point to make it as clear as I possibly can. I am requesting a new review.
Thanks for the review Nitin! Your turn Nick :)
Approved. I made a couple spelling fixes and removed a method that only invoked its super. The more important thing I cleaned up was the removal of the xml.comments context. If anyone was dependent on this context being defined, we'd get errors in the log. Also, XML UI was missing its activation of org.eclipse.wst.sse.comments, which caused no commenting menu contributions. Other than those things, it looks good and will be nice to finally have this. Thanks.
Code checked in.
