Fixes #1098 Fixes #147 Fixes #1030

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• [x] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm run test-all
• [x] code is in uniquely-named feature branch and has no merge conflicts
• [x] PR is descriptively titled
• [x] ask @publiclab/is-reviewers for help, in a comment below
• [x] Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part Thanks!

Codecov Report

Merging #1625 (8eb00b5) into main (853e719) will decrease coverage by 1.26%. The diff coverage is 12.90%.

:exclamation: Current head 8eb00b5 differs from pull request most recent head 82f7068. Consider uploading reports for the commit 82f7068 to get more accurate results

@@            Coverage Diff             @@
##             main    #1625      +/-   ##
==========================================
- Coverage   66.67%   65.41%   -1.27%     
==========================================
  Files         130      132       +2     
  Lines        2686     2741      +55     
  Branches      433      438       +5     
==========================================
+ Hits         1791     1793       +2     
- Misses        895      948      +53     

Impacted Files	Coverage Δ
src/Modules.js	100.00% <ø> (ø)
src/modules/EdgeDetect/Module.js	100.00% <ø> (ø)
src/modules/WebglDistort/Module.js	2.29% <0.00%> (ø)
src/modules/ColorHalftone/Module.js	3.63% <3.63%> (ø)
src/modules/ColorHalftone/index.js	100.00% <100.00%> (ø)
src/modules/EdgeDetect/EdgeUtils.js	86.81% <100.00%> (-0.15%)	:arrow_down:

@publiclab/is-reviewers @jywarren @HarshKhandeparkar please review this :)

It's been a long time, i don't remember the exact context -- but, throwaway sequencers shouldn't in theory be a problem. If it is well tested and you can come up with a solution that is more efficient, i'd be happy to take a look! Thanks!

On Sat, Oct 17, 2020 at 12:00 PM Harsh Khandeparkar < [email protected]> wrote:

@HarshKhandeparkar commented on this pull request.

In src/ReplaceImage.js https://github.com/publiclab/image-sequencer/pull/1625#discussion_r506956952 :

@@ -41,15 +40,21 @@ function ReplaceImage(ref, selector, steps, options) { else make(url);

 function make(url) {

•  var tempSequencer = ImageSequencer({ui: false});
  

Hmm, is it required?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/image-sequencer/pull/1625#discussion_r506956952, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6JYAR23ZGIULQIKG4RDSLG5R7ANCNFSM4KUDCEHA .

I don't like internal sequencers because they do extra work such as converting the data URIs for no reason. Instead of: New img URI -> pixels -> changed pixels -> new uri -> pixels We should have: New img URI -> pixels -> changed pixels -> use these

These internal sequencers are everywhere and do not concern this PR. Approving but I feel we should really replace internal sequencer some time.

/rebase

/rebase