fix(core): queuePostProcessedRowForCleanup is missing

[ghiscoding]

• when merging X-Slickgrid for the Frozen stuff, the queuePostProcessedRowForCleanup was forgotten as part of the merge.
• ref issue queuePostProcessedRowForCleanup is never called #483

Ready for Testing

I don't know how to test it, so please don't merge until someone knows how to test it and possibly change the code if need be. I copied over the missing piece of code, but that might not work since some of the X-Slickgrid uses jQuery DOM references while in this fork, there are portions which use native DOM references. So unless someone knows how to test it, this PR might not be merged.

I tried the example10a-async-post-render-cleanup.html and I don't see any error. I see a bunch of console log showing cleaned/rendered. So I assume everything is good, is there anything else that can be tested for this queuePostProcessedRowForCleanup method? If that is the only example to test with, it looks fine to me.

Some references

Looking at previous version tag 2.3.23, it seems to be called at 2 different places here and here

There's also unpkg - SlickGrid@2.3.23 which you can see any version code.

[ghiscoding]

@6pac
I suggest you install this GitHub free WIP plugin to your project, this plugin basically prevents you from merging something that has the word WIP in its subject.

I have it installed in my repos and it changes the Merge button and its good to prevent merging something that might not be finished or ready... like this PR

[ghiscoding]

@6pac
Can we merge this even if it's maybe not complete? Because I get opened issue in Angular-Slickgrid when I know it's related to this code here, I would rather merge this code and tweak it in the future if need be (instead of completely missing the cleanup portion as it is today)

[6pac]

sure. let me tak a look first, i wrote this code: http://low-bandwidth.blogspot.com/2015/06/slickgrid-async-post-render-cleanup.html

[ghiscoding]

That would be great if we could close this old issue

[6pac]

Hey @ghiscoding ,

I'm well on the way to patching this, but I am a little puzzled by the following code:

function handleMouseWheel(e, delta, deltaX, deltaY) {
  var $rowNode = $(e.target).closest(".slick-row");
  var rowNode = $rowNode[0];
  if (rowNode != rowNodeFromLastMouseWheelEvent) {

    var $gridCanvas = $rowNode.parents('.grid-canvas');
    var left = $gridCanvas.hasClass('grid-canvas-left');

    if (zombieRowNodeFromLastMouseWheelEvent && zombieRowNodeFromLastMouseWheelEvent[left? 0:1] != rowNode) {
      var zombieRow = zombieRowNodeFromLastMouseWheelEvent[left || zombieRowNodeFromLastMouseWheelEvent.length == 1? 0:1];
      zombieRow.parentElement.removeChild(zombieRow);

      zombieRowNodeFromLastMouseWheelEvent = null;
    }

I'm not sure why we can't use zombieRowNodeFromLastMouseWheelEvent directly, I'm aware that it's a jQuery object and that its contained node might have one of several canvas elements as a parent, depending on the frozen rows situation, but why would the jQuery object contain two elements? The row shouldn't be in both canvases, surely?
I actually suspect that this MacOS zombieRow bug may have been fixed anyway, it was from a long time ago, perhaps I'll check that out too.

This is a bit OT, it's not actually parrt of the PostRender functionality, it's just tied up with that code.

[ghiscoding]

@6pac
When I did the PR, I looked at the code before merging freeze feature as mentioned in top paragraph. I didn't investigate that much more, I just tried to merge the code and tested with a console log as shown on top as well.

Looking at previous version tag 2.3.23, it seems to be called at 2 different places here and here

[6pac]

I was asking specifically about the zombieRowNode code. But it looks like the original bug was resolved in 2017 so I think it's safe to remove all the zombie related code.

[ghiscoding]

oh ok, I personally never really understood what those zombie rows were for, so can't really answer on that side 😉

[JEDacreWright]

I'm the one who opened the issue @ghiscoding was talking about, so I check on this issue regulary. 🙂
I just wanted to thank you for your work on the subject. 👍

[6pac]

No worries @JE-DW, it'll be done soon, just need to find a few moments.

[6pac]

@ghiscoding OK I have posted two commits, the first to remove the zombieNode logic, and the second to reinstate the PostRenderCleanup. Can you run these through the test process without me doing a release? I've tested them using the examples and they behave fine there.

[ghiscoding]

@6pac the Cypress passed in here, the GitHub Actions runs on every commit, I can do similar tests (tonight) as I've done in first post

@JE-DW you can maybe help us by going in your node_modules/slickgrid and replace slick.grid.js with latest code that Ben just pushed (just copy the file from here and overwrite yours and then restart your App).

[JEDacreWright]

Ok I'll test this !
I'm answering on my issue as I use Slickgrid via Angular-Slickgrid: ghiscoding/Angular-Slickgrid#828
If you prefer any feedback here, tell me.

[JEDacreWright]

Ok @6pac and @ghiscoding after getting the right file (sorry 😬), it worked perfectly. No more lag, asyncPostRenderCleanup callback is now called. 👌

[ghiscoding]

@6pac
in addition to @JE-DW great testing, I also ran over 400 Cypress tests in 2 of my libs and they all pass. I only have 1 grid which uses async post renderer and it's all good, so I think we're all in the clear.

I assume we should close this PR then @6pac since it's not longer relevant?

[6pac]

Absolutely. Let me know if you want a release.

[6pac]

Was good to get rid of the zombieMouseWheel code too, now that the original bug is fixed it could conceivably have been causing performance or other issues.

[ghiscoding]

Well @JE-DW did mention much more responsiveness in this comment of Angular-Slickgrid.

Ooooh @ghiscoding it's so better !
I got both logs from asyncPostRenderCleanup callback and from ngOnDestroy.
I scrolled a lot in my data and didn't see any more lag (comparing with initial rendering). 👍

Thanks a lot Ben for fixing, another bug bites the dust 🚀