Community
Participate
Working Groups
Currently line_selection implements the same code as word_selection. To benefit from enhancements in word_selection (see Bug 486903) I suggest that LineSelection inherits from WordSelection.
(In reply to Lars Vogel from comment #0) > Currently line_selection implements the same code as word_selection. To > benefit from enhancements in word_selection (see Bug 486903) I suggest that > LineSelection inherits from WordSelection. Not possible due to default constructor in WordSelection. Retargeting the bug to make the same change in LineSelection as in Bug 486903.
New Gerrit change created: https://git.eclipse.org/r/66114
Gerrit change https://git.eclipse.org/r/66114 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=e462c416ddf66bbf893a0f0364be8f88a28963a4
Obviously you have not tested this a single time. I can see by just looking at the code that this won't work and testing proves me right. Test Case: - add a Java editor template yyy with ${x:line_selection(works) - insert yyy in the Java editor on an empty selection ==> works is not inserted
(In reply to Dani Megert from comment #4) I think the change in platform.text is correct. I just re-ran the "TemplateVariablesWordSelectionTest" with adjustments to use line selection instead of wordselection and the tests are green. Line selection used the exact same code as word selection in platform.text AFAICS the incorrect code in the JDT consumer. Looks like http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=1ede3a21fda48901261dad54e9a4cdc60577fb00 was not complete. Is this what you mean?
(In reply to Lars Vogel from comment #5) > (In reply to Dani Megert from comment #4) > > I think the change in platform.text is correct. I just re-ran the > "TemplateVariablesWordSelectionTest" with adjustments to use line selection > instead of wordselection and the tests are green. Line selection used the > exact same code as word selection in platform.text > > AFAICS the incorrect code in the JDT consumer. Looks like > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=1ede3a21fda48901261dad54e9a4cdc60577fb00 > was not complete. Is this what you mean? I was not arguing about the correctness of any code. I just say it does not work and I expect that any change is tested before being committed. And "no", the changes in TemplateEngine are not related.
(In reply to Dani Megert from comment #6) > I was not arguing about the correctness of any code. I just say it does not > work and I expect that any change is tested before being committed. Can you share what you see as problem? It is a time consuming for all involved parties, if you let others guess. My next guess is that you see the issue with the missing usage of JDT for the line_selection variable. JDT uses its own template (SurroundWithLineSelection), so the change would not be visible for JDT.
(In reply to Lars Vogel from comment #7) > (In reply to Dani Megert from comment #6) > > I was not arguing about the correctness of any code. I just say it does not > > work and I expect that any change is tested before being committed. > > Can you share what you see as problem? It is a time consuming for all > involved parties, if you let others guess. My next guess is that you see the > issue with the missing usage of JDT for the line_selection variable. JDT > uses its own template (SurroundWithLineSelection), so the change would not > be visible for JDT. OK, where is it noteworthy then? Where/who sees any benefit of this change? My guess is that you targeted the Java editor and there it is not visible. If you made this change for any other editor, then please name it here, I will verify that the change works and we are done.
(In reply to Dani Megert from comment #8) > OK, where is it noteworthy then? Where/who sees any benefit of this change? Every completion engine based on platform.text which uses the line_selection template will benefit from change. I don't know the consumers of text but my guess would be that CDT, Groovy editor etc, benefit from this. > My guess is that you targeted the Java editor and there it is not visible. > If you made this change for any other editor, then please name it here, I > will verify that the change works and we are done. As the friendly platform developer I'm, I plan to contribute the same change to JDT (I now opened Bug 487901 for that).
(In reply to Lars Vogel from comment #9) > (In reply to Dani Megert from comment #8) > > OK, where is it noteworthy then? Where/who sees any benefit of this change? > > Every completion engine based on platform.text which uses the line_selection > template will benefit from change. I don't know the consumers of text but my > guess would be that CDT, Groovy editor etc, benefit from this. > > > My guess is that you targeted the Java editor and there it is not visible. > > If you made this change for any other editor, then please name it here, I > > will verify that the change works and we are done. > > As the friendly platform developer I'm, I plan to contribute the same change > to JDT (I now opened Bug 487901 for that). I didn't want to sound unfriendly. I "guessed" the same as you i.e. that also works in the Java editor (like word_selection), but it didn't. Hence my question: in which editor did you test your code? Anyway, I verified it now in the Ant Editor. Note that word_selection also has an issue in the Java editor. Let's continue the discussion in bug 486903.
