Increase the height of bracket selection by one point #3653
Conversation
Because of rounding errors the bracket selection highlight is often inconsistent. Making use of residual values provides precise rectangle values and highlight is consistent across all zooms
In some zoom level the highlight is too close to the text and cutoff sometimes.
|
LGTM |
|
@HeikoKlare any concerns here? |
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
Yes the -1 does not make any sense here.
|
|
||
| // draw box around line segment | ||
| gc.drawRectangle(bounds.x, bounds.y + bounds.height - height, width, height - 1); | ||
| gc.drawRectangle(rectangleOfFloat); |
There was a problem hiding this comment.
Why do we need the rectangle of floats here at all? for me the error seems simply to subtract one from the height so a much simpler to remove the -1 from the original code.
gc.drawRectangle(bounds.x, bounds.y + bounds.height - height, width, height);
|
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: with this: 
|
|
Sorry, I failed to mention that this PR depends on #3597 before being review (to avoid conflicts). I will update the screenshots as well. |
4 participants
In some zoom level the highlight is too close to the text and cutoff sometimes.
Example:
Here with zoom 125% the bracket selection seems to cut off the text.
After increasing the size by 1 pt, we get something like this