Skip to content
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

Expose ICUs u_hasBinaryProperty #1029

Merged
merged 2 commits into from
Feb 28, 2025
Merged

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Feb 27, 2025

This is needed to detect emojis in Compose Multiplatform.

Also, it looks like the native implementations of String.intCodePoints() were broken. I fixed them by copying the relevant code from Compose.

internal fun Int.charCount(): Int = if (this >= MIN_SUPPLEMENTARY_CODE_POINT) 2 else 1

internal val String.codePointsAsIntArray: IntArray
get() = codePoints.toList().toIntArray()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it create an extra copy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does (actually more than once, inside toList()), but it's not easy to avoid this copy, as the number of codepoints is not known upfront. And if we just return List<Int> here, the callers will still need to convert it to an int array.

Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few nitpicks

Comment on lines 26 to 29
internal val String.codePoints
get() = codePointsAt(0)

internal fun String.codePointsAt(index: Int) = sequence {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in CharHelpers.skiko.kt mentioned is actually like:

Suggested change
internal val String.codePoints
get() = codePointsAt(0)
internal fun String.codePointsAt(index: Int) = sequence {
internal val CharSequence.codePoints get() = sequence {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the copy is after my changes in JetBrains/compose-multiplatform-core#1869
This is more generic and is not worse, so why change it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String/CharSequence consistency

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have it in more than one place; the one I copied it from uses String, but sure, CharSequence is better.

}

@Suppress("MemberVisibilityCanBePrivate", "unused", "SpellCheckingInspection")
object CharProperties {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing KDoc for public API

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


@ExternalSymbolName("org_jetbrains_skia_icu_Unicode_nCodePointHasBinaryProperty")
@ModuleImport("./skiko.mjs", "org_jetbrains_skia_icu_Unicode_nCodePointHasBinaryProperty")
private external fun nCodePointHasBinaryProperty(codePoint: Int, property: Int): Boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep prefixes consistent (one way or another)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@m-sasha m-sasha requested a review from MatkovIvan February 28, 2025 10:07
@MatkovIvan
Copy link
Member

@m-sasha you forgot to push the changes, right?

@m-sasha m-sasha force-pushed the m-sasha/expose-icu-u_hasBinaryProperty branch from fcddfc7 to 28f117d Compare February 28, 2025 10:50
@m-sasha
Copy link
Member Author

m-sasha commented Feb 28, 2025

@m-sasha you forgot to push the changes, right?

Yes, sorry, pushed now.

@m-sasha m-sasha merged commit 8620e19 into master Feb 28, 2025
6 checks passed
@m-sasha m-sasha deleted the m-sasha/expose-icu-u_hasBinaryProperty branch February 28, 2025 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants