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

eventData.changedRows returning last obj in history on undo rather than history point #11

Closed
swampthang opened this issue May 11, 2017 · 21 comments

Comments

Projects
None yet
2 participants
@swampthang
Copy link

commented May 11, 2017

Question: Shouldn't eventData.changedRows[0] return the new state of any changed rows via the changed history point when undo/redo is triggered?

I have an on("change") function set up that listens for changes to a table. I have an undoing and a redoing variable, the values of which are tied to undo and redo buttons.

Note the 2 console.log's in my on("change") function. You can see that eventData.changedRows[0] always returns the latest point in history rather than the current history point after undo/redo.

I'm getting around that right now by running a query to retrieve the changed row which does return the current history point so I'm able to get it to work. Just wondered if that was your intention for this.

Below is a screenshot of the console.log's highlighted.

(NOTE: Just as a FYI, .settings.activeSelectors.options[0] is the field I'm changing in order to test to keep my log's a little more simplified. In the first history point I assigned "testing one" as the value and in the next assigned "testing one two" )

nSQL("aniblocks").on('change',function(eventData) {

  getHistory();

  switch(eventData.changeType) {
    case 'modified':
      if(eventData.changedRows.length > 0) {

        // eventData.changedRows[0] contains data from the latest history point
        console.log('row: ',eventData.changedRows[0].settings.activeSelectors.options[0]);

        if( undoing || redoing ) {
          db.query('select').where(['id','=', eventData.changedRows[0].id]).exec()
          .then(function(result,db){

            // result contains data from the current history point after an undo/redo
            console.log(result[0].settings.activeSelectors.options[0]);
           
            // doing stuff with it here
            animationsMgr.animateFromDB(result[0]);
            resetDoingVars();
          })
        }
      } else {
        // console.log('nothing changed');
      }
    break;
    case 'deleted':
      console.log('deleted');
    break;
    case 'inserted':
      console.log('inserted');
    break;
  }
});

screen shot 2017-05-10 at 7 06 53 am

@swampthang

This comment has been minimized.

Copy link
Author

commented May 11, 2017

Upon looking at my screenshot of the logs closer, it looks like it might have something to do with the order in which things are executed. Here are the other functions involved. The only reason I had to set up an on("change") listener was because the nSQL().extend history triggers don't return the changed values so I needed a way to listen for those. Is there a way to snag the changed data from the extend functions or possibly order these so that the change trigger always executes after a history point change is triggered?

function getHistory() {
  nSQL().extend("?").then(function(status) {
    console.log(status) // <= [0,0];
  });
}

function resetDoingVars() {
  undoing = redoing = false;
}

// undo/redo functions are agnostic to tables - will have to listen for changes in each table
$('#undo-btn').on("click", function(){

  nSQL().extend("<").then(function(response) {
    console.log(response) //<= If this is true, an undo action was done.  If false, nothing was done.
    if(response) {undoing = true};
  });
})

$('#redo-btn').on("click", function(){

  nSQL().extend(">").then(function(response) {
    console.log(response) //<= If this is true, a redo action was done.  If false, nothing was done.
    if(response) {redoing = true};
  });
})

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 11, 2017

So you were talking about wanting to contribute? ;)

This is definitely a larger issue than the other one, I'll have time later tonight/early tomorrow to dig in.

@ClickSimply ClickSimply added the bug label May 11, 2017

@swampthang

This comment has been minimized.

Copy link
Author

commented May 12, 2017

Ahhh, yea. Given that you see it as rather complex I'm not sure about getting my feet wet with this one. ;-)

Will give it a go as soon as this app gets out of beta though. For now, running another query solves the problem.

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 14, 2017

The only problem I see with this approach is sometimes the changedRows object will return an array of null values. This happens when the history is deleting rows to complete a history step.

Though, I admit that it makes bit more sense showing the results of the history action inside the events instead of what was there before, but it does create this odd situation.

Not that I want to create more work for myself but it seems like we might need another event property, changedRowIDs, an array of row IDs affected by the most recent action. This way you can get some access to what has been removed from the database.

@swampthang

This comment has been minimized.

Copy link
Author

commented May 15, 2017

Yea, that makes a lot of sense. Sorry for the delay replying. Was doing Mother's Day stuff out of town :)

@ClickSimply ClickSimply added enhancement and removed bug labels May 16, 2017

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

Just published an update partially implementing your request. The change events from history now return the after effects of the history change, not before effects.

I'm working on a big update to the transaction system that's requiring I rework pretty large sections of the code, I'll add the primary key array to the event object while I'm finishing that up.

@swampthang

This comment has been minimized.

Copy link
Author

commented May 16, 2017

Awesome, Scott! Much appreciated!

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 17, 2017

Alright, the events now emit primary keys. Take a look and let me know if everything is resolved. 👍

@swampthang

This comment has been minimized.

Copy link
Author

commented May 18, 2017

Thanks so much. I wonder if I need to convert previous objects somehow. My model looks like this:

.model([
  {key:'id',type:'uuid',props:['pk']},
  {key:'blockID', type: 'string', props: ["idx"]},
  {key:'blockInnerHTML', type: 'string'},
  {key:'blockStartTime', type: 'float', default: 0.0},
  {key:'blockTimeScale', type: 'float', default: 0.0},
  {key:'title',type:'string'},
  {key:'type',type:'string'},
  {key:'settings',type:'map', default: null},
  {key:'removeSelector',type:'string', default: null},
  {key:'timelineData',type:'map'}
])

When trying to loadJS from a file (which I've been doing successfully for some time now) I'm getting an undefined error at the point noted below.

function processData(fileData) {

  var projectPath = fileData.projectDir;
  var convertToProject = false;
  nSQL().loadJS("aniblocks",fileData.nSQLData)
  .then(function(result,db){
    // result contains nothing
    return db.query("select").exec();
  })

Here's an example of one of the objects after being loaded from a file. The objects are contained in fileData.nSQLData. As you can see, the id is included. One thing to note - as of 0.8.41 it loads the data fine.

screen shot 2017-05-18 at 5 47 05 am

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 18, 2017

Ah, just looking at the code now I think I know the problem...

loadJS and loadCSV are now using a new transaction syntax, I believe I forgot to pass the db variable through transactions.

Try doing this for the import block and see if it works:

function processData(fileData) {

  var projectPath = fileData.projectDir;
  var convertToProject = false;
  nSQL().loadJS("aniblocks",fileData.nSQLData)
  .then(function(result,db){
    // result contains nothing
    return nSQL("aniblocks").query("select").exec();
  })

If it does, then it's a simple fix. 👍

@swampthang

This comment has been minimized.

Copy link
Author

commented May 18, 2017

Wanted to mention another thing that I've noticed even going back to 0.8.31. Not creating a separate issue for it yet since it might not exist in the latest version but it might be related to this since it only happens when using data loaded from a file. I'm using version 0.8.41 because, as noted above, I'm unable to use the latest (0.8.5).

After loading JSON data from a file using loadJS, I can make changes to a record and the change does get registered. Undo/redo state gets incremented as well. But, when executing undo the results returned contain a null value for changedRows. I've confirmed that this doesn't happen when creating records in the app - not loading the data from a file.

This is what I see in the change event when I make a change to a record right after loading data from a file. Note the history point is [1,1] in the console.

screen shot 2017-05-18 at 7 26 07 am

This shows the next action which was undo. Note the null value for changedRows but the result array recognizes the undo action and decrements the history point.

screen shot 2017-05-18 at 7 28 08 am

@swampthang

This comment has been minimized.

Copy link
Author

commented May 18, 2017

After studying this a little further, it seems like the first change to every row is part of the history count status but the data, or possibly the id, somehow doesn't get associated with it. The next change does get stored but it increments the history point again so here's what you get...

  • On create records from JSON, history array is [0,0]
  • First change to any record increments to [1,1] but returns changedRows[0] == null
  • Second change to any record increments to [2,2] and doesn't return null.

What this means is, if I have 10 records, I could change all 10 once and history would be [10,10] but running undo would return a null value. I could change each one twice and history would be [20,20].

After changing each one twice, putting me at [20,20], I can run undo and see the last change, putting me at [20,19]. The next undo, puts me at [20,18] but returns a null value. The next undo returns the second change to the next to the last record but puts me at [20,17] and so on.

@swampthang

This comment has been minimized.

Copy link
Author

commented May 18, 2017

Oops! Missed your note above. Too quick for me! Checking now.
Yep, that resolved the issue loading the data in 0.8.5. Also, the changedRows[0] object does contain the after data. Awesome!

So that's working but the issue I mentioned above regarding the undo/redo status still exists. Want me to create a new issue for that?

@swampthang

This comment has been minimized.

Copy link
Author

commented May 18, 2017

Another thing to note is that if you undo from [1,1] back to a null value, you can make as many changes as you like to that row and the history stays at [1,0]. Even when not loading from a file, there are some screwy things happening with undo/redo. If I create a record, then run an upsert, I am at [2,2]. If I run an undo, I'm at [2,1]. If I then run a redo, nothing changes and I end up at [2,2] where undo returns false from then on.

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 19, 2017

Alright, just pushed 0.8.51, all history related issues should be fully resolved.

@swampthang

This comment has been minimized.

Copy link
Author

commented May 19, 2017

Everything is working great as long as I don't start with data loaded from a file. It doesn't seem to be picking up the first change to any row. Have you tested that with your new changes? Maybe it's the way I'm loading the data.

I guess the question is, do I need to do anything differently to instantiate history for rows/records loaded from a JSON file?

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 19, 2017

Can you email me the file? Shouldn't be needed and yes I tested everything. 😄

@swampthang

This comment has been minimized.

Copy link
Author

commented May 19, 2017

Sure. Thanks!

@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 20, 2017

So an improvement I added (using transactions to import data) is what is causing this...

Transactions bypass the history system entirely, which is why you're not seeing history points for the imported data.

The newest release has an optional third argument for loadJS and loadCSV that will allow you to disable the transaction import and do a standard import instead.

// Loading JSON data without transactions
nSQL().loadJS("table", data, false).then...
@ClickSimply

This comment has been minimized.

Copy link
Owner

commented May 25, 2017

I'm going to close this issue since there hasn't been a response to it in 5 days, feel free to open it again if the issue is still present.

@swampthang

This comment has been minimized.

Copy link
Author

commented May 25, 2017

It works! Thanks so much, Scott. Sorry for the delay replying. Been knee deep in FFMPEG hell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.