-
-
Notifications
You must be signed in to change notification settings - Fork 1
Added flutter rust bridge impelementation as the dart_icu4x pacage #4
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
base: main
Are you sure you want to change the base?
Added flutter rust bridge impelementation as the dart_icu4x pacage #4
Conversation
# installed. You should not increase this version, as doing so will cause | ||
# the plugin to fail to compile for some customers of the plugin. | ||
cmake_minimum_required(VERSION 3.10) | ||
|
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.
Suggestion, here and elsewhere: seems like this file isn't used right now. I think we should only add files that are being used by the main project.
/gemini review |
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.
Code Review
This pull request adds the initial implementation of the dart_icu4x
package, including the Rust backend, Dart/Flutter frontend, and an example application. The implementation provides functionality to query Unicode character properties and perform case mapping. My review focuses on improving correctness, performance, and maintainability. I've identified several areas for improvement, including fixing placeholder values in configuration files, correcting flawed logic in the search functionality, and addressing performance bottlenecks in the Rust implementation. There are also some critical items like adding a license file and updating placeholder metadata before this package can be considered for publishing.
packages/dart_icu4x/LICENSE
Outdated
@@ -0,0 +1 @@ | |||
TODO: Add your license here. |
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.
packages/dart_icu4x/Makefile
Outdated
clean: ## Cleans Flutter project. | ||
flutter clean | ||
flutter pub get | ||
cargo build --manifest-path rust/Cargo.toml | ||
flutter_rust_bridge_codegen generate |
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.
The clean
target is misnamed. It performs a full build cycle (clean, fetch dependencies, build, generate code) rather than just cleaning build artifacts. This can be confusing and lead to unexpected behavior. It's better to separate cleaning from building.
For example:
clean: ## Cleans the project build artifacts.
flutter clean
cargo clean --manifest-path rust/Cargo.toml
setup: ## Installs dependencies and generates code.
flutter pub get
make cargo-build
make generate
fresh: ## Runs `clean` and `setup` for a fresh setup.
make clean
make setup
s.summary = 'A new Flutter FFI plugin project.' | ||
s.description = <<-DESC | ||
A new Flutter FFI plugin project. | ||
DESC | ||
s.homepage = 'http://example.com' | ||
s.license = { :file => '../LICENSE' } | ||
s.author = { 'Your Company' => '[email protected]' } |
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.
let general_category_map = CodePointMapData::<GeneralCategory>::new(); | ||
let script_map = CodePointMapData::<Script>::new(); | ||
let bidi_class_map = CodePointMapData::<BidiClass>::new(); | ||
let east_asian_width_map = CodePointMapData::<EastAsianWidth>::new(); | ||
let line_break_map = CodePointMapData::<LineBreak>::new(); | ||
let word_break_map = CodePointMapData::<WordBreak>::new(); | ||
let sentence_break_map = CodePointMapData::<SentenceBreak>::new(); | ||
let grapheme_cluster_break_map = CodePointMapData::<GraphemeClusterBreak>::new(); | ||
let hangul_syllable_type_map = CodePointMapData::<HangulSyllableType>::new(); | ||
let joining_type_map = CodePointMapData::<JoiningType>::new(); | ||
let alphabetic = CodePointSetData::new::<Alphabetic>(); | ||
let uppercase = CodePointSetData::new::<Uppercase>(); | ||
let lowercase = CodePointSetData::new::<Lowercase>(); | ||
let white_space = CodePointSetData::new::<WhiteSpace>(); | ||
let math = CodePointSetData::new::<Math>(); | ||
let dash = CodePointSetData::new::<Dash>(); | ||
let diacritic = CodePointSetData::new::<Diacritic>(); | ||
let emoji = CodePointSetData::new::<Emoji>(); | ||
let emoji_presentation = CodePointSetData::new::<EmojiPresentation>(); | ||
let emoji_modifier = CodePointSetData::new::<EmojiModifier>(); | ||
let emoji_modifier_base = CodePointSetData::new::<EmojiModifierBase>(); |
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.
The ICU data providers (CodePointMapData
, CodePointSetData
, etc.) are initialized on every call to get_unicode_char_properties
. This is inefficient and can cause significant performance degradation, especially since this is a synchronous FFI call. These providers should be initialized only once and reused across calls. You can use a crate like once_cell
or lazy_static
to ensure they are created lazily and stored in a static variable. The same issue exists for CaseMapper
in get_character_case_mapping
.
if let Some(ref s) = search { | ||
if !s.is_empty() { | ||
// If search is a single character, search for that specific character | ||
let mut chars = s.chars(); | ||
if let Some(c) = chars.next() { | ||
if chars.next().is_none() { | ||
results.push(c); | ||
} else { | ||
// Search is multiple characters, filter across all fields | ||
results = CodePointInversionList::all() | ||
.iter_chars() | ||
.filter(|&c| { | ||
let char_str = c.to_string(); | ||
let code_point_str = (c as u32).to_string(); | ||
let unicode_value = format!("U+{:04X}", c as u32); | ||
let general_category = format!("{:?}", general_category_map.get(c)); | ||
|
||
s.to_lowercase().contains(&char_str.to_lowercase()) || | ||
code_point_str.contains(s) || | ||
unicode_value.to_lowercase().contains(&s.to_lowercase()) || | ||
general_category.to_lowercase().contains(&s.to_lowercase()) | ||
}) | ||
.collect(); |
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.
The search logic in get_unicode_char_properties
has some issues that affect correctness and user experience:
- Single-character search: When the search term is a single character, the result set is just that character. The pagination (
skip
/take
) is then applied to this single-element list. This means ifoffset > 0
, the function will incorrectly return an empty list. - Multi-character search: The condition
s.to_lowercase().contains(&char_str.to_lowercase())
seems to be reversed. It checks if the search term contains the character, which is probably not what's intended. You likely want to search if the character's name contains the search term. - The search does not include the character's name, which is a primary way users might search for a character.
packages/dart_icu4x/README.md
Outdated
dart_icu4x: | ||
path: ../icu4x/dart_icu4x |
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.
The installation instructions use a local path dependency which is great for contributors. For users of the package, it would be helpful to also show how to add it as a versioned dependency from pub.dev, once it's published. Also, the path ../icu4x/dart_icu4x
seems specific to a particular monorepo structure and might be confusing. The example app uses path: ../
, which is more standard for an example within a package.
dart_icu4x: | |
path: ../icu4x/dart_icu4x | |
dart_icu4x: ^0.0.1 # Or the latest version from pub.dev | |
# For contributors, use a path dependency: | |
# dependencies: | |
# dart_icu4x: | |
# path: ../ |
packages/dart_icu4x/pubspec.yaml
Outdated
@@ -0,0 +1,32 @@ | |||
name: dart_icu4x | |||
description: "A new Flutter FFI plugin project." |
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.
|
||
android { | ||
if (project.android.hasProperty("namespace")) { | ||
namespace 'com.flutter_rust_bridge.dart_icu4x' |
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.
@@ -0,0 +1,3 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
package="com.flutter_rust_bridge.dart_icu4x"> |
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.
test('Can call rust function', () async { | ||
//expect(greet(name: "Tom"), "Hello, Tom!"); |
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.
The integration test contains a commented-out example from a template. Please replace it with a meaningful test for the dart_icu4x
plugin's functionality, for example, by testing getUnicodeCharProperties
or getCharacterCaseMapping
.
test('getUnicodeCharProperties returns properties', () async {
final properties = getUnicodeCharProperties(limit: BigInt.from(1));
expect(properties, isNotEmpty);
expect(properties[0].character, isNotNull);
});
b0cb808
to
7420fd3
Compare
/gemini review |
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.
Code Review
This pull request introduces the dart_icu4x
package, a Flutter plugin using flutter_rust_bridge
to provide ICU4X functionality. The changes are extensive, including the Rust backend, Dart frontend, an example application, and all necessary configuration files for Android and iOS. The documentation is comprehensive and well-written. My review focuses on improving code quality, consistency, and performance. I've identified several areas for improvement, particularly regarding performance in the Rust API by using once_cell::sync::Lazy
for expensive initializations, and ensuring consistency in package naming within the Android build configuration. There are also several minor formatting and style issues that should be addressed.
@@ -0,0 +1,56 @@ | |||
// The Android Gradle Plugin builds the native code with the Android NDK. | |||
|
|||
group 'com.flutter_rust_bridge.dart_icu4x' |
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.
let general_category_map = CodePointMapData::<GeneralCategory>::new(); | ||
let script_map = CodePointMapData::<Script>::new(); | ||
let bidi_class_map = CodePointMapData::<BidiClass>::new(); | ||
let east_asian_width_map = CodePointMapData::<EastAsianWidth>::new(); | ||
let line_break_map = CodePointMapData::<LineBreak>::new(); | ||
let word_break_map = CodePointMapData::<WordBreak>::new(); | ||
let sentence_break_map = CodePointMapData::<SentenceBreak>::new(); | ||
let grapheme_cluster_break_map = CodePointMapData::<GraphemeClusterBreak>::new(); | ||
let hangul_syllable_type_map = CodePointMapData::<HangulSyllableType>::new(); | ||
let joining_type_map = CodePointMapData::<JoiningType>::new(); | ||
let alphabetic = CodePointSetData::new::<Alphabetic>(); | ||
let uppercase = CodePointSetData::new::<Uppercase>(); | ||
let lowercase = CodePointSetData::new::<Lowercase>(); | ||
let white_space = CodePointSetData::new::<WhiteSpace>(); | ||
let math = CodePointSetData::new::<Math>(); | ||
let dash = CodePointSetData::new::<Dash>(); | ||
let diacritic = CodePointSetData::new::<Diacritic>(); | ||
let emoji = CodePointSetData::new::<Emoji>(); | ||
let emoji_presentation = CodePointSetData::new::<EmojiPresentation>(); | ||
let emoji_modifier = CodePointSetData::new::<EmojiModifier>(); | ||
let emoji_modifier_base = CodePointSetData::new::<EmojiModifierBase>(); |
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.
These CodePointMapData
and CodePointSetData
instances are initialized on every call to get_unicode_char_properties
. This is highly inefficient as it involves reading and parsing ICU data repeatedly. To improve performance, these should be initialized only once using once_cell::sync::Lazy
and stored in static variables at the module level.
For example:
static GENERAL_CATEGORY_MAP: Lazy<CodePointMapData<GeneralCategory>> = Lazy::new(CodePointMapData::<GeneralCategory>::new);
Then, inside the function, you can use &*GENERAL_CATEGORY_MAP
.
|
||
#[flutter_rust_bridge::frb(sync)] | ||
pub fn get_character_case_mapping(character: String) -> CaseMappingResult { | ||
let casemapper = CaseMapper::new(); |
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.
The CaseMapper
is created on every call. This is inefficient. It should be initialized once using once_cell::sync::Lazy
and stored in a static variable to improve performance.
Define this at the module level:
static CASE_MAPPER: Lazy<CaseMapper> = Lazy::new(CaseMapper::new);
Then use it in the function as suggested.
let casemapper = CaseMapper::new(); | |
let casemapper = &*CASE_MAPPER; |
packages/dart_icu4x/Makefile
Outdated
make setup | ||
|
||
cargo-build: ## Runs `cargo build --manifest-path rust/Cargo.toml`. | ||
cargo build --manifest-path rust/Cargo.toml |
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.
packages/dart_icu4x/Makefile
Outdated
|
||
test: ## Runs unit tests. | ||
flutter test | ||
|
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.
@@ -0,0 +1,3 @@ | |||
rust_input: crate::api | |||
rust_root: rust/ | |||
dart_output: lib/src/rust |
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.
DESC | ||
s.homepage = 'https://github.com/unicode-org/gsoc-unicode-app' | ||
s.license = { :file => '../LICENSE' } | ||
s.author = { 'Your Company' => 'https://icu4x.unicode.org/' } |
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.
'EXCLUDED_ARCHS[sdk=iphonesimulator*]' => 'i386', | ||
'OTHER_LDFLAGS' => '-force_load ${BUILT_PRODUCTS_DIR}/libdart_icu4x.a', | ||
} | ||
end |
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.
packages/dart_icu4x/pubspec.yaml
Outdated
android: | ||
ffiPlugin: true | ||
ios: | ||
ffiPlugin: true |
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.
#[flutter_rust_bridge::frb(sync)] | ||
pub fn get_character_case_mapping(character: String) -> CaseMappingResult { | ||
let casemapper = CaseMapper::new(); | ||
let langid: LanguageIdentifier = "und".parse().unwrap(); |
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.
Using unwrap()
can cause the program to panic if the Result
is an Err
. While "und"
is a valid language identifier, it's better practice in library code to handle potential errors more gracefully. Consider using expect()
with a descriptive message to provide more context on panic.
let langid: LanguageIdentifier = "und".parse().unwrap(); | |
let langid: LanguageIdentifier = "und".parse().expect("Failed to parse 'und' as a language identifier"); |
/gemini review |
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.
Code Review
This pull request introduces the dart_icu4x
package, a Flutter plugin that provides ICU4X functionality through Rust bindings. The overall structure is well-organized, including an example application and integration tests. However, there are several areas that need improvement. The Rust implementation has significant performance issues due to the repeated initialization of heavy ICU4X data structures within API functions. The Android build configurations use non-standard and future SDK versions, which could cause build failures for users. Additionally, there are minor inconsistencies and typos in various configuration files. Addressing these points will greatly improve the package's stability, performance, and usability.
distributionPath=wrapper/dists | ||
zipStoreBase=GRADLE_USER_HOME | ||
zipStorePath=wrapper/dists | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-8.12-all.zip |
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.
icu = "2.0.0" | ||
icu_casemap = "2.0.0" | ||
icu_collections = "2.0.0" | ||
icu_properties = "2.0.1" | ||
icu_provider = "2.0.0" | ||
icu_locale_core = "2.0.0" | ||
icu_locid = "1.5.0" |
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.
pub fn get_character_case_mapping(character: String) -> CaseMappingResult { | ||
let casemapper = CaseMapper::new(); | ||
let langid: LanguageIdentifier = "und".parse().expect("Failed to parse 'und' as a language identifier"); | ||
|
||
let mut chars = character.chars(); | ||
let c = match chars.next() { | ||
Some(ch) if chars.next().is_none() => ch, | ||
_ => return CaseMappingResult { | ||
original: character.clone(), | ||
mapped: character, | ||
has_mapping: false, | ||
}, | ||
}; | ||
|
||
let original = c.to_string(); | ||
let upper = casemapper.uppercase_to_string(&original, &langid).into_owned(); | ||
let lower = casemapper.lowercase_to_string(&original, &langid).into_owned(); | ||
|
||
// Check if the character has a case mapping (upper or lower is different from original) | ||
let has_mapping = upper != original || lower != original; | ||
|
||
// Return the appropriate mapping (prefer uppercase if different, otherwise lowercase) | ||
let mapped = if upper != original { | ||
upper | ||
} else if lower != original { | ||
lower | ||
} else { | ||
original.clone() | ||
}; | ||
|
||
CaseMappingResult { | ||
original, | ||
mapped, | ||
has_mapping, | ||
} | ||
} |
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.
pub fn get_all_scripts() -> Vec<String> { | ||
let script_map = CodePointMapData::<Script>::new(); | ||
let mut names: BTreeSet<String> = BTreeSet::new(); | ||
|
||
for c in CodePointInversionList::all().iter_chars() { | ||
let s = script_map.get(c); | ||
names.insert(s.long_name().to_string()); | ||
} | ||
|
||
names.into_iter().collect() | ||
} |
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.
let general_category_map = CodePointMapData::<GeneralCategory>::new(); | ||
let script_map = CodePointMapData::<Script>::new(); | ||
let bidi_class_map = CodePointMapData::<BidiClass>::new(); | ||
let east_asian_width_map = CodePointMapData::<EastAsianWidth>::new(); | ||
let line_break_map = CodePointMapData::<LineBreak>::new(); | ||
let word_break_map = CodePointMapData::<WordBreak>::new(); | ||
let sentence_break_map = CodePointMapData::<SentenceBreak>::new(); | ||
let grapheme_cluster_break_map = CodePointMapData::<GraphemeClusterBreak>::new(); | ||
let hangul_syllable_type_map = CodePointMapData::<HangulSyllableType>::new(); | ||
let joining_type_map = CodePointMapData::<JoiningType>::new(); | ||
let alphabetic = CodePointSetData::new::<Alphabetic>(); | ||
let uppercase = CodePointSetData::new::<Uppercase>(); | ||
let lowercase = CodePointSetData::new::<Lowercase>(); | ||
let white_space = CodePointSetData::new::<WhiteSpace>(); | ||
let math = CodePointSetData::new::<Math>(); | ||
let dash = CodePointSetData::new::<Dash>(); | ||
let diacritic = CodePointSetData::new::<Diacritic>(); | ||
let emoji = CodePointSetData::new::<Emoji>(); | ||
let emoji_presentation = CodePointSetData::new::<EmojiPresentation>(); | ||
let emoji_modifier = CodePointSetData::new::<EmojiModifier>(); | ||
let emoji_modifier_base = CodePointSetData::new::<EmojiModifierBase>(); |
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.
This function initializes numerous large CodePointMapData
and CodePointSetData
objects on every call, which is highly inefficient and will negatively impact performance. These objects should be initialized only once and reused. You can achieve this by using once_cell::sync::Lazy
.
For example:
use once_cell::sync::Lazy;
static GENERAL_CATEGORY_MAP: Lazy<CodePointMapData<GeneralCategory>> = Lazy::new(CodePointMapData::<GeneralCategory>::new);
// Then, in your function, you can use it like this:
let general_category = format!("{:?}", GENERAL_CATEGORY_MAP.get(c));
This optimization should be applied to all ICU4X data providers within this function.
s.source = { :path => '.' } | ||
s.source_files = 'Classes/**/*' | ||
s.dependency 'Flutter' | ||
s.platform = :ios, '11.0' |
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.
DESC | ||
s.homepage = 'https://github.com/unicode-org/gsoc-unicode-app' | ||
s.license = { :file => '../LICENSE' } | ||
s.author = { 'YThe Unicode Consortium' => 'https://icu4x.unicode.org/' } |
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.
@@ -0,0 +1,3 @@ | |||
## 0.0.1 | |||
|
|||
* TODO: Describe initial release. |
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.
Please describe the initial release instead of leaving a TODO
. This is important for users to understand the initial set of features and the scope of the version.
* TODO: Describe initial release. | |
* Initial release of the `dart_icu4x` package. | |
* Provides access to Unicode character properties from ICU4X. | |
* Includes case mapping and basic search functionality. |
|
||
### Platform Support | ||
- **Android**: Native ARM64 and x86_64 support | ||
- **iOS**: Native ARM64 and x86_64 support |
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.
dart format . | ||
|
||
test: ## Runs unit tests. | ||
flutter test |
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.
🎯 Overview
This PR significantly improves the dart_icu4x project documentation by replacing the generic Flutter FFI template README with a comprehensive, project-specific guide that accurately describes the ICU4X Flutter plugin functionality.
✨ Key Improvements
✅ Benefits
This PR transforms the dart_icu4x project from having generic template documentation to having comprehensive, project-specific documentation that accurately reflects its purpose as a Unicode and internationalization Flutter plugin powered by ICU4X.