feat: added crate to manage .lotties #23

samuelOsborne · 2024-01-09T10:17:23Z

No description provided.

changeset-bot · 2024-01-09T10:17:27Z

⚠️ No Changeset found

Latest commit: e07a70e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

theashraf

The demo player is no longer building.

I'm getting emcc link error with make all

theashraf · 2024-01-18T03:08:19Z

dotlottie-fms/src/dolottie_manager.rs

+                    } else if let Ok(animations) = manifest.animations.read() {
+                        id = animations.index(0).id.clone();
+                    } else {
+                        panic!("No animations found in dotLottie file");


It's generally better to return an error instead of panicking as panics are not recoverable

theashraf · 2024-01-18T03:09:39Z

dotlottie-fms/src/dolottie_manager.rs

+                    DotLottieManager {
+                        current_animation_id: String::new(),
+                        manifest: Manifest::new(),
+                        zip_data: vec![],
+                        animation_settings_cache: HashMap::new(),
+                        animation_data_cache: HashMap::new(),
+                    }


if the new returns a Result, we can just propagate the error here.

theashraf · 2024-01-18T03:11:46Z

dotlottie-fms/src/dolottie_manager.rs

+                } else if let Ok(animations) = manifest.animations.read() {
+                    id = animations.index(0).id.clone();
+                } else {
+                    panic!("No animations found in dotLottie file");


same as previous comment

theashraf · 2024-01-18T03:11:59Z

dotlottie-fms/src/dolottie_manager.rs

+                self.zip_data = dotlottie;
+            }
+            Err(error) => {
+                eprintln!("Unable to initialize dotLottie manager: {}", error);


same as previous comment

theashraf · 2024-01-18T03:19:44Z

dotlottie-fms/src/dolottie_manager.rs

+    pub fn get_playback_settings(
+        &mut self,
+        animation_id: &str,
+    ) -> Result<ManifestAnimation, DotLottieError> {


from the implementation, i notice that the animation_id may not exists in the map and that would return an Error, while we should return a more appropriate value to the consumer.

Suggested change

) -> Result<ManifestAnimation, DotLottieError> {

) -> Result<Option<ManifestAnimation>, DotLottieError> {

@theashraf if the animation_id isnt present we return an error, otherwise it will be there so we return a manifestAnimation, dont see why it needs to be an option in that case?

@samuelOsborne currently it doesn't return a manifestAnimation not found error, instead it returns a MutexLockError

Changed it to return AnimationNotFound if its not there

theashraf · 2024-01-18T03:55:48Z

dotlottie-rs/src/dotlottie_player.rs

+                return self.load_animation_data(&animation_data, width, height);
+            }
+            Err(error) => {
+                eprintln!("Error: {:?}", error);


theashraf · 2024-01-18T03:56:09Z

dotlottie-rs/src/dotlottie_player.rs

+                return self.load_animation_data(&animation_data, width, height);
+            }
+            Err(error) => {
+                eprintln!("Error: {:?}", error);


i don't think we should have any instances of println in our lib, we should either rely on Result or Option or ignore the error if it's not going to affect the flow

I'll add bool returns so the frameworks can detect errors

things like setting values to the config doesnt need to return Result

@samuelOsborne if we want the frameworks to detect errors then we have to change our exposed our API exposed to contain Result<T, Error>. i think that's the best approach, need to experiment how uniffi udl would look like and check if it's supported in the uniffi-cpp bindgen.

so this way all we need to do is just to propagate the errors up to the consumers.

@theashraf As this will require refactoring the other load functions, should we tackle in a separate PR?

theashraf · 2024-01-18T03:59:14Z

dotlottie-rs/src/dotlottie_player.rs

+    pub fn manifest(&self) -> Manifest {
+        self.dotlottie_manager.manifest()
+    }


if a lottie json was loaded, the Manifest shouldn't exist, i think we should return an Option here

theashraf · 2024-01-18T03:59:40Z

dotlottie-rs/src/dotlottie_player.rs

+                self.config.loop_animation = loop_animation;
+            }
+            Err(error) => {
+                eprintln!("Error: {:?}", error);


theashraf · 2024-01-18T04:07:27Z

dotlottie-fms/src/dolottie_manager.rs

+    zip_data: Vec<u8>,
+    animation_settings_cache: HashMap<String, ManifestAnimation>,
+    animation_data_cache: HashMap<String, String>,
+}


I think both hash maps would hold the same keys, maybe we can combine both together in a single HashMap, where the value is a tuple ?

animations_map: HashMap<String, (ManifestAnimation, String)>

what would the String in the tuple contain?

(ManifestAnimation, String)

@samuelOsborne it would contain the what this HashMap holds as a value animation_data_cache: HashMap<String, String>

@theashraf Im going to create a ticket for this refactoring as at the moment having a single map containing both creates an endless loop inserting in to the map as both functions (getting playback setting, animation_data) will depend on each other

get an animationData -> needs to get the ManifestAnimation -> needs to get animation
get ManifestAnimation -> needs to get animationData -> needs to get ManifestAnimation

samuelOsborne force-pushed the feat/dotlottie-support branch 2 times, most recently from d73b9c4 to 6887648 Compare January 16, 2024 08:42

samuelOsborne marked this pull request as ready for review January 17, 2024 13:07

samuelOsborne added 5 commits January 17, 2024 14:07

feat: added crate to manage .lotties

9bea5a1

feat: added dotlottie_manager

8511f93

wip: multi animation support

76f5dc8

chore: debug for ashraf

54e3891

chore: clean up

7ab06c0

samuelOsborne force-pushed the feat/dotlottie-support branch from b59e8af to 7ab06c0 Compare January 17, 2024 13:10

chore: removed log

9f3afcb

samuelOsborne requested review from theashraf and afsalz January 17, 2024 13:15

feat: added playback setting loading

e4bc8b8

theashraf reviewed Jan 18, 2024

View reviewed changes

samuelOsborne and others added 13 commits January 18, 2024 11:17

fix: comments

227fa54

feat: added manifest(), emscripten bindings + udl bindings

9fe9885

fix: bindings

8635dfc

chore: 🤖 fix emscripten target for fms crate

aed8f15

fix: bindings

9e1fde5

Merge branch 'main' into feat/dotlottie-support

d557893

fix: 🐛 emscripten-bindings

994586a

chore: 🤖 skip .d.ts generation

b008511

chore: 🤖 update emscripten-bindings for load_dotlottie_data

1225b76

Merge branch 'main' into feat/dotlottie-support

7ced463

feat: 🎸 add manifest_string

5bf9bbb

fix: 🐛 panic when call manifest for a lottie json

4b45d16

chore: 🤖 fix typo

4bd33a9

theashraf approved these changes Jan 22, 2024

View reviewed changes

chore: 🤖 uncomment ts gen

e07a70e

theashraf merged commit 68201b4 into main Jan 22, 2024
1 check passed

github-actions bot mentioned this pull request Jan 22, 2024

chore: Release 0.1.12 #54

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added crate to manage .lotties #23

feat: added crate to manage .lotties #23

samuelOsborne commented Jan 9, 2024

changeset-bot bot commented Jan 9, 2024 •

edited

Loading

theashraf left a comment

theashraf Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

samuelOsborne Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

theashraf Jan 18, 2024

samuelOsborne Jan 18, 2024

	) -> Result<ManifestAnimation, DotLottieError> {
	) -> Result<Option<ManifestAnimation>, DotLottieError> {

feat: added crate to manage .lotties #23

feat: added crate to manage .lotties #23

Conversation

samuelOsborne commented Jan 9, 2024

changeset-bot bot commented Jan 9, 2024 • edited Loading

⚠️ No Changeset found

theashraf left a 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

changeset-bot bot commented Jan 9, 2024 •

edited

Loading