Freeze header row functionality

[quadrophobiac]

opening a PR to see if I am on the right path with this or in case I'm missing an easy trick. I have made the minimum changes required to freeze a row for scrolling purposes. I've left it open for a user to freeze multiple rows even though a CSV headers approach would prefer for this not to be the case. As far as I can tell Electron doesn't support dynamics changes to menu items which would fit with a toggled 'freeze row / unfreeze row' addition to both menu items. @chris48s I'm blocked on the inclusion of these functions in the main process menu. Basically it seems like nothing is firing between main and renderer. Was wondering if you could take a glance at the code and tell me if I am doing anything glaringly wrong or if there is a better way of writing code the communicates between the main and renderer process in Electron nowadays

[quadrophobiac]

for fix to #148 sans the header-dialect functionality

[Stephen-Gates]

Should be able to toggle menu value. Implemented in Atom

[chris48s]

Really quick off-the-cuff response. I've not checked out the code and run it, but I think the piece of the jigsaw you are missing is that you haven't defined handlers for the freezeRows and unfreeze events.

If you add a few lines to https://github.com/theodi/comma-chameleon/blob/master/renderer/index.js declaring handlers like

ipc.on('freezeRows', function() {
  // do the stuff
});

..that should get you moving :)

[quadrophobiac]

Thank you for the pointer @chris48s , still reacclimatising to Electron environment, that's unblocked it!

[quadrophobiac]

This is basically the bare bones functionality to freeze the header. What could be improved in subsequent patches are...

• [ ] colour coding of rows frozen
• [ ] consolidation of freeze and unfreeze functions into a single function that toggle in the relevant Electron menus
• [ ] CSV Dialect Support indicated in #148

Additions suffice to add mvp user functionality

[chris48s]

This seems to behave differently depending on whether you drag a selection down from the top or up from the bottom. For example, here I've dragged the selection down from the top:

I'd expect that to freeze the top 4 rows, but it does nothing. I see that is because the 'selected' cell is in the top row, but I think this is still a bit counter-intuitive (is this just me that expects this behaviour?). Would an easier way to make this clearer be to only work with a single row selection? If others think the behaviour I'm expecting here is strange, feel free to ignore this point.

Also sometimes it freezes just the row numbers, not the whole row, like this:

.. but I can't quite work out the steps to reproduce that. I would expect that to do this:

Also a few more minor observations:

1. If you click unfreeze on the right-click menu, you get 'stuck' in a select. I think unfreeze() should call hot.deselectCell(); at the end of the function to prevent this.
2. The operations in the Edit menu and right-click menu are called 'Freeze Rows' and 'Freeze row(s) above' which makes it sound like they do might do slightly different things but I think they do the exact same thing.
3. Can the events be called freezeRows and unfreezeRows or freeze and unfreeze for consistency.

[quadrophobiac]

@chris48s I made the suggestions, with more detail here. To get the row to freeze consistently I had to kludge it though - calling updateSettings twice. I'm not sure if that's necessary or if my understanding of javascript is leading to a very poor solution. However when I tested the callbacks [via script from here https://docs.handsontable.com/0.16.1/tutorial-callbacks.html] I could see that the re-rendering was executing prior to the fixedRowsTop value being altered. I'm inclined to think that this might be a bug that could be fixed in subsequent versions of HandsOnTable but even still I'm not certain about incorporating the kuldge to codebase - thoughts?

[chris48s]

I've got several bits of time-consuming life admin going on at the moment so I'm going to aim to look at this on Monday evening next week for you. If that's blocking you, happy to defer to @pezholio or @Floppy 's reckons :)

[chris48s]

Managed to slot in a bit of time to look at this tonight for you. All the odd behaviours I reported in my previous post seem to be fixed now, so that's useful.

There's one minor problem: The right-click menu item works but the 'Freeze Header Row' in the Edit menu item doesn't. It is firing an event called freeze but the registered handler is expecting an event called freezeRows. I've left a line note where it needs to be changed.

To get the row to freeze consistently I had to kludge it though - calling updateSettings twice. I'm not sure if that's necessary or if my understanding of javascript is leading to a very poor solution

Yeah - I can see the issue you were having. I'm also not entirely sure if this is intended behaviour for HoT or a bug we need to work around, but I think there is a neater way to ensure the re-rendering occurs after the setting are applied. Define a afterUpdateSettings handler when we pass the initial config to the Handsontable constructor, like this:

...
afterUpdateSettings: function() {
  hot.render();
  hot.deselectCell();
},
...

I've also added a line note where that declaration should live. Then that allows you to simplify your event handlers to:

var unfreezeHeaderRow = function(){
    hot.updateSettings({fixedRowsTop: 0, colHeaders: true});
};

var freezeHeaderRow = function(){
    hot.updateSettings({fixedRowsTop: 1}, false);
};

and deal with the cleanup in the handler function (which is guaranteed to execute after the settings have been applied). Maybe also note in issue #159 that we might be able to remove that manual call to render() when we upgrade the library.

[quadrophobiac]

Hey @chris48s , thanks so much for taking the time to give me solid js pointers on how to achieve the requisite functionality in a more cohesive way really appreciate the tuition you're imparting! And aside from personal gratitude I'd also like to thank you because having your insight on hand really helps the work that can be done on Comma Chameleon during present circumstances - so thanks from the Toolbox too :)

[chris48s]

no probs - glad to help :)

[quadrophobiac]

Cool, so I'm taking your input as a code review @chris48s and I'm going to merge this in now