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

Calling update in my case triggers a recalculate on unfiltered values. #898

Closed
Herst opened this issue May 5, 2015 · 21 comments
Closed

Comments

@Herst
Copy link
Collaborator

Herst commented May 5, 2015

Built upon what made me report issue #889:

  1. go to https://jsfiddle.net/Herst/pcmsL943/17/ (all master branch stuff)
  2. load data
  3. filter by something
  4. load data again

Now you will see that the sum represents all the data and not just the filtered one.

@Mottie
Copy link
Owner

Mottie commented May 5, 2015

Hi @Herst!

This might be due to a race condition with the update, filter application and math calculation all trying to execute at the same time.

I got it to work by adding a 500ms delay before triggering a recalculation.

$('#btnLoad').click(function () {
    var $tblBody = $("#table1 tbody");

    $tblBody.empty();

    for (var i = 0; i < 10; ++i) {
        $("<tr></tr>")
            .append("<td>" + i + "</td><td>" + i + (i % 3) + "</td>")
            .appendTo($tblBody);
    }

    $("#table1").trigger("update", [true, function () {
        setTimeout(function () {
            $('#table1').trigger('recalculate');
        }, 500);
    }]);
});

I'll have to dig into the code more when I have the time to figure out why it only works after 500ms and not with shorter delay settings.

@TheSin-
Copy link
Collaborator

TheSin- commented May 5, 2015

maybe the math recall widget is bound to the wrong trigger?

@Mottie
Copy link
Owner

Mottie commented May 5, 2015

It's not in the demo I linked....

It likely has something to do with this bit of code at lines 408-411:

.on('updateComplete.tsmath', function(){
    setTimeout(function(){
        wo.math_isUpdating = false;
    }, 500);
});

@Mottie
Copy link
Owner

Mottie commented May 7, 2015

So I reduced that setTimeout to 20ms... The reason it is needed is because the math widget needs to use the "update" to add the calculations back into the cache; but it is also triggered on update, so the flag prevents infinite recursion.

I don't remember why I originally set the timeout to 500ms... it might be because of IE being slower, but I didn't have the time to test this change. I would appreciate any feedback.

@Herst
Copy link
Collaborator Author

Herst commented May 7, 2015

It does not work for me in my project. Maybe I will find time and look where exactly the problem is. It's clear that a proper solution is needed, best one without any hardcoded timeouts whatsoever and maybe some sort of order for how the widgets get updated (just like the applied order of them).

@Mottie
Copy link
Owner

Mottie commented May 18, 2015

I think the only way around this is to implement an internal queue system, which I do have planned, I just don't have the time right now to work on one.

@Herst
Copy link
Collaborator Author

Herst commented May 27, 2015

OK, I have to correct myself, it works now with the timeout hack in most but not all cases. At each update of the table (happens regularly in my case) all the total values show up first (which is wrong) and then it correctly changes to the sum of the filtered values except if I enter something into the filter field at a certain point as the update is happening. In this case the sum remains representing the total sum but when the next update happens it then corrects itself. (Maybe I will at some point create a dummy backend so I can show it to you in action.)

So in summary: it works, but certainly less than perfect.

@Mottie
Copy link
Owner

Mottie commented May 27, 2015

Yes, as I explained an internal queue system would solve this issue. It would prevent a race condition from occurring. Maybe it would help if you interrupted the regular updates if the user is interacting with the filter? It would certainly reduce server calls.

@Herst
Copy link
Collaborator Author

Herst commented May 27, 2015

What do you mean with "interacting"? While a filter is active? That would kill the use case of just seeing the sum of a subset of rows. In my project a very, very useful one.
Or just right as something is happening? I guess I could catch some filter change event and then reset the update timer. That's a nice idea, might kill the race condition at least (although the AJAX call might be already happening so I would have to add additional checks).

@Mottie
Copy link
Owner

Mottie commented May 27, 2015

I meant while the user is typing into the filter.

@Herst
Copy link
Collaborator Author

Herst commented May 28, 2015

Yeah, that's what I meant with "as something is happening", I will look into it, thanks!

But concerning a less hacky fix to such problems in general, how about instead of implementing the queue system giving the user a way to control when the widgets do their stuff when updating? So that I can explicitly control the order from my code?

@Mottie
Copy link
Owner

Mottie commented May 28, 2015

Yeah, a queue system would be awesome... I have ideas, just no the time to work on it.

@Herst
Copy link
Collaborator Author

Herst commented Feb 25, 2016

OK, so updated the demo (it was broken because of mixed content restrictions) and now I noticed that the issue does not show any more. I guess the issue is still here but for some reason it I can't trigger it this way?

@Mottie
Copy link
Owner

Mottie commented Feb 25, 2016

Hmm, well there have been around 20 updates to the math widget since this issue was opened. So, do you think it's resolved now?

@Herst
Copy link
Collaborator Author

Herst commented Feb 26, 2016

It appears to be but I am not sure because there are still some race conditions between the widgets, in the demo you can see it with the zebra widget (enter "1" on the right column filter and then press "Load Data" several times, sometimes filter comes first, sometimes zebra), so I guess for some reason the changes made led to the recalculation always happening after the filtering?

@Mottie
Copy link
Owner

Mottie commented Feb 26, 2016

Most widgets have a priority setting, if they don't, it defaults to 10.

So, the math widget should always be last. Maybe due to some usage of setTimeout, some widgets may jump the line.

@Herst
Copy link
Collaborator Author

Herst commented Feb 27, 2016

From my tests commit a8d9c2b was the one which apparently fixed this issue. Looking at what changed there, you agree that this might be it? Does this explain why this only fixed race conditions between some of the widgets?

See for yourself https://jsfiddle.net/Herst/yuxa2yd9/2/ which is based on a8d9c2b does not show the issue while https://jsfiddle.net/Herst/yuxa2yd9/1/ based on its parent (06faff7) does.

@Mottie
Copy link
Owner

Mottie commented Feb 27, 2016

Yes, this line of code uses the core's callback function to clear the math updating flag

ts.update( c, undef, function(){
    math.updateComplete( c );
});

The setTimeout is still in there to clear the flag for when something else triggers an update.

Are you only seeing an issue with the zebra widget now? I'm not seeing any issue at all in Chrome or Firefox.

@Herst
Copy link
Collaborator Author

Herst commented Feb 27, 2016

Odd, it did show for me on my work machine quite frequently, here on my home one it only shows rarely on the quickly auto-refreshing one on https://jsfiddle.net/Herst/pcmsL943/ in Chrome.

@Herst
Copy link
Collaborator Author

Herst commented Feb 27, 2016

Caught it! 😌
red_handed

@Herst
Copy link
Collaborator Author

Herst commented Feb 27, 2016

Anyways, I am going to close this issue for now. There is no more working demo. Maybe once am able to ditch the setTimeout in my project (now depends on #1166) I will see whether there is any problem left.

Oh and don't care much about the Zebra issue, #1166 is much more of a problem to me in regards to problems connected to update.

@Herst Herst closed this as completed Feb 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants