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

Auto initialization & web support issue #12

Closed
wants to merge 3 commits into from

Conversation

vatsaltanna
Copy link

No description provided.

Copy link
Owner

@mrcendre mrcendre left a comment

Choose a reason for hiding this comment

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

Hello @vatsaltanna ! Thank you for your pull request.

Although the suggested changes are trying to fix some runtime issues that certain users encountered, these fixes should be performed by developers consuming the package. Enforcing them would take decisions that a developer expects to take himself/herself.

Also, adding a condition that would override the filter quality behavior regardless of the Flutter version, as well as impacting transitive dependencies for no reason is not a good practice.

Unfortunately, I have to close this PR as not actionable. Thanks for your contribution anyways !

IMPORTANT NOTE :

If you would still like to have these changes in your application, please do not republish the package under a different name like some people do. Instead, depend on your own GitHub fork in the pubspec.yaml, like so :

dependencies:
     motion:
       git:
         url: git://github.com/vatsaltanna/motion.git
         ref: my-auto-initialization-branch

Motion.instance.isSafariMobile ? null : widget.filterQuality;
FilterQuality? get filterQuality => Motion.instance.isSafariMobile
? null
: kIsWeb && widget.filterQuality == FilterQuality.high
Copy link
Owner

@mrcendre mrcendre Jan 20, 2023

Choose a reason for hiding this comment

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

This check should be performed by the user, as different versions of Flutter may or may not cause the problem you are attempting to fix. I believe it should not be endorsed by the plugin and the choice should be left to the developers who will pass the appropriate filterQuality.

Copy link
Author

Choose a reason for hiding this comment

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

Agree with you but this package doesn't work on web which led me to these changes.

Copy link
Owner

Choose a reason for hiding this comment

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

I understand it causes issues on the Web, but the app/package using motion should perform the check to kIsWeb themselves, instead of being forced to use an arbitrary value.

This is because when the Flutter team will have fixed this FilterQuality issue, some developers may want to use the filter quality that no longer causes the bug, without waiting for a package update.

Copy link
Author

Choose a reason for hiding this comment

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

I got your point. I can remove this from the PR or you can also update your package to support auto initialisation.


void initialiseInputStream() async {
await Motion.instance.initialize();
Motion.instance.setUpdateInterval(60.fps);
Copy link
Owner

@mrcendre mrcendre Jan 20, 2023

Choose a reason for hiding this comment

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

Some developers may need to delay the plugin's initialization, for example if a permission request is needed, to show a rationale before requesting that permission.

More generally, I believe auto-initializing with quite arbitrary values may cause more problem than it will solve. For example, it may have an unwanted impact on performance.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with your comment and this can be fixed in this PR.

sdks:
dart: ">=2.14.0 <3.0.0"
dart: ">=2.17.0-0 <3.0.0"
Copy link
Owner

@mrcendre mrcendre Jan 20, 2023

Choose a reason for hiding this comment

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

I can tell you were using Flutter 3. However, you should never commit a pubspec.lock without committing the corresponding pubspec.yaml changes.

This package remains compatible with Flutter 2 for now, so this change cannot land.

Copy link
Author

Choose a reason for hiding this comment

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

Understood

@mrcendre mrcendre closed this Jan 20, 2023
@vatsaltanna
Copy link
Author

Although this PR is trying to fix some runtime issues that certain users encountered, these fixes should be performed by developers consuming the package. Enforcing them would take decisions that a developer expects to take himself/herself.

Also, the title is misleading as it claims to add "Auto initialization" but also adds web detection that would override the filter quality behavior, and impacts transitive dependencies with no good reason.

Unfortunately, I have to close this PR as not actionable. Thanks for your contribution anyways !

IMPORTANT NOTE :

If you would still like to have these changes in your application, please do not republish the package under a different name like some people do. Instead, depend on your own GitHub fork in the pubspec.yaml, like so :

dependencies:
     motion:
       git:
         url: git://github.com/vatsaltanna/motion.git
         ref: my-auto-initialization-branch

I have created this PR as I wanted to use this package in one of the other package. we can not use github fork inside a publishable package.

@vatsaltanna vatsaltanna changed the title Auto initialization Auto initialization & web support Jan 20, 2023
@vatsaltanna vatsaltanna changed the title Auto initialization & web support Auto initialization & web support issue Jan 20, 2023
@mrcendre
Copy link
Owner

I have created this PR as I wanted to use this package in one of the other package. we can not use github fork inside a publishable package.

@vatsaltanna Thank you again for contributing to the package. However, motion provides a front-end UI component designed to be embedded directly into Flutter applications, not packages. If yours depends on a certain version of motion, it will lock consumers of your package to a certain version of motion, until you publish a version that upgrades its dependency, and so on...

If you still want to add it in your package's dependencies, in your specific case, you have at least three other options :

  1. Depend on the original motion and implement these specific web fix and auto-initialization behaviors inside of your package.
  2. Do not publish your package, and use the git: type dependencies in both your package's and app's pubspec.yaml.
  3. Host a private pub.dev mirror, on which you can publish any forked package you want. This is especially useful for sharing packages within an organization.

Please always keep in mind that if a package does not have a unique identity and does not bring any new/significant features, it really shouldn't be published on pub.dev and/or republished to be maintained by others without consent from the original author.

Thank you very much for your interest in motion and good luck with your projects !

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.

2 participants