Open GerHobbelt opened 9 years ago
I like your thinking on this, @GerHobbelt.
I spent the past few days looking closely at, and slowly merging/rewriting, the code responsible for managing pinned columns from a fork by @jlynch7.
I can see why @mleibman wouldn't want to merge this fork. It's big: includes 463 non-whitespace changes. There are changes that are not directly related to column pinning, though most are. Column pinning change necessarily touch a great deal of the code base, so it would be very difficult to make sure adding this feature didn't break others.
I think a change this big should involve a rewrite at some level--jlynch built it like a bolt on, but column pinning fundamentally changes the the cache storage mechanism, display mechanics, and event handling.
I'm looking at rebuilding this in as architecturally elegant a way as I can. I'm pretty set on removing pinned rows, I don't see the use case. I'm considering removing the virtualization of columns which, while it has perf benefit, doesn't have much for our use case or ones I think are likely. Also it's possible the overhead of the virtualization calculation could offset the benefit of virtualizing the very few columns that are off screen in our use cases.
There is a perf gain for virtualizing off-screen columns, but I wonder if this has real-world benefit that offsets the code complexity this introduces. I think most real-world cases aren't going to have "many" (>30) columns, only many rows. Would it be a big feature hit to remove this? reduces the complexity in some areas by 30% or more.
There's a fixed header, to which you can add a header row, paired with columns, and a topPanel, spanning all columns. are pinned rows needed in addition to this? I don't think so. There's some use for an iphone contacts style group headers that scroll with content and glue to the top until the next one comes by, but just pinning N rows... I want to see a use case.
Here are some key challenges I've seen with pinning N columns left. I bet we'll find more. Maybe we should maintain a list:
rowsCache
up to date.range
-- it knows the top and bottom index that should be drawn. The range also knows left and right pixel boundaries, and that's used to determine which cells to fill rowsCache[rowIdx].rowNode
with.rowsCache
should always draw cells <=
the pinned index, because we assume they're always on screenrowNodes
: a left dom element and a right dom element. Maybe this is the wrong structure, and the entry should hash cell markup by column index instead.rowsCache
object, we need to know where to put each of these dom elements. Some go left, some go right. If it's not pinned they all go left?Hi folks, this is not a feature I need, so I'm commenting from the sidelines, but if you're trying to emulate Excel, then rows and columns can be pinned, but I think it's limited to a blocked area on the top or left, basically for 'header' information.
I also think there needs to be a bit of an education campaign about pull requests. For various forks to be effectively maintained, it's absolutely essential that pull requests from the community be atomic and minimal, otherwise every one must be reviewed and possibly reworked. I suppose it's up to the gatekeeper(s) who currently have to do the review to pursue that.
I say that because I'm also maintaining a fork that implements quite a different DataSource and has major changes to the codebase. It's great that people contribute pull requests, but disappointing when I find that there are so many different changes wrapped in a given pull request that I have to pick them all apart before they are of any practical use.
Ben
@6pac: this is not going to be an atomic or minimal pull request. In fact, I doubt I'll do a pull request.
Column pinning touches so, so much--draw mechanics, yes, but also the cached dom element storage format, event binding, and on and on.
You show me how to do column pinning without a massive rewrite and I'll give you an enthusiastic high-five. By implementing it with fewer features enabled, there are actually fewer codebase changes to implement.
This is pretty much the fork of no return, as far as I am concerned. @mleibman appears to be abandonwaring the project anyway, and it is showing it's age (hard dependency on jQuery and support for browsers that don't matter any more).
I agree, sorry but you're misunderstanding what I said. I am in the same position as you. I also have done a 'fork of no return'. Not a pull request, but a fork. And I clearly agree that this is necessary for some purposes. What I'm saying is for anyone in the position of having done this, small patches from the community need to be clean and atomic, otherwise WE can't keep our work up to date.
I'm fascinated that you think this is old though - I regard it as the best, most cutting edge grid available. Not that I'm really up with these things. What would you use in preference ?
Does anyone understand what the whole -1000px
dance is in the slick-header
s? I'd like to use the normal l0 r0
style positioning for the column headers. This way pinned top areas can share the same canvas. This will work but I don't want to get rid of that without understanding why it was there.
StackO question: http://stackoverflow.com/questions/25172683/why-does-slickgrid-add-1000px-in-js-then-reverse-it-in-css-style
@6pac I think it's the bestest grid ever too.
But, it hasn't been touched in more than 6 months. From the maintainer:
UPDATE: March 5th, 2014 - I have too many things going on in my life right now to really give SlickGrid support and development the time and attention it deserves. I am not stopping it, but I will most likely be unresponsive for some time. Sorry.
The jquery dependency should go, as should the support for old browsers (lte IE8 or 9). Requiring a browser with box-sizing: border-box
would allow me to remove bunches of janky, slow code that's used to measure border, paddings, and margins.
We're wandering wildly OT here, but I like what you're saying. From what I understand, you basically want a HTML5 version of the grid. I plan on giving this grid a lot of attention in the next 6 months, but I'm busy with other (non web related) stuff for a little while still. While MLeibman has indeed had that for a while, I also think the grid has enough substance to stand on its own feet now. That is, it's sufficiently mature. As I mentioned, I have no-return forked this, and on order to keep things updated I was thinking to try to fork a branch off master and be the gatekeeper for that to apply small fixes etc to that 'alt master' and my no-return fork, perhaps effectively taking over maintenance. It would be good if the different major forks had some degree of co-ordination. Perhaps an HTML5 fork is yet another no-return fork. While I think it definitely could benefit from HTML5 and using the latest versions of jQuery and jQueryUI, how on earth could it break free of jQuery altogether (without a lot of pain ..) ? jQuery seems to me to be pretty much standard now, and it provides a lot of the UI magic for the grid. MLeibman says he 'drops into native javascript' for the speed benefit. I think it has hit the sweet spot.
This renaming was the major effort today. I felt I needed it in order to make sense of all the required dom elements. Does this read ok?
/*
## Visual Grid Components
To support pinned columns, we slice up the grid regions, and try to be very clear and consistent about the naming.
All UI region info objects start as an array with a left [0] and right [1] side
Dom elements are stored at the top level together (still in a left/right pair) because jquery has convenience methods for dealing with multiple elements. (eg: el.empty())
topViewport.width // combined width
topViewport[0].width // left width
topViewport.el // both els
topViewport.el[0] // left el
topViewport[0].el // left el, too? A: no. undefined. let's only have one reference.
*/
// [0] [1]
// ....................
var topViewport = [{},{}], // . . . // The scrolling region
topCanvas = [{},{}], // . . . // The full size of content (both off and on screen)
header = [{},{}], // . . . // The column headers
subHeader = [{},{}], // . . . // Optional row of cells below the column headers
// ....................
contentViewport = [{},{}], // . . . // The scrolling region for the grid rows
contentCanvas = [{},{}], // . . . // Full size of row content, both width and height
rows = [{},{}]; // . . . // Container for information about rows
// . . .
// . . .
// . . .
// . . .
// . . .
// ....................
Yup, it does read fine. :-)
Hi guys! I also needed of this feature and found the @jlynch7's work. However, he doesn't follow the main code format, but I applied his work on branch master keeping the format of the @mleibman. I implemented others features and refactored some codes appointed by codeclimate's analisys. Take a look: http://ddomingues.com/X-SlickGrid/liveDemo/examples/example-group-header.html. Follow my fork: https://github.com/ddomingues/X-SlickGrid#additional-features
Hi Guys! Don't worry: I have neither a pull request neither a non-return fork :-) I've read this thread with great interest because it means to me that SlickGrid might be still alive, after mleibman departure. As 6pac says, it's the best, most cutting edge grid available, period.
Juan Lanus
From which fork can i pull from to get the group column headers and ability to pin columns? It's great that you guys are coordinating...i feel like there should be two main forks..one such as the one 6-pac is maintaining and perhaps one that incorporates these major/big enhancement features. Hopefully after they have baked for a while it can be merged back into 6pac's or mbliebman's.
SimpleGy is working on a pinned columns implementation. There are several other older forks that have pinned column implementations he's using as inspiration. I share his opinion that the code changes are too sweeping to be able to be re-integrated back into the main trunk. But it's his baby, you'll have to ask him for a status update. I don't know anything about the active forks other than what I (and hence you also) can see on GitHub.
I've taken the opposite approach - trying to make it possible for essential small patches to be pushed out to divergent forks to keep them up to date. This is why I have posted the patch list. Maybe someday if someone wants so spend a month refactoring and integrating the pinned column code to the main branch in an elegant way, they could do it.
Following up on my msg to @SimpleAsCouldBe in #897, I think it's better to collect the considerations and work / progress info regarding this big issue in a separate issue -- and, yes, I know the issue tracker already has several issues regarding this subject, but all are specific as to a particular design or pull request.
@mleibman once wrote that he would not consider merging any of the forks that offer this type of functionality for a while as he had pondered the problem himself and concluded that none of the available solutions delivered sufficiently on the underlying issues. Regrettably I was never able to dig up his conclusive analysis and list of underlying issues. This is an occasion where I fear I've been retracing someone else's steps.
Let's answer @SimpleAscouldBe question, rephrased as: where are you guys at and what have you found out?