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.

Sign up for GitHub

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

Jump to bottom

Disabling Isolation Marks in Fluent #46

Merged
merged 3 commits into from
Nov 24, 2020
Merged

Disabling Isolation Marks in Fluent #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18bc16e
Expose `set_use_isolating` to `FluentLanguageLoader`
fosskers Nov 22, 2020
d1aa034
Unit tests for bidirectional isolation
fosskers Nov 23, 2020
53fde0d
Mention correct order of method calls
fosskers Nov 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion i18n-embed/i18n/ftl/en-US/test.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ hello-world = Hello World Localization!
only-us = only US
only-ru = only RU
only-gb = only GB (US Version)
different-args = this message has different {$arg}s in different languages
different-args = this message has different {$arg}s in different languages
isolation-chars = inject a { $thing } here
16 changes: 16 additions & 0 deletions i18n-embed/src/fluent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,22 @@ impl FluentLanguageLoader {

(closure)(&mut iter)
}

/// Set whether the underlying Fluent logic should insert Unicode
/// Directionality Isolation Marks around placeables.
///
/// See [`fluent::bundle::FluentBundleBase::set_use_isolating`] for more
/// information.
///
/// **Note:** This function will have no effect if
/// [`LanguageLoader::load_languages`] has not been called first.
///
/// Default: `true`.
pub fn set_use_isolating(&self, value: bool) {
Copy link
Owner

@kellpossible kellpossible Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

perhaps we could add a unit/integration test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, perhaps it would sufficient to check the byte lengths of the output strings? I'll add a commit today.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess so, or to be more specific you could check if the string contains the expected special characters or not. One test with it enabled, and one without it enabled.

for bundle in self.language_config.write().language_bundles.as_mut_slice() {
Copy link
Contributor Author

@fosskers fosskers Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Rust surprised me here. I'm able to get mutable access to the a nested field several levels down, and yet we're borrowing the parent immutably with &self.

And a question with regard to write(): I see from its docs that it blocks the thread. Is my overall approach to getting down to the internal bundle a good one?

Copy link
Owner

Choose a reason for hiding this comment

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

language_config is wrapped in an RwLock which gives it safe interior mutability with the use of runtime checks. The downside in this case is that threads will block if they try to access it when someone holds the write lock to it, but I hoped that it would not be common to be making changes to the language_config during a performance critical piece of code, thus it would at least be better than using a Mutex. It also has the property that the write lock cannot be obtained until all the read locks have been dropped.

I guess we need to be careful not to introduce a deadlock if this method will be called within another method that already holds the lock, seems like that's not the case here so it looks good to me.

bundle.bundle.set_use_isolating(value);
}
}
}

impl LanguageLoader for FluentLanguageLoader {
Expand Down
27 changes: 27 additions & 0 deletions i18n-embed/tests/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,33 @@ mod fluent {
assert!(loader.has("only-us"));
assert!(!loader.has("non-existent-message"))
}

#[test]
fn bidirectional_isolation_off() {
setup();
let en_us: LanguageIdentifier = "en-US".parse().unwrap();
let loader = FluentLanguageLoader::new("test", en_us.clone());
loader.load_languages(&Localizations, &[&en_us]).unwrap();
loader.set_use_isolating(false);
let args = maplit::hashmap! {
"thing" => "thing"
};
let msg = loader.get_args("isolation-chars", args);
assert_eq!("inject a thing here", msg);
}

#[test]
fn bidirectional_isolation_on() {
setup();
let en_us: LanguageIdentifier = "en-US".parse().unwrap();
let loader = FluentLanguageLoader::new("test", en_us.clone());
loader.load_languages(&Localizations, &[&en_us]).unwrap();
let args = maplit::hashmap! {
"thing" => "thing"
};
let msg = loader.get_args("isolation-chars", args);
assert_eq!("inject a \u{2068}thing\u{2069} here", msg);
}
}

#[cfg(feature = "gettext-system")]
Expand Down