Fix sticky scroll height calculation for variable line heights#3748
Fix sticky scroll height calculation for variable line heights#3748utafrali wants to merge 2 commits intoeclipse-platform:masterfrom
Conversation
…ScrollingControl Replace parameterless getLineHeight() calls with the offset-aware getLineHeight(int offset) variant to correctly account for lines containing emoji or other extended characters that increase line height. - In calculateAndSetStickyLinesCanvasBounds(), sum per-line heights using stickyLineText.getLineHeight(offset) so the canvas bounds accurately reflect the actual rendered height of each sticky line. - In limitVisibleStickyLinesToTextWidgetHeight(), use textWidget.getLineHeight(topOffset) so the visible line count estimate respects the actual height of the top visible line.
|
@Christopher-Hermann can you pls. review? |
|
Closing — can't sign the ECA. |
You mean, there is a technical problem or you won't sign? |
|
Honestly I'm not comfortable with the obligations that come with signing the ECA — I feel like it goes against the open source spirit. Closing this one out. |
Could you please give some example? Eclipse contributors agreement is nowhere against open source. Whole Eclipse Foundation and all open source projects hosted here are based on work of people signed ECA. |
|
Yes I too am genuinely interested in the concrete concerns! I’m on the Board of Directors IP Advisory Committee as an elected committer representative so I can raise any concerns at the highest levels. I’ve never heard anyone express concerns about signing the agreement coming with obligations. Mostly it is intended as an assertion that you authored the content and agree to make it available under the open source license. I’d like to understand how you see it differently. We want everyone to be included and valued in our community with no obligations beyond personal integrity. |
|
Yeah, you're right. I think I just got spooked by the word "agreement" without actually looking into it. I've done a bit more reading and I'm happy to sign the ECA. Could you point me in the right direction for linking it to my GitHub account? |
|
Start here We’re here to help you. Just ask. |
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
Christopher-Hermann
left a comment
Christopher-Hermann
left a comment
There was a problem hiding this comment.
Thanks for providing a fix 👍
Productive coding lokos good. I added one comment about the test
| for (int i = 0; i < 2; i++) { | ||
| expectedHeight += stickyLineText.getLineHeight(stickyLineText.getOffsetAtLine(i)); | ||
| } |
There was a problem hiding this comment.
Not sure about the test. I remember that it was kind of ugly to write proper assertions on the size since OS, rendering, etc. all effects the unit test. So having hard coded size will not work. But I guess this test would be green even without the fix, right?
What do you think about this idea:
- Set the 2 sticky lines as you did, get the
stickyControlCanvas.getBounds().height - Replace the second sticky line with something that requires more space. As far as I understand the description emoji for example? get the new height
- Assert that the new height (with emoji) is greater than the old height (without emoji)
I guess this would also be a good documentation of the actual bug, what do you think?
6 participants
When emoji or other content affects line heights, sticky scroll was calculating the total height wrong by assuming all lines were the same. Now it gets the actual height for each line instead. Also fixes the height check for limiting visible lines.