Bugzilla – Full Text Bug Listing
| Summary: | First attribute quote is deleted if insert JSP expression through statement completion | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Source Editing | Reporter: | Jill Macklem <jillm> | ||||||||||||||
| Component: | wst.sse | Assignee: | Amy Wu <for.work.things> | ||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P2 | CC: | david_williams, jeffliu, wlw-releng | ||||||||||||||
| Version: | unspecified | ||||||||||||||||
| Target Milestone: | 1.0.2 M102 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows XP | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Jill Macklem
This was on IBuild1028. Phil, judging from the abstract, this appears your area of expertise? Any word on a target milestone for this one? It is pretty annoying when you edit JSPs a lot. Phillip, this is a usability issue, but users hit it repeatedly. Added to the 102 hot list. Setting priority to p2 and target to 1.0.2 M102 03/16 WTP status meeting - No progress this week. Will investigate. What are you expecting if you have the following scenario: 1. In a new JSP page, place cursor inside the empty quotes of an attribute (for example, place cursor inside the "" of the href attribute in the line below): <a href="my|link"></a> 2. Hit ctrl-space at | to bring up the statement completion selection box 3. Select JSP expression from statement completion box Do you expect <a href="my<%= %>link"></a> or <a href="<%= %>"></a> or how about if there are no "" would you expect inserting the jsp expression to also insert the quotes? The problem has to do with the insertion offset being set to right after the = and then overwriting the attribute value with whatever you select. The tricky thing is the insertion offset is used by all proposals, not just the jsp expression, so if I change it, the other proposals will have to be changed to react to the new expected offset, and I don't think that will be very safe. I'll investigate further to see if there's something we can do on the template side. Ignore my questions in the previous comment. I've answered them myself. Inserting templates like JSP expression will no longer delete the first attribute quote. Also, if you insert a template that is supposed to be valid in "all" context, the template will be inserted wherever your cursor is (unless your cursor is in the middle of a word, then it will replace the beginning of the word) So basically: Ex1: with <a href="|"></a> content assist on | and insert template Results: <a href="template"></a> Ex2: with <a href="hello|there"></a> content assist on | and insert template Results: <a href="templatethere"></a> Ex3: with <a href="hello |there"></a> content assist on | and insert template Results: <a href="hello templatethere"></a> This bug is actually a bug for xml, html, and jsp (not just jsp) so I'll move over to the wst.sse component. Created attachment 37284 [details]
org.eclipse.jst.jsp.ui.patch
-add new private method, addTemplates that takes in 1 extra parameter for startOffset (so use startOffset can be explicitly set)
-in addAttributeValueProposals, check if attribute value region starts with ' or " if so, make startOffset the offset after the quote, and call the new addTemplates method
-in computeCompletionProposals call new addTemplates method and pass in original document offset passed in (instead of start offset of document region)
Created attachment 37285 [details]
org.eclipse.wst.html.ui.patch
-add new private method, addTemplates that takes in 1 extra parameter for startOffset (so use startOffset can be explicitly set)
-in addAttributeValueProposals, check if attribute value region starts with ' or " if so, make startOffset the offset after the quote, and call the new addTemplates method
-in computeCompletionProposals call new addTemplates method and pass in original document offset passed in (instead of start offset of document region)
Created attachment 37286 [details]
org.eclipse.wst.xml.ui.patch
-add new private method, addTemplates that takes in 1 extra parameter for startOffset (so use startOffset can be explicitly set)
-in addAttributeValueProposals, check if attribute value region starts with ' or " if so, make startOffset the offset after the quote, and call the new addTemplates method
-in computeCompletionProposals call new addTemplates method and pass in original document offset passed in (instead of start offset of document region)
attached the fix (it's the same in all 3 plugins) Amy, upon reviewing these patches, they look fine, except I'm a tiny bit worried about one odd case. If an existing template (that an adopter already has added in their plugin or a user had defined in their preferred templates) started with a quote, would this "change their behavior" in an unexpected way? For example, if, being aware of the existing behavior of overwriting a quote, they *might* have defined some templates to use '\"attrValue\"' then would they now end up with ""attrValue"? (that is, two leading quotes?) In 1.5, we could, perhaps justify some small "change in behavior", since I expect pretty rare, but in 1.0.2, I think they'd expect it to still work "as is" without them changeing their code (and, of course, if we do that way for 1.0.2, we could just leave that way for 1.5). So, would it be possible to check the first character of the chosen template, and then if its a single or double quote, adjust the effect of a pre-existing single or double quote? (And, of course, we have to do something different if one is a single and one is a double, since they might be expecting to end up with something like attribute="'xvvv"). Feel free to point out if this is already handled "indirectly" by the way the algorigthm works, but I'd just like to to make sure its "covered". Good point. I might be able to do something if the template is applicable only in the "attribute value" context. Maybe I can check if the character right before the template insertion point is ' or " and if the first character in the template is a ' or " However, I'm not so sure if I should do the same if the template is applicable in the "all" context. In that case, the template is valid anywhere in the document and can be inserted anywhere, so there really should not be any special processing/handling of quotes for that case (fyi, jsp expression is valid in "all" context) Yes, I agree, attribute values only is where we want to do this. Please do investigate for this bug, though, since I think there is that teeny tiny chance that otherwise this fix might appear to someone somwwhere as a "change in behavior". Sorry, didn't mean to mark as fixed ... had some bugzilla mouse click problems. Okay. For this 1.0.2, I will only be fixing the templates valid in "ALL" context. I will not fix the "attribute value" context templates because that issue seems to require more thought. (I will open another bug to track the attribute value issue) The JSP expression template is an "all" context template, so it will be fixed. Explanation of problem When inserting an "all" context template via content assist, the template will overwrite any text between the start of the text region and the point of insertion (where content assist was invoked. For example: if you invoke content assist at | and you have the following: Example1: <tag attr="hello | how are you?" /> inserting a template will give you Results: <tag attr=TEMPLATE how are you?" /> Note: '"hello ' was overwritten. Expected: <tag attr="hello TEMPLATE how are you?" /> Note: template is just inserted. Example2: <tag attr="hello TEM| how are you?" /> inserting a template will give you Results: <tag attr=TEMPLATE how are you?" /> Note: '"hello ' was overwritten. Expected: <tag attr="hello TEMPLATE how are you?" /> Note: template is just inserted. *and* since "TEM" was already written, only the rest of the template is written. Solution of problem When adding "all" context template proposals, use the offset where content assist was invoked instead of the offset where the text region starts. I also added extra calculation to handle Example2 so that if you are inserting a template, and you've already started typing the template name, then when you insert the template, the template name that you typed will be overwritten by the template (so that there is no redundancy. this is how java templates work as well) Created attachment 37374 [details]
org.eclipse.jst.jsp.ui.patch
Created attachment 37375 [details]
org.eclipse.wst.html.ui.patch
Created attachment 37376 [details]
org.eclipse.wst.xml.ui.patch
I've updated the patches with the new fix. The same fix is in each plugin. modify XXXContentAssistProcessor -created new method addTemplates(ContentAssistRequest, String, int) which is just like addTemplates(ContentAssistRequest, String) except the additional int (offset where content assist was invoked) is passed into the TemplateCompletionProcessor instead of the offset from ContentAssistRequest (offset of text region) -when adding "all" context templates, call the new addTemplates method, passiing in the offset where content assist was invoked new class: ReplaceNameTemplateContext -extends platform's DocumentTemplateContext which was originally used -when evaluating template to be inserted, check if the text before the template to be inserted matches the template name, and take that into consideration when inserting template modify XXXTemplateCompletionProcessor -override computeCompletionProposals to use new ReplaceNameTemplateContext Small change to ReplaceNameTemplateContext.evaluate. I had put in an unnecessary try/catch. The method was already throwing that exception, so the try/catch should be removed, and the exception will be thrown instead. Thanks Amy. I just wanted to document here that I have reviewed all the alternatives with Amy, and have carefully reviewed the code, and agree this is a good fix, and an important one. While the problem is definitely not of the 'stop ship' variety, it is a very common use-case for users to want to use jsp expression after typing attribueNme=" So is an important usability fix. Amy will open bugzillas to cover other similar (but less common) cases that are not fixed by this case (and would be a tiny bit riskier), but we believe we are covering what is the "80% case" here. bug 134320 was opened to track the outstanding issues about "attribute value" context I had mentioned in comment 17. +1 for WTP 1.0.2 RC Fix released to M & I build. (1.0.2 & 1.5) Verified fixed using 1.5. closing |