-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Set letterSpacing to 0 for cursive writing systems #596
base: androidx-main
Are you sure you want to change the base?
Conversation
* as it can break the connection between letters and make the text fragmented. | ||
*/ | ||
@Stable | ||
private fun String.isFirstLetterCursive(): Boolean { |
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.
In this version, we have defined a binarySearchRange extension for the range list that performs a binary search to check if a value is in any of the ranges. This drastically reduces the number of comparisons needed compared to the original approach of comparing against each range individually.
@Stable
private fun String.isFirstLetterCursive(): Boolean {
val cursiveRanges = listOf(
1536..1920,
1984..2048,
2144..2304,
4096..4256,
43488..43520,
43616..43648,
64336..65024,
65136..65280
)
for (char in this) {
if (!char.isLetter()) continue
val c = char.code
if (cursiveRanges.binarySearchRange(c)) {
return true
}
}
return false
}
private fun List<IntRange>.binarySearchRange(value: Int): Boolean {
var low = 0
var high = size - 1
while (low <= high) {
val mid = (low + high) / 2
val range = this[mid]
if (value in range) {
return true
}
if (value < range.first) {
high = mid - 1
} else {
low = mid + 1
}
}
return false
}
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.
Hey @miguelalegria, thanks for the suggestion. The reason I didn't incorporate binary search and ranges is to avoid unnecessary allocations given the size of our search. I assume the current workaround would be more efficient. However, this pull request isn't probably going to be merged since the problem lies in lower layer of the system.
Hey @YektaDev thanks for surfacing this issue and proposing a fix! I don't work directly on the Compose text stack and this should probably ultimately be decided by someone who does, but I thought I would share some of my thoughts. There's a bit of a philosophical question here. I definitely agree that a cursive writing system and large letter spacing feel a bit at odds with each other and that in most cases, applying a positive letter spacing here is usually not desired, but I am also not sure that enforcing that here is the right place to be doing so. One thing in particular that bothers me about this solution is the fact that we aren't giving anyone a way to get around it if they need to. Additionally, this is being done for Material 3's I can imagine some fonts may handle cursive characters slightly differently and may require very slight tweaking of letter spacing in order to display correctly, and this would defeat it. I can imagine some typesetting app where adjusting these values would be expected to result in differences. I am sure there are others but I'm not all that imaginative :) Additionally, I am curious if there are examples of any other frameworks or toolkits which behave this way? Android does not and I just tried and it looks like browsers do not either. |
Hey @lelandrichardson, thanks for your time and attention. I agree with almost all of the points you've made. Especially that Material 3's However, for cursive systems ─ or at least for Persian and Arabic since I can speak for them ─ fragmentation of characters is not a matter of "not feeling right", it is simply broken and wrong (it's even mentioned by https://www.w3.org/TR/css-text-3/#example-9902d8b5). Even though I still agree that this is the way both Android and Browsers behave and this gives us a valid reason to not touch the current implementation and leave it as it is (and of course, it's not possible given the breaking change it introduces). I came up with another idea, which I don't know if it's feasible. What if this kind of behavior/fix can be opted-in through the use of some flag or property (maybe something in |
Letter spacing doesn't play well with cursive writing systems, as it can break the connection between letters and make the text fragmented. To solve this problem, I propose checking the first "letter" character in the text using
isLetter()
. If it belongs to a cursive writing system (Persian and Arabic to name a few) andletterSpacing
is greater than 0, setletterSpacing
to0.sp
. However, I agree that it feels "hacky" and better fixes can be applied at lower levels. I just wanted to share my solution to this.An example of how the text is fragmented
This is a "Hello World" in Persian:
سلام دنیا
This is how it looks like when space is added between the letters (Which looks broken/odd in a cursive writing system):
سـ ـلـ ـا م د نـ ـیـ ـا