More formulas and easier formula editing

[jkirschner]

Important Changes

• More Formulas: Replaced x-spreadsheet's custom formula parser with the MIT licensed' hot-formula-parser based on a fork of formula.js. This makes most formulas available in Excel also available in x-spreadsheet.
• Easier Formula Editing: When editing a formula, click to select a cell OR click and drag to select a range. Single cell and range selections can then be modified with arrow keys with or without the SHIFT key to respectively expand/shrink or move the selection.
• Absolute Cell References: Absolute cell references are explicitly supported, including when a formula cell is copied by clicking and dragging the anchor in the bottom-right corner.
• Translation Fallback: If a translation is attempted but no associated key exists in the primary locale, the application can use fallback locale(s). The default fallback locale is English but this can be overridden when specifying a locale to use.

Known Limitations

I recommend accepting this PR while acknowledging there is room for improvement. These improvements are all limitations of formula.js rather than of its integration into x-spreadsheet:

• Some formulas in formula.js don't have the same flexibility as they do in Excel. For example, in Excel, COUNTIF(A1:A5, "a") returns how many cells in that range contain the letter "a" (with the asterisk allowing any number of characters before or after "a"). But in formula.js, regex characters like "*" don't work. Instead, formula.js will return how many cells in that range have the exact value "a".

Merge Notes This PR includes commits from a different fork for which a PR was also submitted: https://github.com/myliang/x-spreadsheet/pull/309

If this PR is accepted and merged, PR 309 should be closed without merging.

Known Resolved Issues #362 - more formulas #320 - more formulas #280 - only difference being that it's just click, not shift click

Note: I can only understand the issues written in my native language (English), so there may be others I am missing.

[codecov[bot]]

Codecov Report

Merging #363 (8a2f8ff) into master (549f5cf) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 8a2f8ff differs from pull request most recent head 597b095. Consider uploading reports for the commit 597b095 to get more accurate results

@@      Coverage Diff      @@
##   master   #363   +/-   ##
=============================
=============================

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 549f5cf...597b095. Read the comment docs.

[jkirschner]

Merge conflict introduced by recent update to master has been resolved

[jkirschner]

@myliang and team: ready for your consideration

[jkirschner]

I see that my PR currently fails the code coverage check. I'm wondering whether this was mostly caused by removing the formula tests (since, in this PR, formula parsing/calculation is now handled by library code rather than x-spreadsheet). I'll see if I can increase the number of tests in general, but I only feel comfortable writing tests for code I touched (and I'm not sure that will bring the test percentage up enough to pass the check).

[jkirschner]

@myliang : I have improved the test coverage since my last comment. alphabet.js now has all functions covered, and I added tests for locale.js (which previously didn't have them).

Please review at your earliest convenience.

[jkirschner]

@myliang : is there anything I can do to facilitate a review by you or another project maintainer? I don't want to continue with additional changes until this PR has been reviewed, approved, and merged.

[myliang]

too big changes will not be accepted.

[jkirschner]

@myliang : I am happy to split this into several smaller PRs:

• chieveit's original formula editing improvements -> #382
• npm run test fix -> #380
• locale improvements -> #381
• formula.js conversion
• click and drag cell range selection

Would that be acceptable?

[jkirschner]

@myliang : I've separated out some of the functionality into smaller PRs:

• npm run test fix -> #380
• locale improvements -> #381

Once those are reviewed, approved, and merged, I'll create small PRs for the other functionality included in this large PR.

[jkirschner]

@myliang : thanks for the review on 380 and 381. Another smaller PR is now ready for your review:

• chieveit's formula editing improvements -> #382

Once that is reviewed, approved, and merged, I'll move on to creating a PR for formula.js integration.

[mm738]

On this branch: https://github.com/jkirschner/x-spreadsheet/tree/formulajs-range-selection

@myliang and @jkirschner this is a great feature update!

This makes it more like how Excel works. Another good feature is dragging and dropping cells from one place to another. Very common feature.

Thanks.

[jkirschner]

Thanks @mm738. Trying to work with the maintainers to get this integrated upstream in parts (since this PR was too large). So far we are 2/5 of the way there!

[harlan3]

jkirschner this is a great improvement wrt formula calculations within the worksheet. With the old implementation, simple calculations resulted in erroneous values. However, your formulajs branch in its current state breaks document printing. The references to "this" formulaParser within the renderCell function call are invalid in src/component/table.js

[ThibautSF]

I report a possible bug for @jkirschner here (hope this is the place) On this branch: https://github.com/jkirschner/x-spreadsheet/tree/formulajs-range-selection

If I use the formula selector, any formula will write "=undefined()" in the cell instead of the correct formula.

Note that it's still possible to select formulas from the cell completion suggestions.

[pbadenski]

This is a very powerful feature and I'm sad to see is not progressing - what can be done to help?

[harlan3]

I believe the project owner is not interested in incorporating this functionality unfortunately.

In my opinion, it is a huge improvement. It does break some things like printing that needs to be addressed.

Best regards, Harlan

---

From: Pawel Badenski [email protected] Sent: Monday, February 15, 2021 1:03 PM To: myliang/x-spreadsheet [email protected] Cc: harlan3 [email protected]; Comment [email protected] Subject: Re: [myliang/x-spreadsheet] More formulas and easier formula editing (#363)

This is a very powerful feature and I'm sad to see is not progressing - what can be done to help?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/myliang/x-spreadsheet/pull/363#issuecomment-779378415, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AF3GHP7ZPFPTIFPQOWZXIW3S7FOXTANCNFSM4RF43UUA.

[jkirschner]

@pbadenski, @harlan3 : I started breaking this large PR up into smaller PRs upon request by the project owner. One of these smaller PRs is still awaiting review/approval/merge: https://github.com/myliang/x-spreadsheet/pull/382. Perhaps the next step is getting that PR through.

Unfortunately, I have a lot less time now than I did when I originally created this PR. I can't push it forward right now (e.g., fix the printing bug), but I do want to come back to it eventually.

[jkirschner]

@pbadenski, @harlan3, @ThibautSF, @myliang : I have some time over the next week to push this PR forward and hopefully get it through the review process. My understanding from the maintainer is that it needs to be broken up into smaller PRs which can then be individually reviewed and merged. I'm partway through that process: the latest PRs to review are #382, #464, #465, and #466.

[jkirschner]

@harlan3 : I've fixed the issue with document printing.

[harlan3]

@harlan3 : I've fixed the issue with document printing.

Great, can't wait to try it out once everything has been incorporated to master.

[ThibautSF]

Hi @jkirschner

Another bug from your branch.

When a user edit a cell and try to add multiple spaces (ie "test____" (4 space)) to input only one space is taken into account thus become (ie "test ") -> Looks like to be the behaviour of x-spreadsheet (I don't know if it is intentional) BUT if you type another character after a trailing, the trailing space is removed and replaced by the new character (this explains why only one space in taken into account). So if we try to enter "a b" it will become "ab" -> this part is working in x-spreadsheet current branch

I made some investigation and I saw that if I removed the following line (this.render() from inputEventHandler()) in editor.js (added in commit https://github.com/jkirschner/x-spreadsheet/commit/ee61ed62a100ed41632084550bc276b2ba499df7) I was able to enter spaces and characters without problem (even multiple trailing spaces)

[jkirschner]

Thanks @ThibautSF. I'm not super familiar with that commit because it was written by another user (chieveit), so I appreciate your investigation and will look into it soon.

[MartinDawson]

@jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

[harlan3]

@jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

I have been using the @jkirschner branch for several months now (along with some additional improvements for loading and saving files) and I would say it is vastly superior in comparison to the x-spreadsheet that doesn't have smart formula parsing. I have used it with spreadsheets spanning several hundred cells and it does slow down some but for a "normal" sized spreadsheet it is fine. If you have something better then you should submit it back to git master and the code should fall under the MIT license as the original project uses the MIT. It isn't right that you leverage all the code to x-spreadsheet and then not contribute your work back to the project. That is completely unethical. I am perfectly happy using the @jkirschner branch. It is just unfortunate that it has not been promoted into the git master yet.

[ThibautSF]

And just at a UX level, this PR is way better. Formula edition alongside absolute cell reference gives a better end-user experience, near enough of what you can expect on a desktop sheet and simple enough for a integrated web use (didn't find anything better for now in other open source sheet libraries).

X-spreadsheet is really good, this PR made it just better. It sure has drawbacks in case of big sheets, but those can be solved with code review, suggestions and improvements.

[MartinDawson]

I can’t. The solution uses a paid package which is not MIT to do the calculations. Hot formula parser is not maintained anymore

Also, it’s a complete rewrite so it’s never going to be approved by the maintainer

On Fri, 28 May 2021 at 22:55, harlan3 @.***> wrote:

@jkirschner https://github.com/jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

I have been using the @jkirschner https://github.com/jkirschner branch for several months now (along with some additional improvements for loading and saving files) and I would say it is vastly superior in comparison to the x-spreadsheet that doesn't have smart formula parsing. I have used it with spreadsheets spanning several hundred cells and it does slow down some but for a "normal" sized spreadsheet it is fine. If you have something better then you should submit it back to git master and the code should fall under the MIT license as the original project uses the MIT. It isn't right that you leverage all the code to x-spreadsheet and then not contribute your work back to the project. That is completely unethical. I am perfectly happy using the @jkirschner https://github.com/jkirschner branch. It is just unfortunate that it has not been promoted into the git master yet.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/myliang/x-spreadsheet/pull/363#issuecomment-850694400, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSVRWZCXP6IJTFVXYX3GFTTQAGNPANCNFSM4RF43UUA .

[jkirschner]

@jkirschner Thanks for the effort but I've tested this PR a lot and there fundamental issues with it that cannot be solved easily. Just saying cause don't want you to waste your time.

1. x-datasheet doesn't handle smart formula parsing. I.e it loops through all cells in a linear order, it does not build any graphs. This means when changing cell dependents it wont update cells in the correct order. I'm not sure if hot-formula parser does build the graph correctly but when I tested this PR it was very laggy.
2. The parsing of the formulas in this PR is done on every render of the cell. This isn't efficient. It should be done once and cached and not on every render.

Due to the above issues the spreadsheets lags with any reasonable amount of formulas or circle references.

@MartinDawson : Can you help me understand how either of those concerns is related to this PR? If I understand correctly, (1) looping through all cells in a linear order and (2) parsing formulas on every render of the cell were both behaviors of x-spreadsheet before this PR. This PR changes the parsing engine for formulas, but not when parsing is performed.

Yes, improvements to efficiency can be made, but those should be done in a separate PR.

[jkirschner]

@harlan3, @ThibautSF: because of @MartinDawson 's comments, I put together a proof-of-concept that builds on this PR to (1) perform efficient recalculations and (2) detect circular references: https://github.com/jkirschner/x-spreadsheet/tree/efficient-recalculation

Let me know if you try that branch out and see a significant enough performance improvement that it's worth me finishing the branch. I still need to make some small fixes (such as to print mode) and significantly cleanup the commit history via rebasing, so I wouldn't use the efficient-recalculation branch yet... but it's ready to try out!

[harlan3]

@harlan3, @ThibautSF: because of @MartinDawson 's comments, I put together a proof-of-concept that builds on this PR to (1) perform efficient recalculations and (2) detect circular references: https://github.com/jkirschner/x-spreadsheet/tree/efficient-recalculation

Let me know if you try that branch out and see a significant enough performance improvement that it's worth me finishing the branch. I still need to make some small fixes (such as to print mode) and significantly cleanup the commit history via rebasing, so I wouldn't use the efficient-recalculation branch yet... but it's ready to try out!

I tried it with some minimal testing, and it seemed to work ok. To really test it I would need to merge in my spreadsheet file loading and saving capability which would allow the loading of large x-spreadsheet files. If you are interested in putting the file loading and saving functionality in your main baseline let me know and I will create a branch for a PR.

[jkirschner]

I tried it with some minimal testing, and it seemed to work ok. To really test it I would need to merge in my spreadsheet file loading and saving capability which would allow the loading of large x-spreadsheet files. If you are interested in putting the file loading and saving functionality in your main baseline let me know and I will create a branch for a PR.

@harlan3 : I probably won't merge any PRs, including my own, into my fork's main branch because I'm trying to keep that consistent with upstream - which relies on getting things merged upstream. If you submit a PR against the main branch, I'll probably take those commits and place them on top of efficient-recalculation to create a new branch that you can test in. Or, you could create a PR against efficient-recalculation yourself!

It's worth doing that so we can test the performance improvements and see if it's worth pursuing further. Thanks!