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

More formulas and easier formula editing #363

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jkirschner
Copy link

@jkirschner jkirschner commented Sep 10, 2020

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.

image

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: #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
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

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

❗ Current head 8a2f8ff differs from pull request most recent head 597b095. Consider uploading reports for the commit 597b095 to get more accurate results
Impacted file tree graph

@@      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 jkirschner marked this pull request as draft September 10, 2020 22:05
@jkirschner
Copy link
Author

Merge conflict introduced by recent update to master has been resolved

@jkirschner jkirschner marked this pull request as ready for review September 17, 2020 14:03
@jkirschner
Copy link
Author

@myliang and team: ready for your consideration

@jkirschner
Copy link
Author

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 jkirschner marked this pull request as draft September 28, 2020 02:23
@jkirschner jkirschner force-pushed the formulajs-range-selection branch 2 times, most recently from c25618d to b70c5f9 Compare September 28, 2020 02:44
@jkirschner jkirschner marked this pull request as ready for review September 28, 2020 02:46
@jkirschner
Copy link
Author

@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
Copy link
Author

@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
Copy link
Owner

myliang commented Oct 2, 2020

too big changes will not be accepted.

@jkirschner
Copy link
Author

jkirschner commented Oct 2, 2020

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

Would that be acceptable?

@jkirschner
Copy link
Author

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

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

@jkirschner
Copy link
Author

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

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

@mm738
Copy link

mm738 commented Oct 16, 2020

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
Copy link
Author

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
Copy link

harlan3 commented Oct 16, 2020

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
Copy link

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.
image

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

@pbadenski
Copy link

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

@harlan3
Copy link

harlan3 commented Feb 15, 2021 via email

@jkirschner
Copy link
Author

@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: #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 jkirschner marked this pull request as draft May 13, 2021 15:17
@jkirschner
Copy link
Author

@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.

@harlan3
Copy link

harlan3 commented May 28, 2021

@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
Copy link

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
Copy link

MartinDawson commented May 29, 2021 via email

Jared Kirschner and others added 8 commits May 29, 2021 20:40
Replace existing formula parsing and execution with an external parser library
built on top of formula.js: https://github.com/handsontable/formula-parser

This dramatically increases the number of supported formulas; see list here:
https://formulajs.info/functions/
With the inclusion of many formulas from formula.js, the number of formula
options is now too great to display without a scrollable container.
Formula keys within locale files should match the name of the formulajs
function that a translation string is being provided for.

If a formula name has a '.' in it, it should be escaped as follows:
{
	"FORMULA\\.NAME": "TRANSLATION"
}
Absolute cell references are evaluated correctly.

When dragging the bottom-right corner of a formula cell to copy its contents to
other cells, absolute cell references will be incremented or decremented
appropriately (only relative axes will be modified, not absolute).

When adding or removing rows or columns, absolute cell references will be
adjusted appropriately.
The initial click sets the cell reference range start position.

If the click is held, the cell reference range end position is updated on mouse
move. The cell reference range can then be modified in the same ways as a
single cell reference (e.g., arrow keys).
If the user is editing a cell reference range within a formula and is holding
the shift key while using the direction arrows to move the range, the start
of the range will be fixed.

This mirrors the behavior of Excel when editing a cell reference range.
@jkirschner
Copy link
Author

@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
Copy link
Author

@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
Copy link

harlan3 commented Jul 5, 2021

@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
Copy link
Author

jkirschner commented Jul 6, 2021

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!

@harlan3
Copy link

harlan3 commented Jul 7, 2021

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!

I created a fork of your x-spreadsheet repo and merged in the changes for file loading and saving in my efficient-recalculation branch. You can clone it from here: https://github.com/harlan3/x-spreadsheet/tree/efficient-recalculation
To use the new feature, click on the "pen" icon which is on the far right of the toolbar. This toggles between the spreadsheet view and the JSON view of the table. To load the spreadsheet, copy and paste the contents of one of the files in the new sample_files directory such as loan_calc.txt. It is a pretty large file and I think your performance changes in this branch have helped out considerably.

@vipulism
Copy link

vipulism commented Jul 12, 2021

@jkirschner @harlan3 : is there any way to get parsed value programmatic ? likecellParsedValue(2,6)

@ThibautSF
Copy link

ThibautSF commented Jul 12, 2021

@jkirschner @harlan3 : is there any way to get parsed value programmatic ? likecellParsedValue(2,6)

I made this function (based on x-spreadsheet code and using the xspreadsheet cell(rowIndex, colIndex, sheetIndex) method) in order to get parsed value for any cell.

function getCellValue(xspreadsheet, rowIndex, colIndex, sheetIndex) {
    if (xspreadsheet) {
        if ((cell = xspreadsheet.cell(rowIndex, colIndex, sheetIndex))) {
            var cellText = cell.text;

            if (cellText && cellText[0] == "=") {
                cellText = xspreadsheet.sheet.table.formulaParser.parse(
                    cellText.slice(1)
                );

                cellText = cellText.result;
            }

            return Number(cellText) || cellText;
        }
    }
}

Note: xspreadsheet is the xspreadsheet instance object.

@smazydev
Copy link

smazydev commented Oct 5, 2021

This should've been merged a long time ago, such splendid work!

@harlan3
Copy link

harlan3 commented Oct 5, 2021 via email

@Tardo
Copy link

Tardo commented May 19, 2022

This works fine... can you rebase and resolve the conflicts? We can supersed if not :)

@jkirschner
Copy link
Author

Hi @Tardo,

The maintainer previously said they would not accept this PR as is because it was too large. That's very understandable, so I started the process of breaking it into many smaller PRs. The first few sub-PRs were merged. But the next few sub-PRs have not been reviewed. And there's a lot of work in this branch that hasn't made it into a sub-PR yet, because I was waiting for the existing sub-PRs to be merged.

A lot of time has since passed.

I cannot justify spending more time on this unless the maintainer expresses interest in this PR. And if that happens, I'm not sure I even have the bandwidth to pick this back up right now.

I really appreciate the feedback and collaboration I've had with the community members in this thread. I wish you the best of luck with your spreadsheets :)

@harlan3
Copy link

harlan3 commented May 23, 2022 via email

@Tardo
Copy link

Tardo commented May 24, 2022

@jkirschner Thank you for your response and the work made here. Too bad the creator doesn't want to review/merge the PR... what matters are the commits :/ It's good to know how eager the maintainer is to include improvements in his repo...

@MartinDawson
Copy link

MartinDawson commented May 24, 2022

Guys, just use this canvas spreadsheet: https://github.com/glideapps/glide-data-grid

It's far better and not got 10,000 bugs in it.

And just use hyperformula as your cell engine parser

@myliang myliang marked this pull request as ready for review May 30, 2022 01:20
@WeTruck
Copy link

WeTruck commented Sep 17, 2022

@smazydev how can I use xspreadsheet.js file from this pull request? Please help.

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.