gets Sheet name for multi cell named range, fixes #680 #700

2017-06-19T12:52:48Z

fzumstein commented

2017-06-19 12:52:48 +00:00

(Migrated from github.com)

Please have a look, this seems to resolve the issue for us.
Do you want us to add a test case or do you prefer to do that yourself?

Please have a look, this seems to resolve the issue for us. Do you want us to add a test case or do you prefer to do that yourself?

SheetJSDev commented

2017-06-19 14:03:59 +00:00

(Migrated from github.com)

This doesn't seem to work on all of the files from the test corpus.
After the line in question, add a line that throws an error if the field is missing:

				sname = (supbooks && supbooks[1] ? supbooks[1][ixti+1] : supbooks.SheetNames[ixti]);
				if(!sname) throw new Error("missing name"); // <-- add this line
				stack.push(sname + "!" + encode_range((r/*:any*/)));

To check against the test corpus, you can run the following command:

$ env WTF=1 FMTS=xlsb make test

(The WTF=1 part throws errors that would otherwise be caught and FMTS=xlsb restricts to reading only XLSB files.)

The first fail case is test_files/2013/apachepoi_50939.xls.xlsb, tripping on the newly-added error.

This doesn't seem to work on all of the files from the test corpus. After the line in question, add a line that throws an error if the field is missing: ```js sname = (supbooks && supbooks[1] ? supbooks[1][ixti+1] : supbooks.SheetNames[ixti]); if(!sname) throw new Error("missing name"); // <-- add this line stack.push(sname + "!" + encode_range((r/*:any*/))); ``` To check against the test corpus, you can run the following command: ```bash $ env WTF=1 FMTS=xlsb make test ``` (The `WTF=1` part throws errors that would otherwise be caught and `FMTS=xlsb` restricts to reading only XLSB files.) The first fail case is [`test_files/2013/apachepoi_50939.xls.xlsb`](https://github.com/SheetJS/test_files/blob/master/2013/apachepoi_50939.xls.xlsb?raw=true), tripping on the newly-added error.

fzumstein commented

2017-06-25 14:52:46 +00:00

(Migrated from github.com)

Hi, sorry for the late reply. I've fixed the commit to fall back again to **MISSING**.

It still fails somewhere with the if(!sname) throw new Error("missing name"); line, but that is the same behaviour as on master, so I don't think that's being caused by my changes.

Let me know know what you think. Thanks!

Hi, sorry for the late reply. I've fixed the commit to fall back again to `**MISSING**`. It still fails somewhere with the `if(!sname) throw new Error("missing name");` line, but that is the same behaviour as on `master`, so I don't think that's being caused by my changes. Let me know know what you think. Thanks!

fzumstein commented

2017-07-06 13:36:47 +00:00

(Migrated from github.com)

@SheetJSDev have you been able to look at my fixed commit? see previous comment. thanks for your feedback!

SheetJSDev commented

2017-07-06 18:26:07 +00:00

(Migrated from github.com)

I wasn't saying your commit wasn't useful for your case, but if we're going to change the function processing it's better to fixit in one fell swoop :)

Looked into this a little bit, https://github.com/SheetJS/js-xlsx/issues/680#issuecomment-307869085 was my initial brain dump on the matter.

The XLSB workbook processor is missing a few records like:

0x0165: /* 'BrtSupSelf' */
0x0166: /* 'BrtSupSame' */
0x0163: /* 'BrtSupBookSrc' */
0x029B: /* 'BrtSupAddin' */
0x016A: /* 'BrtExternSheet' */
0x0169: /* 'BrtPlaceholderName' */

After adding some initial implementation, we ran into a few more random cases like https://github.com/SheetJS/test_files/blob/master/2013/apachepoi_ex45978-extraLinkTableSheets.xls.xlsb?raw=true . I suspect the external links files need to be processed.

I wasn't saying your commit wasn't useful for your case, but if we're going to change the function processing it's better to fixit in one fell swoop :) Looked into this a little bit, https://github.com/SheetJS/js-xlsx/issues/680#issuecomment-307869085 was my initial brain dump on the matter. The XLSB workbook processor is missing a few records like: ``` 0x0165: /* 'BrtSupSelf' */ 0x0166: /* 'BrtSupSame' */ 0x0163: /* 'BrtSupBookSrc' */ 0x029B: /* 'BrtSupAddin' */ 0x016A: /* 'BrtExternSheet' */ 0x0169: /* 'BrtPlaceholderName' */ ``` After adding some initial implementation, we ran into a few more random cases like https://github.com/SheetJS/test_files/blob/master/2013/apachepoi_ex45978-extraLinkTableSheets.xls.xlsb?raw=true . I suspect the external links files need to be processed.

fzumstein commented

2017-07-07 09:14:08 +00:00

(Migrated from github.com)

OK thanks for the feedback!

reviewher commented

2021-10-03 08:37:16 +00:00

(Migrated from github.com)

The particular part was reworked, but you've been credited on a related commit involving incorrect quoting of #REF in an unknown 3d reference.