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.

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

Add Lottie integration #285

Closed
wants to merge 11 commits into from
Closed

Conversation

onlymice
Copy link
Contributor

What does this change?

  • Adds lottie integration.

  • Checks for lottie files by in json tags

  • Example includes on how to use it

  • Docs changed accordingly

Related

Initial Feature Request: #47
Previous PR that tried to introduce support for Lottie: #70

What's different now?

Previously HiroyukiTamura #70 implemented a Lottie integration per request #47 (comment) but as stated by britannio's #70 (comment) in the initial PR, their implementation will be better off checking for keys in the file itself, rather than only it's extension *_lottie.json as it was initially requested two years ago.

So I did.

Other issues

Why there is a lot of changes to tests

Initially Implementation of this feature required File read access, there was no issues with that if you run code as designed e.g

flutter packages pub run build_runner build

But if you try to run tests you might ended up seeing that the relative asset path that you get in isSupport is not relative to the Directory.current.path and the code in 'packages/core/lib' can't access it while running tests.

So I introduced mocked rootPath #assets_gen_integrations_test.dart#L13 for tests and now the AssetType has field for absolutePath that is constructed from the passed rootPath (config.rootPath) and the key/path.
Other integrations haven't tried to read files, cause they don't need it, so here we are, feel free to propose a better solution.

What is the value of this and can you measure success?

Measure

  • Pass tests.

  • No linting issues with generated code.

  • Example runs, plays the animation and works well.

Value

  • Others who tends to use Lottie in their projects will be happy to know that flutter_gen supports it

* example & test asset
* auto strip "lottie" from final asset var name
* dropped support for '*_lottie.json' per FlutterGen#70 PR
* include root path for tests
* update example
* rename resourcesPath for better code readability
@onlymice
Copy link
Contributor Author

For Discussion

  1. I have set a strict version for lottie: '>=1.4.1 <2.0.0'
    because lower versions accept different types for parameters.
    1.1. we can use dynamic and recheck for allowed runtimeType in generated constructor.
    1.2. leave as is.

  2. keys are checked by every with list lottieKeys that must be included in the file for Lottie to render it properly,
    the thing is that I haven't found the exact specification for what keys are required (and other libs don't even care if they don't exist), in fact the only thing I have found is that the version in the flutter Lottie package is set to check for >=4.4.0, and layers is where most of the data are.
    2.1. choose what keys are actually required.
    2.2. leave as is.

@wasabeef wasabeef self-requested a review August 30, 2022 17:09
@wasabeef
Copy link
Member

@onlymice

Thank you for your nice PR. I'll check it next week, so please wait. 🙏🏽

@@ -14,6 +14,7 @@ version_gen:
path: lib/

dependencies:
lottie: '>=1.4.1 <2.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need dependencies on the core package.

AssetType(this.path);
AssetType(this.path, String rootPath) : absolutePath = p.join(rootPath, path);

final String absolutePath;
Copy link
Member

@wasabeef wasabeef Sep 15, 2022

Choose a reason for hiding this comment

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

We don't prefer to widen the influence range because it is only used in LottieIntegration#isSupport.

@@ -29,7 +30,7 @@ class AssetsGenConfig {

factory AssetsGenConfig.fromConfig(File pubspecFile, Config config) {
return AssetsGenConfig._(
pubspecFile.parent.path,
pubspecFile.parent.absolute.path,
Copy link
Member

Choose a reason for hiding this comment

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

We believe the problem can be solved by simply passing the RootPath to isSupport. No big changes to assets_generator.dart is needed.

@@ -31,8 +33,10 @@ void main() {
expect(integration.className, 'SvgGenImage');
expect(integration.classInstantiate('assets/path'),
'SvgGenImage(\'assets/path\')');
expect(integration.isSupport(AssetType('assets/path/dog.svg')), isTrue);
expect(integration.isSupport(AssetType('assets/path/dog.png')), isFalse);
expect(integration.isSupport(AssetType('assets/path/dog.svg', resPath)),
Copy link
Member

Choose a reason for hiding this comment

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

We don't add any variables to the AssetType, so no changes here are needed either.

@wasabeef
Copy link
Member

My codes.

integration.dart

abstract class Integration {
  // ..
  bool isSupport(AssetType type, {String? rootPath});
  // ..
}

svg/rive/flare integratinos.dart

class SvgIntegration extends Integration {
  // ..
  @override
  bool isSupport(AssetType type, {String? rootPath}) => type.mime == 'image/svg+xml';
  // ..
}

lottie integratinos.dart

class LottieIntegration extends Integration {
  // ..
  @override
  bool isSupport(AssetType type, {String? rootPath}) =>
      isLottieFile(type, rootPath ?? p.current);

  @override
  bool get isConstConstructor => true;

  bool isLottieFile(AssetType type, String rootPath) {
    if (!type.path.endsWith('.json')) {
      return false;
    }
    var input = File(p.join(rootPath, type.path)).readAsStringSync();
    var fileKeys = jsonDecode(input);
    if (fileKeys.runtimeType != Map &&
        !lottieKeys.every((key) => fileKeys.containsKey(key))) {
      return false;
    }
    var versions = fileKeys['v'];
    if (versions is! String) {
      return false;
    }
    var version = int.tryParse(versions.replaceAll('.', '')) ?? 0;
    // Lottie version 4.4.0 is the first version that supports BodyMovin.
    // https://github.com/xvrh/lottie-flutter/blob/0e7499d82ea1370b6acf023af570395bbb59b42f/lib/src/parser/lottie_composition_parser.dart#L60
    return version / 1000 >= 0.440;
  }

  // ..
}

assets_generator.dart

// ... only this
(element) => element.isSupport(assetType, rootPath: rootPath),
// ...

assets_gen_integrations_test.dart

   // No change except here
   // only add.
    test('Assets with Lottie integrations on pubspec.yaml', () async {
      const pubspec = 'test_resources/pubspec_assets_lottie_integrations.yaml';
      const fact =
          'test_resources/actual_data/assets_lottie_integrations.gen.dart';
      const generated =
          'test_resources/lib/gen/assets_lottie_integrations.gen.dart';

      await expectedAssetsGen(pubspec, generated, fact);

      final integration = LottieIntegration();
      expect(integration.className, 'LottieGenImage');
      expect(integration.classInstantiate('assets/lottie'),
          'LottieGenImage(\'assets/lottie\')');
      final testResPath = p.absolute('test_resources');   // 🌟
      expect(
          integration.isSupport(AssetType('assets/lottie/hamburger_arrow.json'),
              rootPath: testResPath), // 🌟
          isTrue);
      expect(
          integration.isSupport(
              AssetType('assets/lottie/hamburger_arrow_without_version.json'),
              rootPath: testResPath),
          isFalse);
      expect(integration.isConstConstructor, isTrue);
    });

@wasabeef
Copy link
Member

wasabeef commented Sep 15, 2022

@onlymice Thanks for the great suggestions for a lottie users and me.

@wasabeef wasabeef added this to the v5.0.0 milestone Sep 15, 2022
@onlymice
Copy link
Contributor Author

onlymice commented Sep 19, 2022

Hi @wasabeef,
Hope you doing well,
I'm pinging you because I've decided to use a proper semver, my bad for not going with it initially 😅

updated code 🎯

bool isLottieFile(AssetType type) {
    try {
      if (type.extension != '.json') {
        return false;
      }
      String input = File(type.absolutePath).readAsStringSync();
      final fileKeys = jsonDecode(input) as Map<String, dynamic>;
      if (lottieKeys.every((key) => fileKeys.containsKey(key)) &&
          fileKeys['v'] != null) {
        var version = Version.parse(fileKeys['v']);
        // Lottie version 4.4.0 is the first version that supports BodyMovin.
        // https://github.com/xvrh/lottie-flutter/blob/0e7499d82ea1370b6acf023af570395bbb59b42f/lib/src/parser/lottie_composition_parser.dart#L60
        return version >= Version(4, 4, 0); 🎯
      }
    } on FormatException catch (e) {
      // Catches bad/corrupted json and reports it to user. 🎯
      stderr.writeln(e.message);
    }
    return false;
  }

@wasabeef wasabeef removed this from the v5.0.0 milestone Sep 27, 2022
@wasabeef wasabeef mentioned this pull request Sep 27, 2022
@wasabeef
Copy link
Member

wasabeef commented Sep 28, 2022

Hi @onlymice,
I've merged #298 based on your PR codes created, it changed a few variable names, etc.
Thank you so much. We will be releasing v5.0.0 soon.

@wasabeef wasabeef closed this Sep 28, 2022
@wasabeef wasabeef mentioned this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants