feat: some Date editor/filter improvements after new major version

[zewa666]

hey there.

I've been working today with Copy&Paste support for the new DateEditor and noticed a couple of issues:

1. default dataItemColumnValueExtractor will return always return null for active editor cells. That means if the editor is opened and you press CTRL+C it actually will copy null into the buffer
2. a macro task is not necessary, micro is sufficient for re-render during init, which helps with some potential issues down-the-line
3. pasting in a dateeditor cell leaves the editor opened -> also caused by render timings

So I needed a way to show this, hence I've decided to modify Example19. Not sure if we want to keep this but it's the easiest way to show the errors. If you try it out, just comment out the modifications but keep the Example19 as is to see the issues.

EDIT:
I'll make sure to cleanup comments and add tests/docs once we feel this is the way to go. There also might be a few more findings over the next couple of days as I go ahead. The CellExcelCopyManager is a huge beast with somewhat similar but than again different workflows compared to the normal editor experience, so there might still be some gotchas around but most of them are thankfully easily mitigated.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[ghiscoding]

oh that's cool, does that do 1 CPU cycle? Because that's typically why I use setTimeout

[zewa666]

the event loop consists of cyclic tasks. Every such task is performing one render action. If you say setTimeout(..., 100) it skips 100ms and puts your task onto the macrotask queue.

There is a priority lane though, called microtask queue, which is a happening within every macrotask.
So all promises are pushed towards the end of a macrotask and executed all together before the next macrotask starts. Even nested promises.

Hence why often an optimization when working with DOM Elements is to stuff things into microtasks and only render them once alltogether in the next macrotask

[ghiscoding]

BTW, after reading the article it seems that we could instead use queueMicrotask(f) which seems shorter, isn't it better?

[zewa666]

its the same but yeah more expressive 👍

[ghiscoding]

would there be a reason to NOT do that though? Do we really need to add this if it's expected to always do that? I remember that SlickGrid will first try to commit if an editor is opened before continuing, so I wonder if we need this extra arg?

[zewa666]

its a breaking change and we just had one recently ;)

besides, the reason for this could also be that it interfers with the original browser ctrl+c within the editor itself. e.g if it were a text editor and you merely select a part of it would you expect the part or the whole cell being copied?

[ghiscoding]

I still don't understand why that is a breaking change? As for the browser ctrl+c, I assume that we have a preventDefault on the even that will not bubble up to the browser (if we don't, then we really should) and so it shouldn't interfere with the browser one. I would assume that it copies the whole cell content

[zewa666]

not sure what you mean but the default ctrl+c can never happen if there is that slickCellExternalCopyManager.handleKeyDown in place that circumvents and overrides the clipboard. And yep it does behave this way with my mentioned feature. So no matter whether you're in the cell or the editor it will always copy the cells content via dataItemColumnValueExtractor. As such it definitely feels like a breaking change for me if introduced as default.

[ghiscoding]

well ok, it doesn't matter much to me since I'm barely using that feature, so go ahead if you want a flag.

[zewa666]

the amount of feature toggles is increasing indeed. 😅

[zewa666]

there will be one more feat that I'm gonna add tomorrow, transform value callback before pasting.

[ghiscoding]

there will be one more feat that I'm gonna add tomorrow, transform value callback before pasting.

isn't that what the dataItemColumnValueExtractor is for? Or actually I guess that's for the output but you're trying to target the input instead, is that it?

Also as a side note, I wouldn't mind if you do a project search and revisit all the use of setTimeout and tell me if some could be changed to your approach. I found this JS info event.loop article that describes the micro vs macro, I'm reading about it, it's interesting since I never heard of these terms before 😄

[zewa666]

there is this

slickgrid-universal/packages/common/src/extensions/slickCellExternalCopyManager.ts

Line 160 in 5f2ad5a

return this._addonOptions.dataItemColumnValueSetter(item, columnDef, value);

but as you see it early returns. so what I wanted to do is merely replace the value and continue with the original implementation if the else path. right now I simply had to duplicate that code.

so I thought of modifying the behavior so that one can control whether it should early exit or continue

[zewa666]

Also as a side note, I wouldn't mind if you do a project search and revisit all the use of setTimeout and tell me if some could be changed to your approach.

thats one of the hardest things in JS to debug 🤪 the trouble is that if somewhere in the loop one macro task is forcefully queued that makes the whole execution a big guessing game.

so I'd rather prefer going at those one by one once I come across one if them. the amount of potential side effects is otherwise too large

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (1889702) to head (bab4891).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1551     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21723   21727      +4     
  Branches     7271    7273      +2     
========================================
+ Hits        21662   21666      +4     
  Misses         61      61             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ghiscoding]

wow triple if, can we merge some of them? 😄

[zewa666]

absolutely. what do you think about the change in general? its kind of a small breaking change but I dont assume anyone returned true so far

[ghiscoding]

I searched the internet for SlickGrid and dataItemColumnValueSetter and couldn't find anything, I searched my own examples and I don't have any demo with it. So I seriously don't think anyone uses apart from you maybe haha. But what does it mean to "Return true if default logic should continue to evaluate"? Continue evaluating what and where? Do you have a demo of this?

I searched on SlickGrid core lib and found 1 example with it, the code is like below and the live demo is this Spreadsheet: features of the previous example but using a DataView

  function getVal(item, columnDef) {
    //return dataView.getItemById(item.id)[columnDef.field];
    return item[columnDef.field];
  }

  function setVal(item, columnDef, value) {
    item[columnDef.field] = value;
//    dataView.updateItem(item.id, item);
  }

  var options = {
    editable: true,
    enableAddRow: true,
    enableCellNavigation: true,
    asyncEditorLoading: false,
    autoEdit: false,
    dataItemColumnValueExtractor: getVal,
    dataItemColumnValueSetter: setVal
  };

[zewa666]

well if you look at the place where I've added the additional if clause, the return statement now is called as before but only if the return value isnt true, on which case it would continue with line 166. this gives me the ability to e.g format a pasted date like dd.mm.yyyy to yyyy-mm-dd so that the dateEditor is happy

[ghiscoding]

do you mean copyActiveEditorCell: true? because I don't see any usage of dataItemColumnValueSetter. I'm ok with the change in general, I don't see much of a breaking change and if it fixes your problem then the PR looks ok to me so far.

[zewa666]

no the last commit if (this._addonOptions.dataItemColumnValueSetter(item, columnDef, value) !== true) {

so if that now returns true the rest of the original method will continue, whereas before it just exited with the return

[zewa666]

ok I think I covered everything here and it works with my app specific tests. Didn't see the need to document this very specific feature toggle but if you prefer so let me know where you'd see it fit