fix: SlickCellExternalCopyManager should use DataView

ghiscoding commented 3 months ago

while investigating for issue #1634, I found the problems shown below
so it's a bit surprising that we didn't catch this earlier but the plugin was only coded for SlickGrid usage to get read/write data and without the DataView, but Slickgrid-Universal is all built with the DataView usage only and so we should really fix this to support both
add a new DataWrapperService to make it easier to call the correct functions depending on our usage SlickGrid or SlickDataView

stackblitz[bot] commented 3 months ago

Run & review this pull request in StackBlitz Codeflow.

ghiscoding commented 3 months ago

cc @zewa666

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.8%. Comparing base (b231d89) to head (ffa5a08). Report is 11 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1646 +/- ## ======================================== + Coverage 99.8% 99.8% +0.1% ======================================== Files 198 199 +1 Lines 21849 21872 +23 Branches 7306 7319 +13 ======================================== + Hits 21786 21809 +23 + Misses 63 57 -6 - Partials 0 6 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zewa666 commented 3 months ago

the new wrapper might come in really handy. could you shortly elaborate on what the difference between the dataview vs old approach is?

ghiscoding commented 3 months ago

Welcome back @zewa666 😉

So the differences between SlickGrid and the SlickDataView is mainly that SlickGrid is very basic in data functionalities, it has functions to add/remove/select but that's about it (if you wanted to just create a simple grid to display things then just using SlickGrid was enough, i.e. this basic example). On the other hand, the original author also created an optional DataView to add more functionalities like Grouping & Aggregators, which SlickGrid itself doesn't have, plus all other indirect features (group/totals/nested groups). Other features that the DataView also has are custom Filter & Sort, local Pagination and probably more. Lastly, the theory is that the DataView was created like a plugin, any user could come and create it's own custom one and just swap it out whenever he wants, like the slick.remotemodel-yahoo.ts extends on this idea... so considering that the DataView has much more functionalities, including the Grouping & Aggregators, I decided from the start to only use the DataView in my projects on the expense of being a bit larger (it has 1800 lines of code), I never cared about the size but some people do (hence why some people still prefer to use the original project).

And finally below, is one of the issue that I found when investigating the other issue mentioned above, the grid.getData() returns the dataset when using plain SlickGrid but when we add a DataView (which it always does in our case because like I said above, I always add the DataView internally) and so grid.getData().length will probably throw because that doesn't exist in the DataView, then other calls like grid.getDataItem(i) will return nothing because the dataset is not in SlickGrid, it's rather in the DataView that we use, so there's about 10 of these calls that you can see in this PR.

As for the wrapper, I only added the ones that were used by this plugin and typically most user will never use or care about this but I prefer to keep the plugins in sync and to keep the grid functions similar to the original ones so that if I there are PRs on that side, then I could apply them on my side too and vice versa (for example the auto-scroll when dragging cell/row selection (this example), was added afterward by a SlickGrid user and so I took the code and applied it to my plugins too, thanks to a SlickGrid contributor)

I hope you don't mind long explanation 😉

https://github.com/ghiscoding/slickgrid-universal/blob/b231d8915beee1925298141d313e733ebaac2235/packages/common/src/extensions/slickCellExternalCopyManager.ts#L276-L281

zewa666 commented 3 months ago

awesome. could you perhaps release that as a (next) version? that way I could more easily give it a try in our app if you're up for some testdrive

ghiscoding commented 3 months ago

awesome. could you perhaps release that as a (next) version? that way I could more easily give it a try in our app if you're up for some testdrive

I'm going to do another release this weekend mostly because of the other PR #1644 since all my PRs on Vanilla-Calendar-Pro were accepted and merged (about a dozen of them), so I'm quite happy about that. There are also other small fixes in here and also in Angular-Slickgrid that are still unreleased, so yeah another release for next weekend is planned

ghiscoding / slickgrid-universal

fix: SlickCellExternalCopyManager should use DataView #1646

Codecov Report