Bidi Data Adapter #1784

younies · 2022-04-07T23:13:55Z

Closes: #1784

Manishearth · 2022-04-07T23:31:32Z

components/properties/src/bc.rs

+use icu_codepointtrie::CodePointTrie;
+
+pub struct BidiClassAdaptor {
+	code_point_trie: CodePointTrie;


Suggested change

code_point_trie: CodePointTrie;

code_point_trie: CodePointTrie,

Manishearth · 2022-04-07T23:31:39Z

components/properties/src/bc.rs

+use icu::properties::{maps, BidiClass};
+use icu_codepointtrie::CodePointTrie;
+
+pub struct BidiClassAdaptor {


nit: Adapter

Manishearth · 2022-04-07T23:32:03Z

components/properties/src/bc.rs

+		     maps::get_bidi_class(&provider)
+		         .expect("The data should be valid");
+		 let data_struct = payload.get();
+		 BidiClassAdaptor{ data_struct.code_point_trie }


Suggested change

BidiClassAdaptor{ data_struct.code_point_trie }

BidiClassAdaptor{ code_point_trie: data_struct.code_point_trie }

For now, I added a reference to the provider. However, I would like to have a way to save a reference to the code_point_trie instead. Shall I pass a code_point_trie?

I have added it.

sffc · 2022-04-07T23:37:05Z

components/properties/Cargo.toml

@@ -38,6 +38,7 @@ icu_provider = { version = "0.5", path = "../../provider/core", features = ["mac
 icu_uniset = { version = "0.4.1", path = "../../utils/uniset"}
 serde = { version = "1.0", default-features = false, features = ["derive", "alloc"], optional = true }
 zerovec = { version = "0.6", path = "../../utils/zerovec", features = ["derive"] }
+unicode-bidi = "0.3.7"


Issue: This should be an optional dependency. Enabling this dependency should enable the code that uses it.

zbraniecki · 2022-04-08T00:45:42Z

components/properties/src/bc.rs

@@ -0,0 +1,71 @@
+// This file is part of ICU4X. For terms of use, please see the file


nit: please, name this file src/bidi.rs - bc becomes very cryptic for readers.

zbraniecki · 2022-04-08T00:48:44Z

components/properties/src/bc.rs

+// called LICENSE at the top level of the ICU4X source tree
+// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).
+
+use crate::BidiDataSource;


question: where is BidiDataSource defined? Why do you need a trait?

I think it comes from the unicode-bidi crate. The import needs to be fixed.

yes, it comes from unicode-bidi and I have fixed the import.

@zbraniecki , we need to implement to plugin the data from ICU4X to unicode-bidi.

younies · 2022-04-21T04:58:57Z

components/properties/src/bidi.rs

+            BidiClass::LeftToRightIsolate => DataSourceBidiClass::LRI,
+            BidiClass::RightToLeftIsolate => DataSourceBidiClass::RLI,
+            BidiClass::PopDirectionalIsolate => DataSourceBidiClass::PDI,
+            _ => {


what shall we do here? unfortunately, BidiClass is not an enum?

@sffc , @Manishearth

It should return the neutral/undefined/default value.

Running this code is rare, but it can happen if old code runs with new data.

Okay, I have returned Other_Neutral

components/properties/src/bidi.rs

sffc · 2022-04-21T08:00:11Z

components/properties/src/lib.rs

@@ -92,3 +93,5 @@ pub use props::{
    BidiClass, CanonicalCombiningClass, EastAsianWidth, EnumeratedProperty, GeneralCategory,
    GeneralCategoryGroup, GraphemeClusterBreak, LineBreak, Script, SentenceBreak, WordBreak,
 };
+
+pub use bidi::BidiClassAdapter;


Nit: Don't hoist this to the top. Leave it exported under the bidi module.

I did not get this, could you illustrate more?

Should clients write

use icu_properties::BidiClassAdapter;

or

use icu_properties::bidi::BidiClassAdapter;

I think we should lean toward putting this into a child namespace rather than the top level, but I can be convinced otherwise.

CC @Manishearth @echeran

I think child module makes sense

components/properties/src/bidi.rs

jira-pull-request-webhook · 2022-04-21T13:56:19Z

Notice: the branch changed across the force-push!

Cargo.lock is different
components/properties/Cargo.toml is different
components/properties/src/lib.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

components/properties/src/lib.rs

sffc · 2022-04-21T15:49:29Z

components/properties/Cargo.toml

@@ -52,3 +54,4 @@ std = ["icu_provider/std"]
 default = []
 serde = ["dep:serde", "zerovec/serde", "icu_uniset/serde", "icu_codepointtrie/serde"]
 datagen = ["serde", "zerovec/serde_serialize", "icu_uniset/serde_serialize", "icu_codepointtrie/serde_serialize"]
+unicode-bidi = [ "dep:unicode-bidi" ]


Suggestion: name the feature just "bidi"

Manishearth · 2022-04-26T12:27:50Z

components/properties/Cargo.toml

 icu_uniset = { version = "0.4.1", path = "../../utils/uniset"}
 serde = { version = "1.0", default-features = false, features = ["derive", "alloc"], optional = true }
 zerovec = { version = "0.6", path = "../../utils/zerovec", features = ["derive"] }
+unicode-bidi = { git = "https://github.com/servo/unicode-bidi", optional = true , deafault-features = false }


i've made servo/unicode-bidi#78 to get this published, since we have a working PR now and are close to landing it

Okay, I am waiting to merge it.

components/properties/src/bidi.rs

Co-authored-by: Shane F. Carr <[email protected]>

components/properties/src/bidi.rs

Co-authored-by: Manish Goregaokar <[email protected]>

sffc · 2022-04-27T11:40:15Z

components/properties/Cargo.toml


 [dev-dependencies]
 icu = { path = "../icu", default-features = false }
 icu_testdata = { version = "0.5", path = "../../provider/testdata" }
+icu_provider_fs = {version = "0.5.0", path = "../../provider/fs" }


Nit: I do not think you are using this dependency, so you should remove it

younies requested a review from a team as a code owner April 7, 2022 23:13

younies changed the title ~~Bidi~~ Bidi Data Adopter Apr 7, 2022

younies marked this pull request as draft April 7, 2022 23:14

Manishearth reviewed Apr 7, 2022

View reviewed changes

sffc requested changes Apr 7, 2022

View reviewed changes

zbraniecki reviewed Apr 8, 2022

View reviewed changes

younies marked this pull request as ready for review April 21, 2022 03:46

younies requested review from zbraniecki, sffc and Manishearth April 21, 2022 04:57

younies commented Apr 21, 2022

View reviewed changes

components/properties/src/bidi.rs Show resolved Hide resolved

sffc requested changes Apr 21, 2022

View reviewed changes

younies requested a review from sffc April 21, 2022 13:44

younies and others added 10 commits April 21, 2022 15:47

Implement DataSource

12ca497

add tests

9c9ff4c

make unicode bidi crate optional

c062f8e

Fix comments

b93dc24

Finish the BidiDataSource trait implementation.

9a0fee7

make unicode-bidi optional

883e9ad

fix most of the issues.

685e705

add unicode-bidi to features.

fefa84a

allow dead code.

e239f5f

return other neutral as a default case.

f636b12

younies force-pushed the bidi_data_helper branch from 520bda6 to f636b12 Compare April 21, 2022 13:56

sffc changed the title ~~Bidi Data Adopter~~ Bidi Data Adapter Apr 21, 2022

sffc reviewed Apr 21, 2022

View reviewed changes

add docs and fix comments.

cb708ae

younies requested a review from sffc April 25, 2022 13:15

younies and others added 3 commits April 26, 2022 00:07

add no_std

301dd4b

std = false

156ac37

default = false

2bd29b4

Manishearth reviewed Apr 26, 2022

View reviewed changes

fix default.

6c30b8a

younies mentioned this pull request Apr 26, 2022

Bump to 0.3.8 servo/unicode-bidi#78

Merged

younies added 2 commits April 26, 2022 16:43

fix

0c4af58

fix bidi version

f2f092e

younies requested a review from Manishearth April 26, 2022 15:44

younies closed this Apr 26, 2022

younies reopened this Apr 26, 2022

sffc reviewed Apr 26, 2022

View reviewed changes

components/properties/src/bidi.rs Outdated Show resolved Hide resolved

components/properties/src/bidi.rs Show resolved Hide resolved

younies and others added 2 commits April 26, 2022 18:38

Update components/properties/src/bidi.rs

b1694d9

Co-authored-by: Shane F. Carr <[email protected]>

update cargo.lock

10f5e1e

younies requested a review from sffc April 26, 2022 18:22

Manishearth previously approved these changes Apr 27, 2022

View reviewed changes

components/properties/src/bidi.rs Outdated Show resolved Hide resolved

Update components/properties/src/bidi.rs

62b3f27

Co-authored-by: Manish Goregaokar <[email protected]>

younies dismissed Manishearth’s stale review via 62b3f27 April 27, 2022 10:40

younies requested a review from Manishearth April 27, 2022 10:41

sffc reviewed Apr 27, 2022

View reviewed changes

remove not needed dependencies

8054628

younies requested a review from sffc April 27, 2022 11:52

sffc approved these changes Apr 27, 2022

View reviewed changes

younies merged commit f6a56a1 into unicode-org:main Apr 27, 2022

younies deleted the bidi_data_helper branch April 27, 2022 13:38

younies mentioned this pull request May 24, 2022

Bidi - Add a function to port the Bidi-class data to Unicode Bidi #1734

Closed

hsivonen mentioned this pull request May 25, 2022

Use a higher numeric value for Strength::Identical #1942

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bidi Data Adapter #1784

Bidi Data Adapter #1784

younies commented Apr 7, 2022 •

edited

Loading

Manishearth Apr 7, 2022

younies Apr 21, 2022

Manishearth Apr 7, 2022

younies Apr 21, 2022

Manishearth Apr 7, 2022

younies Apr 21, 2022

younies Apr 21, 2022

sffc Apr 7, 2022

younies Apr 21, 2022

zbraniecki Apr 8, 2022

younies Apr 21, 2022

zbraniecki Apr 8, 2022

sffc Apr 8, 2022

younies Apr 21, 2022

younies Apr 21, 2022

sffc Apr 21, 2022

younies Apr 21, 2022

sffc Apr 21, 2022

younies Apr 21, 2022

sffc Apr 21, 2022

Manishearth Apr 21, 2022

younies Apr 25, 2022

jira-pull-request-webhook bot commented Apr 21, 2022

sffc Apr 21, 2022

younies Apr 25, 2022

Manishearth Apr 26, 2022

younies Apr 26, 2022

younies Apr 26, 2022

sffc Apr 27, 2022

younies Apr 27, 2022

	code_point_trie: CodePointTrie;
	code_point_trie: CodePointTrie,

	BidiClassAdaptor{ data_struct.code_point_trie }
	BidiClassAdaptor{ code_point_trie: data_struct.code_point_trie }

		@@ -0,0 +1,71 @@
		// This file is part of ICU4X. For terms of use, please see the file

Bidi Data Adapter #1784

Bidi Data Adapter #1784

Conversation

younies commented Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younies commented Apr 7, 2022 •

edited

Loading