Localisation Wrapper #663

jojolepro · Apr 24, 2018

Wraps a localisation context in a Asset wrapper.
Slightly inconvenient to use because we have to wait for the asset to be loaded, but I'm not sure how to go around that.

This change is

torkleyy · Apr 24, 2018

torkleyy reviewed Apr 24, 2018

View changes

I recommend you use Progress to wait for the asset and then place the locale context somewhere in the world.

Just realized I forget to rename things I copy pasted. Don't review now thanks ;)

jojolepro · Apr 26, 2018

Just realized I forget to rename things I copy pasted. Don't review now thanks ;)

Fixing now...

jojolepro · May 2, 2018

Fixing now...

Ready for review my dudes

jojolepro · May 2, 2018

Ready for review my dudes

Rhuagh · May 2, 2018

Rhuagh reviewed May 2, 2018

View changes

Rhuagh · May 2, 2018

examples/locale/main.rs

+
+impl State for Example {
+    fn on_start(&mut self, world: &mut World) {
+        world.add_resource(AssetStorage::<Locale>::new());


No longer needed, you add Processor<Locale> below, which will register this resource.

Ah, this is not fixed yet.

This should be using world.exec.

@torkleyy Its not useful in this case as ProgressCounter is made when batch loading multiple assets to prevent checking them one by one.

jojolepro · May 2, 2018

@torkleyy Its not useful in this case as ProgressCounter is made when batch loading multiple assets to prevent checking them one by one.

torkleyy · May 2, 2018

torkleyy reviewed May 2, 2018

View changes

torkleyy · May 2, 2018

examples/locale/main.rs

+        let store = world.read_resource::<AssetStorage<Locale>>();
+        // Check if the locale has been loaded.
+        // If you are doing this for multiple assets, you should be using `ProgressCounter`.
+        if let Some(locale) = store.get(&self.handle.clone().unwrap()) {


Why are you cloning the handle?

torkleyy · May 2, 2018

torkleyy reviewed May 2, 2018

View changes

torkleyy · May 2, 2018

examples/locale/main.rs

+fn main() {
+    let resources_directory = format!("{}/examples/assets", env!("CARGO_MANIFEST_DIR"));
+    let mut game = Application::build(resources_directory, Example::new())
+        .expect("Fatal error")


This should be using ? like the other examples.

Rhuagh · May 2, 2018

Rhuagh reviewed May 2, 2018

View changes

Rhuagh · May 2, 2018

amethyst_assets/src/asset.rs

@@ -3,6 +3,20 @@ use std::sync::Arc;
 use amethyst_core::specs::storage::UnprotectedStorage;
 #[cfg(feature = "profiler")]
 #[macro_use]
+#[cfg(feature = "profiler")]


torkleyy · May 3, 2018

torkleyy reviewed May 3, 2018

View changes

torkleyy · May 3, 2018

amethyst_assets/src/asset.rs

@@ -9,6 +9,14 @@ use amethyst_core::specs::storage::UnprotectedStorage;
 #[macro_use]
 #[cfg(feature = "profiler")]
 #[macro_use]
+#[cfg(feature = "profiler")]


I think one of these might be enough ;P

Fix the conflicts and fix the review comment above and this is ready for merge.

Rhuagh · May 7, 2018

Fix the conflicts and fix the review comment above and this is ready for merge.

#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
use thread_profiler::{register_thread_with_profiler, write_profile};

I'm rolling back the formatting changes.

jojolepro · May 7, 2018

#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
#[cfg(feature = "profiler")]
#[macro_use]
use thread_profiler::{register_thread_with_profiler, write_profile};

I'm rolling back the formatting changes.

Done

jojolepro · May 7, 2018

Done

torkleyy · May 7, 2018

torkleyy reviewed May 7, 2018

View changes

Thanks for the fixes and cleanup, I have some more comments.

torkleyy · May 7, 2018

examples/locale/main.rs

+        let store = world.read_resource::<AssetStorage<Locale>>();
+
+        // Check if the locale has been loaded.
+        // If you are doing this for multiple assets, you should be using `ProgressCounter`.


This example should show good practices, so please use the counter.

torkleyy · May 7, 2018

examples/locale/main.rs

+                    .context
+                    .get_message("hello")
+                    .and_then(|msg| locale.context.format(msg, None))
+                    .unwrap()


Should not use unwrap

Moxinilian · May 10, 2018

Moxinilian reviewed May 10, 2018

View changes

How does one conveniently load and use multiple languages with this architecture?

https://github.com/projectfluent/fluent-rs

jojolepro · May 10, 2018

https://github.com/projectfluent/fluent-rs

I've seen that already, and ironically enough I could not find anything about that.
Does that mean we have to create a Locale for every language? If so, that means we have to conditionally get the asset everywhere, right?

Moxinilian · May 10, 2018

I've seen that already, and ironically enough I could not find anything about that.
Does that mean we have to create a Locale for every language? If so, that means we have to conditionally get the asset everywhere, right?

Conflictomania, but apart from that ready I believe?

Rhuagh · May 12, 2018

Conflictomania, but apart from that ready I believe?

Should be

jojolepro · May 12, 2018

Should be

torkleyy · May 28, 2018

torkleyy approved these changes May 28, 2018

View changes

Please add another language and address the comment, then we can merge this.

torkleyy · May 28, 2018

examples/locale/main.rs

+
+impl State for Example {
+    fn on_start(&mut self, world: &mut World) {
+        world.add_resource(AssetStorage::<Locale>::new());


This should be using world.exec.

soon ™️

I know I say that often ;)

jojolepro · Jun 15, 2018

soon ™️

I know I say that often ;)

Just updating the branch, will try to finish this thing this weekend. I'm not sure what changed last month so I don't know how much changes I need to make yet.

jojolepro · Jun 15, 2018

Just updating the branch, will try to finish this thing this weekend. I'm not sure what changed last month so I don't know how much changes I need to make yet.

Xaeroxe · Jun 15, 2018

Xaeroxe reviewed Jun 15, 2018

View changes

Xaeroxe · Jun 15, 2018

Cargo.toml

@@ -152,12 +152,17 @@ name = "appendix_a"
 path = "examples/appendix_a/main.rs"
 
 [[example]]
+<<<<<<< HEAD


Woops, merge conflict merged

Rhuagh · Jun 20, 2018

Rhuagh approved these changes Jun 20, 2018

View changes

rustfmt pass 1 sec

jojolepro · Jun 20, 2018

rustfmt pass 1 sec

Moxinilian · Jun 20, 2018

Moxinilian approved these changes Jun 20, 2018

View changes

Ready for merge yay. :D
@torkleyy can re-review if he wants to.

jojolepro · Jun 20, 2018

Ready for merge yay. :D
@torkleyy can re-review if he wants to.

missing changelog entry

jojolepro · Jun 20, 2018

missing changelog entry

Ready for real now! (I totally forgot I had to update the changelog for a full week :P )

jojolepro · Jun 27, 2018

Ready for real now! (I totally forgot I had to update the changelog for a full week :P )

Xaeroxe · Jun 30, 2018

Xaeroxe approved these changes Jun 30, 2018

View changes

This looks great! Nicely done!

bors r+

Build succeeded

bors · Jun 30, 2018

Build succeeded

jojolepro added some commits Apr 23, 2018

Initial implementation of Asset for Locale

107a546

Added localisation via rust fluent as an Asset

Loading status checks…

004cee0

jojolepro added type: feature pri: low project: assets status: ready labels Apr 24, 2018

Removed comment that I forgot

Loading status checks…

f112d83

jojolepro added status: working and removed status: ready labels Apr 26, 2018

jojolepro added some commits May 2, 2018

Fixed doc, improved example clarity

cbbdd3d

Merge with develop

Loading status checks…

051d1d9

jojolepro added status: ready and removed status: working labels May 2, 2018

jojolepro added some commits May 2, 2018

Fixed newline

c937fe9

Added Result. Improved storage loaded check.

9a940d7

Merge branch 'develop' of https://github.com/amethyst/amethyst into l…
…ocale

5be455d

Formatted using rustfmt-preview v 0.3.8-nightly (), running on stable…
… rust

Loading status checks…

ea63e3c

Re-added ProgressCounter in locale example

e6fd3d8

Merge branch 'develop' into locale

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

3cb6c8d

Rhuagh added status: review comments and removed status: ready labels May 17, 2018

Merge branch 'develop' of https://github.com/amethyst/amethyst into l…
…ocale

Loading status checks…

37d170c

Fixed dependencies due to merge

Loading status checks…

e5af8ce

Added french example locale. Now using world.exec. Fixed missing data…
….update.

Loading status checks…

37996b6

rustfmt-stable pass

Loading status checks…

464af70

Updated changelog for amethyst locale

e463646

Merge branch 'develop' into locale

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

e775ea3

bors bot merged commit e775ea3 into amethyst:develop Jun 30, 2018
3 checks passed

3 checks passed

bors Build succeeded
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jojolepro deleted the jojolepro:locale branch Jun 30, 2018

amethyst/amethyst

Join GitHub today

Localisation Wrapper #663

Conversation

jojolepro commented Apr 24, 2018 • edited by torkleyy Edited 1 time torkleyy edited Apr 24, 2018 (most recent)

jojolepro added some commits Apr 23, 2018

jojolepro added type: feature pri: low project: assets status: ready labels Apr 24, 2018

torkleyy reviewed Apr 24, 2018 View changes

jojolepro added status: working and removed status: ready labels Apr 26, 2018

This comment has been minimized.

jojolepro commented Apr 26, 2018

This comment has been minimized.

jojolepro commented May 2, 2018

jojolepro added some commits May 2, 2018

This comment has been minimized.

jojolepro commented May 2, 2018

jojolepro added status: ready and removed status: working labels May 2, 2018

Rhuagh reviewed May 2, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jojolepro commented May 2, 2018

torkleyy reviewed May 2, 2018 View changes

This comment has been minimized.

torkleyy reviewed May 2, 2018 View changes

This comment has been minimized.

Rhuagh reviewed May 2, 2018 View changes

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed May 3, 2018 View changes

This comment has been minimized.

This comment has been minimized.

Rhuagh commented May 7, 2018

This comment has been minimized.

jojolepro commented May 7, 2018

jojolepro added some commits May 2, 2018

This comment has been minimized.

jojolepro commented May 7, 2018

torkleyy reviewed May 7, 2018 View changes

This comment has been minimized.

This comment has been minimized.

Moxinilian reviewed May 10, 2018 View changes

This comment has been minimized.

jojolepro commented May 10, 2018

This comment has been minimized.

Moxinilian commented May 10, 2018 • edited Edited 3 times Moxinilian edited May 10, 2018 (most recent) Moxinilian edited May 10, 2018 Moxinilian edited May 10, 2018

This comment has been minimized.

Rhuagh commented May 12, 2018

This comment has been minimized.

jojolepro commented May 12, 2018

Rhuagh added status: review comments and removed status: ready labels May 17, 2018

torkleyy approved these changes May 28, 2018 View changes

This comment has been minimized.

This comment has been minimized.

jojolepro commented Jun 15, 2018 • edited Edited 1 time jojolepro edited Jun 15, 2018 (most recent) jojolepro created Jun 15, 2018

This comment has been minimized.

jojolepro commented Jun 15, 2018

Xaeroxe reviewed Jun 15, 2018 View changes

This comment has been minimized.

Rhuagh approved these changes Jun 20, 2018 View changes

This comment has been minimized.

jojolepro commented Jun 20, 2018

Moxinilian approved these changes Jun 20, 2018 View changes

This comment has been minimized.

jojolepro commented Jun 20, 2018

This comment has been minimized.

jojolepro commented Jun 20, 2018

This comment has been minimized.

jojolepro commented Jun 27, 2018

Xaeroxe approved these changes Jun 30, 2018 View changes

bors bot added a commit that referenced this pull request Jun 30, 2018

This comment has been minimized.

bors bot Jun 30, 2018

Build succeeded

bors bot commented Jun 30, 2018

Build succeeded

Hide details View details bors bot merged commit e775ea3 into amethyst:develop Jun 30, 2018 3 checks passed

3 checks passed

jojolepro deleted the jojolepro:locale branch Jun 30, 2018

jojolepro commented Apr 24, 2018 •

edited by torkleyy

Edited 1 time

torkleyy edited Apr 24, 2018 (most recent)

torkleyy reviewed Apr 24, 2018

View changes

Rhuagh reviewed May 2, 2018

View changes

torkleyy reviewed May 2, 2018

View changes

torkleyy reviewed May 2, 2018

View changes

Rhuagh reviewed May 2, 2018

View changes

torkleyy reviewed May 3, 2018

View changes

torkleyy reviewed May 7, 2018

View changes

Moxinilian reviewed May 10, 2018

View changes

Moxinilian commented May 10, 2018 •

edited

Edited 3 times

Moxinilian edited May 10, 2018 (most recent)

Moxinilian edited May 10, 2018

Moxinilian edited May 10, 2018

torkleyy approved these changes May 28, 2018

View changes

jojolepro commented Jun 15, 2018 •

edited

Edited 1 time

jojolepro edited Jun 15, 2018 (most recent)

jojolepro created Jun 15, 2018

Xaeroxe reviewed Jun 15, 2018

View changes

Rhuagh approved these changes Jun 20, 2018

View changes

Moxinilian approved these changes Jun 20, 2018

View changes

Xaeroxe approved these changes Jun 30, 2018

View changes

bors bot merged commit `e775ea3` into amethyst:develop Jun 30, 2018
3 checks passed