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

Fix Excel error due to state enum changes #13465

Merged
merged 4 commits into from
Mar 14, 2022
Merged

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Mar 11, 2022

Link to issue number:

fixes #13457

Summary of the issue:

With UIA for Excel disabled, navigating to a cell with a formula and "has formula" is not reported.
This is a regression caused by #13414

In NVDAHelper/remote/excel.cpp the properties of a cell are determined and bits are set on the state member of a EXCEL_CELLINFO struct.
The constatnts used for this are in NVDAHelper/remote/excel/Constants.h, see the NVSTATE_* constants, previously these matched the controlTypes.State constants directly.

Description of how this pull request fixes the issue:

Rather than couple the excel implementation to the controlTypes implementation, these constants have been converted to enums (both in C++ and in Python), renumbered, and an explicit mapping to the corresponding controlTypes.State enum has been created.

Testing strategy:

  • Added a unit test to ensure all cellState values are mapped.
  • Manually tested Excel with formula and comments on cells (both with UIA and without).

Known issues with pull request:

None

Change log entries:

For Developers

- Excel cell state constants (``NVSTATE_*``) are now values in the ``NvCellState`` enum, mirrored in the ``NvCellState`` enum in ``NVDAObjects/window/excel.py`` and mapped to ``controlTypes.State`` via _nvCellStatesToStates.
- ``EXCEL_CELLINFO`` struct member ``state`` is now ``nvCellStates``.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@feerrenrut feerrenrut requested a review from a team as a code owner March 11, 2022 13:05
@feerrenrut feerrenrut requested review from seanbudd and removed request for a team March 11, 2022 13:05
@feerrenrut feerrenrut mentioned this pull request Mar 12, 2022
5 tasks
michaelDCurran
michaelDCurran previously approved these changes Mar 13, 2022
@AppVeyorBot

This comment was marked as outdated.

@feerrenrut feerrenrut merged commit b3cc309 into beta Mar 14, 2022
@feerrenrut feerrenrut deleted the fixNVStateConstants branch March 14, 2022 03:11
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 14, 2022
feerrenrut added a commit that referenced this pull request Mar 15, 2022
Following PR: #13465
Two locations were not correctly updated to type std::uint64_t
@seanbudd seanbudd modified the milestones: 2022.2, 2022.1 Mar 15, 2022
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.

5 participants