-
Notifications
You must be signed in to change notification settings - Fork 231
Increase the height of bracket selection by one point #3653
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
base: master
Are you sure you want to change the base?
Conversation
|
LGTM |
|
@HeikoKlare any concerns here? |
laeubi
left a comment
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 -1 does not make any sense here.
bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/source/MatchingCharacterPainter.java
Outdated
Show resolved
Hide resolved
|
This PR misses the mentioned that it is an extract of #3597, which combined two issues:
While the first change is fine for me, I proposed to extract out the second change (removing the "-1") into a separate PR (see #3597 (comment)), as the "-1" was there to fit the highlight into the line (see screenshot here #3597 (review)). So there seems to habe been a specific reason for the -1 and it might be up for discussion if that should be removed (i. i.e., if the highlight should fit in the line or if highlight should not being drawn over the bottom part of the bracket). And the PR description actually seems wrong, as the example with the bracket exceeding the highlight at some zooms is about the OfFloat change, which is already covered by that other PR. The screenshot of the result is a combination of the two changes instead of just the single one that is supposed to be done by this PR (putting the "-1" removal on top. @ShahzaibIbrahim can you please update this PR accordingly? If I am not mistaken, this change on top of https://github.com/eclipse-platform/eclipse.platform.ui/pull/3597/files will just change this appearance: |
|
Sorry, I failed to mention that this PR depends on #3597 before being review (to avoid conflicts). I will update the screenshots as well. |
dbb2767 to
4d991a5
Compare
In some zoom level the highlight is too close to the text and cutoff sometimes.
4d991a5 to
bd4c7a7
Compare
@laeubi I guess your change request (#3653 (review)) is obsolete since the PR now exactly removes the -1. |
In some zoom level the highlight is too close to the text and cutoff sometimes.
Example:
Here with zoom 175% the bracket selection seems to cut off the text.

After increasing the size by 1 pt, we get something like this
To me this looks much more cleaner than the current state.