Removed redundant parameters in ImageSequencer.js

[vajean]

Fixes #959

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 test
• 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

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

Thanks!

[vajean]

@publiclab/is-reviewers
Please review my PR:
Done removing redundant parameters.
I could not manage to remove "loadimages export at the end of the file" - when I do, it will not pass the tests. I must have misunderstood something.

[harshkhandeparkar]

@publiclab/is-reviewers

[harshkhandeparkar]

I could not manage to remove "loadimages export at the end of the file" - when I do, it will not pass the tests. I must have misunderstood something.

Does the issue mention something like that?

[vajean]

The issue says:
... Also since now sequencer image can hold only one image at a time, loadimages export at the end of the file can be removed.

[harshkhandeparkar]

Ok let it be. Doesn't matter.

[harshkhandeparkar]

@Divy123 does ImageSequencer no longer hold multiple imgs? Also please review this.

[Divy123]

@vajean you can leave the loadImages() part.
I will have a look there and make it a new isssue.
@harshkhandeparkar yes now we support only one image per sequencer.

[Divy123]

@vajean I also request you to test your branch's code in browser as sometimes only the tests are not sufficient .
If everything goes well, the PR will be ready to merge.
Thanks a lot for your precious contributions.

[harshkhandeparkar]

yes now we support only one image per sequencer.

What is the use of sequencer.images ?

[harshkhandeparkar]

I mean loadImages()

The func name is the same

[harshkhandeparkar]

Does it not support multiple images or different steps for different imgs?

[harshkhandeparkar]

I mean that sequencer.loadImage and sequencer.loadImages both return the same function called loadImages. The function loadImages supports multiple arguements.

[harshkhandeparkar]

This is from src/ImageSequencer.js

function loadImages() {
    var args = [];
    var prevSteps = this.getSteps().slice(1).map(step=>step.options.name)
    var sequencer = this;
    sequencer.image = arguments[0];
    for (var arg in arguments) args.push(copy(arguments[arg]));
    var json_q = formatInput.call(this, args, "l");
    if(this.getSteps().length!=0){
      this.options.sequencerCounter = 0;
      inputlog = [];
      this.steps = [];
    }
    inputlog.push({ method: "loadImages", json_q: copy(json_q) });
    var ret = {
      name: "ImageSequencer Wrapper",
      sequencer: this,
      addSteps: this.addSteps,
      removeSteps: this.removeSteps,
      insertSteps: this.insertSteps,
      run: this.run,
      UI: this.UI,
      setUI: this.setUI
    };

[jywarren]

Is it something where we wanted to preserve backwards-compatibility for single-image uses that had been using loadImages()? And if so, perhaps we should put a comment next to that line in the code to note that it's for backwards compatibility only and doesn't actually support multiple images? Just wondering, i'm not sure.

…

On Mon, Apr 1, 2019 at 3:56 AM Slytherin ***@***.***> wrote: @vajean <https://github.com/vajean> I also request you to test your branch's code in browser as sometimes only the tests are not sufficient . If everything goes well, the PR will be ready to merge. Thanks a lot for your precious contributions. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#967 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ49jRPUgQy3omOeWJUQ_WHrQ9LQ5ks5vcbt7gaJpZM4cUl83> .

[Divy123]

@jywarren the loadImages() and loadImage() are an abstraction of the same function loadImages(). But since we use only one image, I think we can remove loadImages() from the export but since @vajean is encountering some error removing which can be due to its usage in some UI code, I will send a PR after figuring it out.

[vajean]

@vajean I also request you to test your branch's code in browser as sometimes only the tests are not sufficient .
If everything goes well, the PR will be ready to merge.
Thanks a lot for your precious contributions.

Hi, I have installed and tested it in a browser on my computer, everything works as the demo on your repo.

[Divy123]

I think this is ready to be merged.
@publiclab/is-reviewers @jywarren please merge this.

[jywarren]

Hmm, was this at some point to allow inserting a step at a different index? And same for removeSteps() above? @Divy123

Thank you!

[Divy123]

I think we are managing that through arguments array.

[welcome]

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will be published to https://beta.sequencer.publiclab.org in a day or two.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

[jywarren]

Thank you!!! This is great.