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

feat: add final rowspan implementation #1101

Merged
merged 6 commits into from
Jan 18, 2025
Merged

feat: add final rowspan implementation #1101

merged 6 commits into from
Jan 18, 2025

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Jan 18, 2025

closes #614
closes #381

So I've been working over a month on this, initially based on the @GerHobbelt's fork from his initial rowspan commit and after a few bug fixes, I was surprised to see it working at least for the most part but it had particular negative side effects that made me rethink if I could improve it further. I had a few problems with that first approach and they are mostly all related to each other:

  1. it required to loop through the entire dataset to build a 2D array of every cells of that entire dataset with a 2D array that keeps [[row, cell, colspan, rowspan]]
    • it had a few nested for loops and I got a little concerned when I saw a for loop going down 3 level deep (scary😨)
  2. this 2D array mapping was being built for every grid regardless if you were using rowspan or not
  3. and finally it required to keep this large 2D array in memory (again for every grid)

So I decided to see if I could do it in a different way that would require less of a footprint (no 2D array) and less intrusive (it shouldn't impact regular grids perf at all) and I came up with this new approach. Instead of creating this large 2D array, what if we keep only the start/end indexes of each rowspan but instead of saving them by rows, we keep them by column indexes (basically the inverse of item metadata because that is row based)... and that is what I went with, the perf is quite good (about twice as fast as the first approach above). Note that this 2nd approach still does require to loop through all dataset rows to extract every existing rowspan from all item metadata but it does it in a much quicker way. For example, let's say that on the 1st column from the 2nd row, we have a rowspan of 5 then the mapping that we'll keep internally would be { 0: '1:6' }... great but what if on top of the rowspan, we also have a colspan of 2 on the same cell? Then our mapping becomes { 0: '1:6', 1: '1:6' } (basically duplicating the same start/end on the 2nd column as well). Also another concept is that if any cell has a rowspan child that becomes out of the viewport, we need to keep its parent rendered in the DOM at least until none of it is no longer shown at all (because the parent cell, is the cell holding the height for the entire rowspan). Then when if its the cell parent & children no longer intersect in the viewport then we can remove it from our cache (basically this will keep certain data rows a little longer in the DOM but that is of course necessary for the rowspan to work properly, i.e. our example has a rowspan that spreads across the grid until the bottom, so that parent row is basically never being removed from the cache because that one in particular must be kept for it to show correctly)

So why is this new approach better than the first approach? Well this approach takes a lot less space in memory and if our rowspan is very large, let say that it spans over 100 rows, then the mapping is nearly the same and we just need to update our ending for example { 0: '5:105' } (6th row, spanning for 100 rows, so 100+5=>105 is the ending)... simple! The other bonus is that with this new approach, I do have to keep a mapping object but it's relatively small and the bonus is that I've put this under a grid option flag enableRowSpan so that regular grids have 0 perf impact as opposed to the previous approach that impacted all grids (but just to be clear though, the perf impact wasn't that huge, for 500K rows it was around 300-400ms to build mapping for the 1st approach and around 100-200ms for new approach and also note that it was assuming only about 10 rowspan defined which is what the example demo has, so you can expect a little longer time spent on grids with a lot more rowspan).

image

image

@ghiscoding ghiscoding changed the title Feat/row span feat: add final rowspan implementation Jan 18, 2025
@ghiscoding ghiscoding merged commit 2e65fa8 into master Jan 18, 2025
2 checks passed
@ghiscoding ghiscoding deleted the feat/row-span branch January 18, 2025 20:05
@6pac
Copy link
Owner

6pac commented Jan 27, 2025

Interesting. I'm also in the process of working on a variable row height feature which approaches things in a not dissimilar way.

@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Jan 27, 2025

@6pac well actually that first commit from GerHobbelt's fork also included variable height and it was working for the most part... but I removed it because it brings a lot of different problems. The biggest challenge with rowspan was actually Cell Navigation (goto left, go 1 up, go 1 down, ...) and Scroll to Row or Cell X or Y. When there's no rowspan involved then saying "Go to last row" is easy, you just take dataset length and multiply by the row height to find your X coordinate and you're done. When you add rowspan then you need to do some more calculation of where exactly your position will be depending on not just the dataset length but also the height of the rowspan... if you then add variable row height then you bring a whole lot more complexity. For the rowspan, I added a new rowspan cache of some kind which loops through all row item metadata to get the full mapping of all rowspan, and if you wanted to add variable row height then I guess you could also include the variable row height (since we're already looping through all rows for rowspan mapping) of each of these one that do not have the default row height.

So if you go on GerHobbelt's fork and open his Example 31 with rowspan, it also includes variable row height on this line but like I said above, the challenge is the scroll to X row and cell navigation and that is actually not behaving correctly on his fork too... so is it doable? Probably but you'll have to consider all of these cell navigations and calculations and every else around that.

Note that I added a bunch of new cell navigation shortcuts in recent PR #1093 (i.e.: Ctrl+End will go to last row and last cell of that row, Ctrl+Home will go to coordinate 0,0, Ctrl+DownArrow will go on last row of same active column, ...), so all of these new cell navigations and shortcuts were really hard to get them all right which such new feature like colspan/rowspan... In summary, the hardest part of the variable row height wouldn't be how to make it work, but rather all of the other side effect that you didn't think about that also really have to work! Also remember that when you edit, the default behavior is to go to next cell down and that is a cell navigation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there an example of cell merging? Rowspan
2 participants