Prev step final API #268

tech4GT · 2018-05-24T23:15:21Z

@jywarren After some research here are the results
We are calling the module from within the addStep function and there is a variable(function) getStep defined in the addStep scope which we wish to pass on to the module.
There are 3 ways we can do this

Input parameter: pass as a parameter, we do not want to do this
this: Using this of the function scope we can bind getStep and the data it operates on to the this of
module, here we would have to use this.getStep(), hence not desirable
global variable: this is kind of what we were doing with the this binding but with unnecessary complexity, so we make the getStep global then we can call it directly but the data also becomes shared among all steps and hence the relative offset won't do

My solution: I have passed the input given to the module's draw as a parameter to the getStep(global function) through which we can determine the relative offset in the steps array

SO this makes our API like this

//inside module
draw(input,UI){
    getStep(input, offset);
}

offset keeps the same values as before so an offset of -1 gives previous step and an offset of 0 gives current step

@jywarren how does this look??

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-24T23:16:44Z

fixes #266

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-25T15:12:53Z

@jywarren have a look at this one

jywarren · 2018-05-25T16:57:48Z

Hi, Varun - this is getting very close and I appreciate your trying to get all the way there.

I wonder -- what if we passed in a reference to the sequence and a reference to the current step's unique ID when we construct the module -- for example as part of the options object -- that way it's available within the scope of the draw() method?

Each step ought to know its own unique id, right? So can getStep() see the unique id?

tech4GT · 2018-05-25T17:02:55Z

@jywarren Actually I researched this and the thing is that since getStep is not defined inside of the module, as in its definition was not done inside the module code, it CANNOT have access to any local variables inside the module directly. Actually we can never really inject local variables inside of a function directly, best we can do is pass it with this, so if we bind the index to options and then inside the getStep I think we can access it by saying this.options.index

jywarren · 2018-05-25T17:38:45Z

Let me think about this a bit!

tech4GT · 2018-05-25T17:44:58Z

@jywarren I got this but there is one problem, we would have to bind the this for getStep since it is a global function

tech4GT · 2018-05-25T17:45:26Z

SO the API will become getStep(offset).bind(this)

tech4GT · 2018-05-25T17:45:44Z

I don't think this is desirable, right @jywarren ??

jywarren · 2018-05-25T17:46:37Z

Were you able to look into Object prototype inheritance at all as another potential way to create built-in methods?

jywarren · 2018-05-25T17:48:49Z

src/AddStep.js

@@ -25,10 +25,10 @@ function AddStep(_sequencer, image, name, o) {
      options: o
    };
    var UI = _sequencer.events;
-    this.getStep = function getStep(offset) {
-      return _sequencer.images[image].steps.slice(offset - 1)[0];
+    getStep = function getStep(input, offset) {


wait, why does this have to be global? So that it can be accessed from draw()? Yeah, i think we probably don't want to do this... it can get very messy. Let me think a bit more about it!

@jywarren Actually The idea here is that getStep() is a function on sequencer itself and then what we pass it is a context and and an offset

getStep(context,offset)

@jywarren the other solution we did before this essentially made it global as well!! that is why the data was being shared across all steps. If we want to call a function without using something.getStep() then either getStep has to be defined locally inside of the module or we have to make it global, I read a lot about this and we cannot inject local variables inside of a function, all we can remap is the this pointer

tech4GT · 2018-05-25T17:50:26Z

@jywarren then too we won't be able to call getStep directly, we would have to call it like someObject.getStep()

jywarren · 2018-05-25T17:54:39Z

OK, i think we should settle for someObject.getStep() -- let's do tools.getStep() maybe.

Before we commit to this, can you look through Jasmine docs/code to see how they get such simple functions in their tests?

https://jasmine.github.io/tutorials/your_first_suite

tech4GT · 2018-05-25T18:03:47Z

@jywarren Got It!! They are injecting these functions on the top level of each test file

tech4GT · 2018-05-25T18:06:51Z

@jywarren According to what I can gather, they are using some process to inject this code on the top level of each file which runs and consumes these functions, if we want to do this then while building the project we would have to inject the whole getStep api code onto the top level of each module

tech4GT · 2018-05-25T18:07:15Z

@jywarren I maybe wrong here, this is what I could understand

jywarren · 2018-05-25T18:07:43Z

Anything from there we could learn from?

jywarren · 2018-05-25T18:11:10Z

injecting how -- with eval() or something?

…

On Fri, May 25, 2018 at 2:07 PM, Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> I maybe wrong here, this is what I could understand — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ13zZPPb4wItfmBvHwSHSWo8h7Fbks5t2EhUgaJpZM4UNI2S> .

tech4GT · 2018-05-25T18:30:54Z

@jywarren Yeah we can make the function not global but put it in the minimum scope such that it can be shared by all the files

tech4GT · 2018-05-25T18:41:58Z

@jywarren found this here https://stackoverflow.com/questions/14020189/node-js-accessing-local-variables-from-another-module
Please see the first answer, this is what we can do with each module in Modules.js, so every module will have it's own copy of these functions hence the scope problem will be solved

tech4GT · 2018-05-25T18:47:47Z

@jywarren Another idea!!!! What if we bindle this api up as a module itself and then allow the module maker to import it if he wishes to do so, I am including a few examples

import getStep from tools 
getStep(offset);

import {getStep, getPreviousStep} from tools
getStep(offset);

import * as tools from tools
tools.getStep(offset);

tech4GT · 2018-05-25T18:49:31Z

@jywarren this one seems very natural to me!! Importing the functions on demand, this way they will not even the code will be optimized as well, since if no step uses getStep api then these functions will not be created at all!!

jywarren · 2018-05-25T19:20:54Z

But how would that module have access to the step's position and the whole steps array? I think we need to know how these are passed into getStep, right?

At the moment we define getStep, can we pass it into draw() with an intact way to know what the current index is? Perhaps one of the parameters of draw(__,__) should be index -- or we should pass an options object to draw() which can contain this?

tech4GT · 2018-05-25T19:24:39Z

@jywarren actually this can be handled through this. So what we happens is that if the function is global, this inside of it points to global scope, hence I had to bind it but if the function is not global then the this will be same as this inside of draw, in which we can store the index and it will be accessible inside of the getStep function via this.index.

tech4GT · 2018-05-25T19:27:14Z

@jywarren fundamentally what we are doing is that we need a context and an offset to determine the right step, currently due to getStep being global we are not able to use the this pointer to pass context to it, but if it will be a non global function we can pass the context via this and offset is an argument hence completing both the requirements. Does that make sense??

jywarren · 2018-05-25T19:27:50Z

You know, i do really want to not have to say tools.getStep() but this is just making things extra nice... it's not a critical thing. So why don't we keep this discussion in mind, but move to passing in a tools parameter to draw() for now, and then think about a better version later.

I hear what you're saying on this -- but in general as soon as there is a this command, i assign it to a named local variable so we don't get mixed up, since this can change often. So i usually do:

function Step() {
  var step = this;
  // do things with "step"
  return step;
}

to avoid any confusion, you know?

jywarren · 2018-05-25T19:28:22Z

OK - can you refactor with your latest proposal in mind and I'll read it over? Thanks!!!

tech4GT · 2018-05-25T19:32:47Z

@jywarren in that case one can always say import getStep from tools but I'll keep thinking on this one

jywarren · 2018-05-27T18:06:05Z

But also confirmed that this does work without bugs!

jywarren · 2018-05-27T18:10:14Z

dist/image-sequencer.js

@@ -47836,6 +47923,11 @@ function Run(ref, json_q, callback, progressObj) {
      var image = drawarray[pos].image;
      var i = drawarray[pos].i;
      var input = ref.images[image].steps[i - 1].output;
+
+      getStep = function getStep(offset) {


Ah and also this is still a global, no? I'm reluctant about using a global. Can we stick with this.getStep or something.getStep to avoid this until we find a better solution? Thanks!!!

@jywarren Actually I am pretty sure that if we want to call getStep() directly from inside module then either this function would have to be defined inside the module or we have to make it global, what that means for us is that if we want to do getStep(offset) then we have to use global or else we have to use this.getStep()

Also @jywarren I don't think global here is a bad thing since utility functions are the only thing that we normally make global anyway

Ok, so although I hear what you're saying, I really want this library to be self contained. Let's go for this.getStep and we can get this big fixed and next steps in another PR!

@jywarren Okay doing this without global now😁

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-27T18:28:44Z

@jywarren I have done all the changes except for the global one, are you sure we don't want the direct getStep() ?? If not then we can actually go back all the way since we don't have to worry about the global object anymore

jywarren · 2018-05-27T18:35:49Z

Let's move away from the global. I really feel we can revisit with a better architecture to get rid of this. later, but don't want that to stop our progress right now as it doesn't make that big a difference to users.

Thank you Varun!

tech4GT · 2018-05-27T18:41:34Z

@jywarren Also then all the util functions would be defined on all steps separately, I guess that's okay?

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-27T18:57:34Z

@jywarren please have a look I think this covers everything now

Signed-off-by: tech4GT <[email protected]>

jywarren · 2018-05-28T03:19:30Z

src/Run.js

@@ -73,6 +70,11 @@ function Run(ref, json_q, callback, progressObj) {
    }
    return json_q;
  }
+
+  function getStep(offset) {
+    if(i + offset >= ref.images[image].steps.length) return {options:{name:undefined}};


I believe it's saying i is undefined here?

ReferenceError: i is not defined at getStep (http://localhost:41099/bundle.js:49660:5)

Oh @jywarren my bad, I forgot the closure!! fixing this now

jywarren · 2018-05-28T03:19:56Z

src/Run.js

@@ -57,6 +71,11 @@ function Run(ref, json_q, callback, progressObj) {
    return json_q;
  }

+  function getStep(offset) {
+    if(i + offset >= ref.images[image].steps.length) return {options:{name:undefined}};


Is this right? do we want to return an object or just let it fail? Either way, just want to know your thinking on this, thanks!!!

@jywarren Actually I was thinking we can write standard tests, even for cases like these so we can have a test which compares the name to undefined

I mean like in our test we would have tests which will check the .options.name and if the returned value itself is undefined then we will face an issue . that test will fail, that's what I thought, what do you think?

jywarren · 2018-05-28T03:21:40Z

src/util/getStep.js

+    },
+
+    getIndex : function(){
+        return this.getStep(0).options.number;


Will this work if a new step is inserted? Just checking! If not, we can file an issue for this and solve in a future PR -- perhaps in part2 of #261...

@jywarren It should!! Actually the thing here is that i added our utility functions in Run and not Addstep so whatever we do, add or remove or insert steps in the end we will call sequencer.run() right, that's where all the functions get bound!! SO this should work nicely!! @jywarren Do you understand or I should explain in more detail??

So i just mean if number is assigned on this line or in corresponding line from AddStep:

image-sequencer/src/InsertStep.js

Line 18 in 09c4f25

ID: o.number,

and then we insert a step halfway through the sequence, number may become invalid as a reference to the position in the array of steps. So either number should be updated in insertStep or we should use a function like getStep() that recalculates and returns the updated position in the array. I think we could just update options.number, right?

Let's just do this now as part of addStep/insertStep, then in #261 when we consolidate those two, it'll be a bit less repetitive. Then I'll merge this -- sounds great!

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-28T04:24:38Z

@jywarren Sorry for bad pr, I just fixed it, now everything is working nicely!!

jywarren · 2018-05-28T04:53:58Z

Oh that's awesome. Great idea!

…

On Mon, May 28, 2018, 12:20 AM Varun Gupta ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Run.js <#268 (comment)> : > @@ -57,6 +71,11 @@ function Run(ref, json_q, callback, progressObj) { return json_q; } + function getStep(offset) { + if(i + offset >= ref.images[image].steps.length) return {options:{name:undefined}}; I mean like in our test we would have tests which will check the . options.name and if the returned value itself is undefined then we will face an issue . that test will fail, that's what I thought, what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ95soRzTGGdpURd5ScSrkgITmCSYks5t23rLgaJpZM4UNI2S> .

jywarren · 2018-05-28T04:55:06Z

Oh do you mean options.number gets reassigned/updated each run?

…

On Mon, May 28, 2018, 12:22 AM Varun Gupta ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/util/getStep.js <#268 (comment)> : > @@ -0,0 +1,22 @@ +module.exports = { + getPreviousStep : function () { + return this.getStep(-1); + }, + + getNextStep : function() { + return this.getStep(1); + }, + + getIndex : function(){ + return this.getStep(0).options.number; @jywarren <https://github.com/jywarren> It should!! Actually the thing here is that i added our utility functions in Run and not Addstep so whatever we do, add or remove or insert steps in the end we will call sequencer.run()right, that's where all the functions get bound!! SO this should work nicely!! @jywarren <https://github.com/jywarren> Do you understand or I should explain in more detail?? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJwtJ-CVSC4o2cr49RfjLwqkJIqVtks5t23t8gaJpZM4UNI2S> .

tech4GT · 2018-05-28T04:55:51Z

@jywarren Yeah!! Every time sequencer will be run all that data will be recalculated!!

tech4GT · 2018-05-28T04:56:29Z

So that gives us the freedom to whatever with the steps array and then call run and everything will work nicely!!

tech4GT · 2018-05-28T04:57:49Z

@jywarren So what do you say, should we merge this in??

tech4GT · 2018-05-28T04:58:55Z

I'll implement the rest of the getStep API methods #242 in another PR

jywarren · 2018-05-28T05:00:22Z

Oh sorry i wasn't sure -- it doesn't look like options.number is recalcuated -- can you confirm with a line number?

I have to go to sleep now! But I'll check in the morning :-)

jywarren · 2018-05-28T05:00:27Z

Thanks!!!!

tech4GT · 2018-05-28T05:02:11Z

@jywarren sure, I'll have it marked and if it does not happen I'll implement it by the time you wake up!! Good night 😁

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-05-28T05:30:03Z

@jywarren you were right!!! Thanks a lot!! Fixed this
https://github.com/tech4GT/image-sequencer/blob/71eead52239073f2300a9c9e1c38db835d87bba7/src/Run.js#L27-L29

jywarren · 2018-05-28T05:34:59Z

Awesome. Looks like next up is the importimage drag drop listeners and the #261 changes, sound good? Thanks Varun!

tech4GT · 2018-05-28T05:35:56Z

@jywarren sure!!

tech4GT added 2 commits May 25, 2018 03:06

1.7.0

fb1a355

fix the api

56c85b3

Signed-off-by: tech4GT <[email protected]>

fixes publiclab#266

ed8dd64

Signed-off-by: tech4GT <[email protected]>

jywarren reviewed May 25, 2018

View reviewed changes

jywarren requested changes May 27, 2018

View reviewed changes

changes and refactor

4f274d3

Signed-off-by: tech4GT <[email protected]>

now using this.getStep and related functions

773110d

Signed-off-by: tech4GT <[email protected]>

tech4GT added 2 commits May 28, 2018 00:38

add getNextStep

4b6fdbe

Signed-off-by: tech4GT <[email protected]>

minor refactor

8a2c129

Signed-off-by: tech4GT <[email protected]>

jywarren approved these changes May 28, 2018

View reviewed changes

jywarren reviewed May 28, 2018

View reviewed changes

fix

d247418

Signed-off-by: tech4GT <[email protected]>

update getIndex

71eead5

Signed-off-by: tech4GT <[email protected]>

jywarren merged commit 1de72d7 into publiclab:master May 28, 2018

Prev step final API #268

Prev step final API #268

Conversation

tech4GT commented May 24, 2018 • edited Loading

tech4GT commented May 24, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

tech4GT commented May 25, 2018 • edited Loading

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

jywarren commented May 25, 2018 via email

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

tech4GT commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 25, 2018

jywarren commented May 25, 2018

tech4GT commented May 25, 2018

jywarren commented May 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented May 27, 2018

jywarren commented May 27, 2018

tech4GT commented May 27, 2018

tech4GT commented May 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented May 28, 2018

jywarren commented May 28, 2018 via email

jywarren commented May 28, 2018 via email

tech4GT commented May 28, 2018

tech4GT commented May 28, 2018

tech4GT commented May 28, 2018

tech4GT commented May 28, 2018

jywarren commented May 28, 2018

jywarren commented May 28, 2018

tech4GT commented May 28, 2018

tech4GT commented May 28, 2018

jywarren commented May 28, 2018

tech4GT commented May 28, 2018

tech4GT commented May 24, 2018 •

edited

Loading

tech4GT commented May 25, 2018 •

edited

Loading