From 9d35b6c64c2b10e710a3d90ddbb210e2c744b4ed Mon Sep 17 00:00:00 2001 From: Stein Strindhaug Date: Mon, 12 Feb 2018 13:02:04 +0100 Subject: [PATCH 1/2] Improve line-height calculation accuracy. Round the division by 2 result up and down so that the sum is still the same as the lineheight when the additional variable happens to be odd. Added test test to see that bottom-top is still equals the line height when it's odd. This test does not care which value the implementation chooses to round up or down as long as the sum is correct. --- .../views/text/CustomLineHeightSpan.java | 10 ++++++---- .../views/text/CustomLineHeightSpanTest.java | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java index 18645e56df5736..06ce5cdb3bc7c4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java @@ -55,10 +55,12 @@ public void chooseHeight( // Show proportionally additional ascent / top & descent / bottom final int additional = mHeight - (-fm.top + fm.bottom); - fm.top -= additional / 2; - fm.ascent -= additional / 2; - fm.descent += additional / 2; - fm.bottom += additional / 2; + // Round up for the negative values and down for the positive values (arbritary choice) + // So that bottom - top equals additional even if it's an odd number. + fm.top -= Math.ceil(additional / 2.0f); + fm.ascent -= Math.ceil(additional / 2.0f); + fm.descent += Math.floor(additional / 2.0f); + fm.bottom += Math.floor(additional / 2.0f); } } } diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java index 5d3919105ae232..0e5194a57d43f2 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java @@ -13,7 +13,7 @@ public class CustomLineHeightSpanTest { @Test - public void shouldIncreaseAllMetricsProportionally() { + public void evenLineHeightShouldIncreaseAllMetricsProportionally() { CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(22); Paint.FontMetricsInt fm = new Paint.FontMetricsInt(); fm.top = -10; @@ -21,10 +21,27 @@ public void shouldIncreaseAllMetricsProportionally() { fm.descent = 5; fm.bottom = 10; customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm); + // Since line height is even it should be equally added to top and bottom. assertThat(fm.top).isEqualTo(-11); assertThat(fm.ascent).isEqualTo(-6); assertThat(fm.descent).isEqualTo(6); assertThat(fm.bottom).isEqualTo(11); + assertThat(fm.bottom - fm.top).isEqualTo(22); + } + + @Test + public void oddLineHeightShouldAlsoWork() { + CustomLineHeightSpan customLineHeightSpan = new CustomLineHeightSpan(23); + Paint.FontMetricsInt fm = new Paint.FontMetricsInt(); + fm.top = -10; + fm.ascent = -5; + fm.descent = 5; + fm.bottom = 10; + customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm); + // Only test that the sum is correct so the implementation + // is free to add the odd value either on top or bottom. + assertThat(fm.descent - fm.ascent).isEqualTo(13); + assertThat(fm.bottom - fm.top).isEqualTo(23); } @Test From 8ff3424b161d453e0cc758602718804812539a5d Mon Sep 17 00:00:00 2001 From: Stein Strindhaug Date: Tue, 13 Feb 2018 16:01:49 +0100 Subject: [PATCH 2/2] Fix line height in multiline texts --- .../com/facebook/react/views/text/CustomLineHeightSpan.java | 4 ++-- .../facebook/react/views/text/CustomLineHeightSpanTest.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java index 06ce5cdb3bc7c4..abba786b1bd44d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/CustomLineHeightSpan.java @@ -58,9 +58,9 @@ public void chooseHeight( // Round up for the negative values and down for the positive values (arbritary choice) // So that bottom - top equals additional even if it's an odd number. fm.top -= Math.ceil(additional / 2.0f); - fm.ascent -= Math.ceil(additional / 2.0f); - fm.descent += Math.floor(additional / 2.0f); fm.bottom += Math.floor(additional / 2.0f); + fm.ascent = fm.top; + fm.descent = fm.bottom; } } } diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java index 0e5194a57d43f2..eadb7d3544bcb5 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/views/text/CustomLineHeightSpanTest.java @@ -23,8 +23,8 @@ public void evenLineHeightShouldIncreaseAllMetricsProportionally() { customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm); // Since line height is even it should be equally added to top and bottom. assertThat(fm.top).isEqualTo(-11); - assertThat(fm.ascent).isEqualTo(-6); - assertThat(fm.descent).isEqualTo(6); + assertThat(fm.ascent).isEqualTo(-11); + assertThat(fm.descent).isEqualTo(11); assertThat(fm.bottom).isEqualTo(11); assertThat(fm.bottom - fm.top).isEqualTo(22); } @@ -40,7 +40,7 @@ public void oddLineHeightShouldAlsoWork() { customLineHeightSpan.chooseHeight("Hi", 0, 2, 0, 0, fm); // Only test that the sum is correct so the implementation // is free to add the odd value either on top or bottom. - assertThat(fm.descent - fm.ascent).isEqualTo(13); + assertThat(fm.descent - fm.ascent).isEqualTo(23); assertThat(fm.bottom - fm.top).isEqualTo(23); }