When computing the size of a widget always return values rounded up#2551
Conversation
Test Results 118 files ±0 118 suites ±0 10m 37s ⏱️ +15s For more details on these failures, see this check. Results for commit 67a7758. ± Comparison against base commit ff6bf58. ♻️ This comment has been updated with latest results. |
ShahzaibIbrahim
force-pushed
the
round_size_up
branch
2 times, most recently
from
59b68e6 to
a479282
Compare
|
I cannot un draft it as this PR was not created by me. I also have tested the original issue after my changes: #2166. Also tested runtime workspace if something is broken. The commit message is needed to adapted and also some API check failures need to be fixed, but feel free to review/test it. |
ShahzaibIbrahim
force-pushed
the
round_size_up
branch
5 times, most recently
from
1926a71 to
9bdfef4
Compare
| private static final long serialVersionUID = -1862062276431597053L; | ||
|
|
||
| public float residualX, residualY; | ||
| public RoundingMode roundingMode; |
There was a problem hiding this comment.
@akoch-yatta this field was added as public while it is only used in this class internally. Was there a reason for that? Why not simply have it private?
There was a problem hiding this comment.
Probably because it was work in progress. If it can be private, make it private
There was a problem hiding this comment.
it should at least be final ... on the other hand residualX/y are public modifiable as well... I think one best make both private.
akoch-yatta
left a comment
akoch-yatta
left a comment
There was a problem hiding this comment.
Checking all changed calls in this PR, they look semantically correct, regarding the different rounding for sizes and locations
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
@amartya4256 thanks for taking over this and working on this, it looks all sane for me.
| private static final long serialVersionUID = -1862062276431597053L; | ||
|
|
||
| public float residualX, residualY; | ||
| public RoundingMode roundingMode; |
There was a problem hiding this comment.
it should at least be final ... on the other hand residualX/y are public modifiable as well... I think one best make both private.
ShahzaibIbrahim
force-pushed
the
round_size_up
branch
3 times, most recently
from
bdeebd9 to
147e135
Compare
When converting pixels to point there can be depending on the zoom level be a fractional result. Currently in all cases these result is converted into an integer using `Math.round()` that will make values `+/-0.5` resulting in small values to be round towards a smaller value. While it is maybe valid for a _location_, when using points to express a _dimension_ this is not okay as it will result in the reported (integer) value to be to small leading to errors when the SWT API is then used after performing additional computations maybe. See - eclipse-platform#2381 - eclipse-platform#2166
ShahzaibIbrahim
force-pushed
the
round_size_up
branch
from
147e135 to
67a7758
Compare
|
Changing residualX, residualY to private will be done in another PR and test failures are unrelated: #2516. From my side this PR is ready to be merge if no other objections. |
akoch-yatta
left a comment
akoch-yatta
left a comment
There was a problem hiding this comment.
I tested it with different zooms like 125%, 150% and 175% and didn't not see a regression caused by this. As I already had a look into the code this PR is good to go from my side.
|
Test are still failing after rebuild, but unrelated #2516 |
4 participants
When converting pixels to point there can be- depending on the zoom level - a fractional result. Currently, in all cases the result is converted into an integer using
Math.round()that will make values+/-0.5resulting in small values to be rounded towards a smaller value.While it is maybe valid for a location, when using points to express a dimension this is not okay as it will result in the reported (integer) value to be to small leading to errors when the SWT API is then used after performing additional computations maybe.
See
This PR also resolves the issue #2166