Excel: fix comments/formulas listing (NVDA+f7) on some non-English systems#17720
Conversation
|
Pending triage of #11366 |
seanbudd
left a comment
There was a problem hiding this comment.
Code changes look good, pending triage/example to test with
|
@seanbudd thanks for your review. Regarding triage / repro, see #11366 (comment). |
01cdd95 to
0e50ac8
Compare
NVDA+f7) when they are in non-contiguous cellsNVDA+f7) on some non-English systems
|
Many thanks @michaelDCurran, your intuition (#11366 (comment)) was correct! |
3 similar comments
|
Many thanks @michaelDCurran, your intuition (#11366 (comment)) was correct! |
|
Many thanks @michaelDCurran, your intuition (#11366 (comment)) was correct! |
|
Many thanks @michaelDCurran, your intuition (#11366 (comment)) was correct! |
seanbudd
left a comment
There was a problem hiding this comment.
Thanks @CyrilleB79 - approach looks great
| numCellsFetched = ctypes.c_long() | ||
| address = collectionObject.address(True, True, xlA1, True) | ||
| sep = worksheet.Application.International(XlApplicationInternational.LIST_SEPARATOR) | ||
| localeAwareAddress = address.replace(",", sep) |
There was a problem hiding this comment.
As the address contains the file name and sheet name, replacing the commas like this will mangle the file or sheet names if they happen to contain a comma, which Excel definitely allows.
E.g.
'[test, ding.xlsx]ding, dong'!$A$1
At very least, you need to split on the exclamation mark (!) and only replace the commas after that point.
There was a problem hiding this comment.
Thanks @michaelDCurran for noting this! I have opened #17754 to fix it up in a more robust way, at least I hope so...
Fix-up of #17720 Summary of the issue: #17720 fixed Excel element list dialog on non-English system, using the local list separator to request a range. Though, the way it was implemented could fail in case the Excel file or sheet names also contained a comma. Description of user facing changes Listing comments or formulas in Excel no longer fails when the name of the file or the sheet contains a comma. Description of development approach Split the range string on the last "!" character to isolate the cells range from the file/sheet names. Replace the separator only in the cells range part. Testing strategy:
Link to issue number:
Fixes #11366
Summary of the issue:
In an Excel sheet, when comments or formulas are located in non-contiguous cells (i.e. most of the time), listing them in the elements dialog (
NVDA+f7) fails on some non-English systems.It's because the range containing the addresses of the cells of interest are retrieved in native (English) format whereas the call to
application.rangein cpp code expects a locale aware version of the range's address to retrieve the cells content. The two formats differ one from another because they do not use the same list separator: "," in English / ";" in some other languages such as French (where "," is already the decimal separator).Description of user facing changes
Excel will no longer fail to list formulas/comments in its elements dialog on some non-English systems for non-contiguous cells.
Description of development approach
In a first attempt, I added one extra loop to retrieve the content for each area separately; however, calling NVDA helper many times may lead to numerous cross-process calls causing delays.
Finally, (thanks to @michaelDCurran), I have converted the native (English) address range before passing it to NVDA helper which calls
application.range().Testing strategy:
Manual test for comments, formulas in non-contiguous cells.
Tested on French system with:
If someone wants to test on other office versions (e.g. 365, or older ones) feel free to provide feedback. Thanks.
Known issues with pull request:
None
Code Review Checklist:
@coderabbitai summary