Fix error in replaceImage function

[rishabhshuklax]

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!

• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm run test-all
• code is in uniquely-named feature branch and has no merge conflicts
• PR is descriptively titled
• ask @publiclab/is-reviewers for help, in a comment below
• 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]

Codecov Report

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

❗ 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%)	⬇️

[rishabhshuklax]

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

[gitpod-io]

[jywarren]

Hi, do you think we could add a test to confirm that this behaves as we expect? That'll help me do a good review. No rush at all -- apologies for the slow process here but this will help me assess the fix better! Thank you!

[jywarren]

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 < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ReplaceImage.js <#1625 (comment)> : > @@ -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 <#1625 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JYAR23ZGIULQIKG4RDSLG5R7ANCNFSM4KUDCEHA> .

[harshkhandeparkar]

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

[harshkhandeparkar]

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

[harshkhandeparkar]

Oh but it still needs a test!

[jywarren]

/rebase

[jywarren]

/rebase

[gitpod-io]