-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
code minings drawn more consistently always via drawAsLeftOf1stCharacter #2243
code minings drawn more consistently always via drawAsLeftOf1stCharacter #2243
Conversation
Test Results 1 815 files ±0 1 815 suites ±0 1h 39m 32s ⏱️ + 5m 21s For more details on these failures and errors, see this check. Results for commit 8c67165. ± Comparison against base commit f9b743f. ♻️ This comment has been updated with latest results. |
"pr-head" says: Pls add a second mitt that increases the version of org.eclipse.jface-text to "3.25.300". This should fix this issue. |
371ac78
to
5f67aff
Compare
There seems to be an infrastructure issue:
|
5f67aff
to
28f94ae
Compare
@mickaelistria: What are your thoughts on this PR? |
28f94ae
to
8c61750
Compare
org.eclipse.jface.text.tests also needs a version bump:
Can you pls. ammend your version change commit? |
Some of the code mining tests are failing: org.eclipse.jface.text.tests.codemining.CodeMiningTest.testCodeMiningOnEmptyLine |
c0f3414
to
0b85447
Compare
Thanks |
I didn't try this PR yet. |
Thanks a lot for the feedback @mickaelistria. I fully agree. I just tested the java method parameter code minings with this change and selection behavior looks ugly. Without this change, one can select the method parameter value 0 and the code mining text is not included. With this change, this is no longer possible. Selecting 0 always includes the code mining which does not feel right. I will make a proposal with an API enhancement so that the selection behavior of the code minings can be configured. For sure in a compatible way. |
Yes. That's a good idea. |
0b85447
to
fdabddd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested it, but the code looks already good.
Just a comment inline discussing whether a new CodeMining type is useful here.
* Line content code mining which is drawn right to the given position. Cursor and selection | ||
* handling are not including the code mining. | ||
*/ | ||
public abstract class SuffixLineContentCodeMining extends LineContentCodeMining { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather imagine a new flag on LineContentCodeMning than a whole new method here.
IMO, a new constructor LineContentCodeMining(Position, boolean afterPosition /* default being false in other constructors that don't have this arg */, provider)
would be simpler to both consume and maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the fast reply. I changed the API as requested and added a further constructor in LineContentCodeMining. I have put "boolean afterPosition" as last contructor argument for compatibility reasons. Do you think it is better to place it as second argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For compatibility, you only need to keep the current constructors. You're free to design your new ones freely.
So yes, I think it's better to place it as 2nd argument if it's not conflicting with any other existing constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I added it as 2nd argument. Is it because first argument is "position" and next argument "afterPosition" is then influencing the position?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the position where the codemining is rendered is a function of those 2 arguments. Grouping them together makes things faster to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, understood. Makes absolutely sense.
fdabddd
to
fbdbd21
Compare
fbdbd21
to
46e6ea3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes about probable complaints from API tools about new API methods.
* @param afterPosition if true code mining is treated as suffix code mining where cursor and | ||
* selection is not including the mining | ||
*/ | ||
public LineContentCodeMining(Position position, boolean afterPosition, ICodeMiningProvider provider, Consumer<MouseEvent> action) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is new API, this will likely require some @since 3.26
and a version increase in the MANIFEST.
@@ -158,7 +158,7 @@ StyleRange updateStyle(StyleRange style, FontMetrics fontMetrics, ITextViewer vi | |||
return style; | |||
} | |||
|
|||
static boolean drawRightToPreviousChar(int widgetOffset, StyledText textWidget) { | |||
protected boolean drawRightToPreviousChar(int widgetOffset, StyledText textWidget) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is new API, this will likely require some @since 3.26
and a version increase in the MANIFEST.
Or better if you can find a way to not make this method API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot for the help. I tried to fix the API tools messages with 80b1736. I added an additional argument to package method LineContentAnnotation#updateStyle . Is this allowed? If not, I would need to introduce method "protected boolean drawRightToPreviousChar" as protected instance method.
46e6ea3
to
80b1736
Compare
Yes, it is allowed because this method is package visible only (not API then). |
* | ||
* @since 3.26 | ||
*/ | ||
protected boolean afterPosition= false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there is a getter, couldn't this one be private final
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree, thanks for the feedback.
which allows to render a code mining where the cursor selection does not include the code mining at the given source position.
80b1736
to
8c67165
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks very good and the use-case is pretty interesting. Let's merge it as soon as CI reports a successful build on Jenkins.
failed test are unrelated to this PR |
Thank you for this good code and for making the IDE capable of achieving 1 new use-case! |
this improves the selection behavior for the scenario where the selection end offset contains a LineContentAnnotation. Selection is no longer extended with the code mining.
The selection behavior before this change always included the code mining text which is located at the end selection offset. The following screenshot shows the behavior. By including the last character "o" of "echo" in the selection, the selection is extended with the code mining text next to the "o" character.
This is no longer the case with this change. The code mining text is no longer added to the selection when "echo" is selected.
Is the current behavior of drawing the code mining texts intended? Is this behavior needed for a feature (which I am not aware of)?
Let me try to explain why I request this change.
In the ABAP Development Tools in Eclipse we use the code minings to show inline keyword completions while the user is typing a keyword. If a matching keyword is available, it is shown as code mining and user can take over the text by pressing the tab key. Here a screencast showing the scenario when the user is typing the keyword "definition".
cursor_jumps.mp4
It works quite well. Users don't like the way how the cursor always jumps to the end of the code mining showing the rest of the keyword "definition". The cursor jumps to the end of the code mining because method drawAsRightOfPreviousCharacter is used to render the code mining.
The cursor does not jump to the end of the code mining with this pull request by using method drawAsLeftOf1stCharacter to render the code mining.
cursor_does_not_jump.mp4
The question now is if we would break any existing functionality regarding rendering of code minings by always using method drawAsLeftOf1stCharacter?
An alternative approach would be to provide an API with which the user can decide how to draw the code mining. What would be your preferred solution? Could you please give any guidance?
Thanks a lot,
Tobias