Opened 3 years ago

Last modified 12 days ago

#1987 incubating enhancement

Add the ability for NVDA to read charts in Excel

Reported by: elliott94 Owned by: mdcurran
Priority: minor Milestone:
Component: Microsoft Office support Version: master
Keywords: Cc:
Operating system: Blocked by: #4838
Blocking:
Changes document entry (for developers):

Description

When working with data in worksheets, it sometimes becomes necessary to create charts out of several cells in a worksheet.

Unfortunately, NVDA doesn't currently support these. The attached example shows a Bar Chart, with the labeled cells A1, B1, and C1. It would be great to add support for these, if at all possible.

Attachments (2)

Test_Chart_1.xls (16.0 KB) - added by elliott94 3 years ago.
A test Bar Chart.
nvda.log (2.5 KB) - added by dineshkaushal 3 months ago.
For comment 17

Download all attachments as: .zip

Change History (35)

Changed 3 years ago by elliott94

A test Bar Chart.

comment:1 Changed 12 months ago by deependra230

Hello,
I am working on this ticket. I understand the basic functioning of 'excel.py' but i couldn't find anything related to charts' interface on the web. Moreover, there is no default way to navigate inside the chart/graph. Can someone please suggest where to start and how to proceed ?
I know its quite a vague question but i am stuck at the start.

comment:2 Changed 4 months ago by dineshkaushal

Please review the branch in_t1987_f7

https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

The problem of excel chart elements being announced multiple times after every alt tab is solved. Thanks to Jamie, the problem is solved using weakref.

comment:3 Changed 4 months ago by jteh

  • Component changed from Core to Microsoft Office support
  • Keywords needsCodeReview added
  • Owner set to mdcurran

comment:4 Changed 4 months ago by jteh

  • Status changed from new to assigned

comment:5 Changed 4 months ago by mdcurran

Great work again so far.

Some things to change:

  1. When object navigating next or previous from a chartElement, you end up on another window outside the chart. At a minimum you should set previous and next properties to None on ExcelchartElementBase. If possible though, it would be cool to implement next and previous properties that actually fetched the next and previous chart element. However this may be impossible to do.
  2. I don't think there is a need to provide all those redirect scripts on ExcelChartElementBase. Instead, on the scripts of the chart, set canPropagate to True. As the chart is the parent, NVDA will automatically allow matching on these. For example:
    script_reportTitle.canPropagate=True
    
  3. Provide translator comments for all your translation strings.
  4. the _select method on ExcelChartElementBase and subclasses should be renamed to something like _getLabel. _select really suggests that it will do something, rather than get something.
  5. We have a role_chart in controlTypes. This can be used for the chart's role.

comment:6 Changed 4 months ago by mdcurran

Also, this branch is still based on the work for the NVDA+f7 list in Excel. We should have a discussion soon around how that code can be rewritten to make use of the browse mode abstraction work in #2975, so that Excel can have quick nav to comments and formulas and such, and how that elements list can also list charts. But for now, only the points in my last comment are important.

comment:7 follow-up: Changed 4 months ago by dineshkaushal

Thank you Mike.

I have made the changes as per your recommendation.

  1. previous and next properties for ExcelChartElement are now set to None. Since these elements get focus from excel events, I don't know how to set actual previous and next element.
  2. removed the redirect scripts and added canPropogate for those scripts.
  3. added translators comments
  4. _select renamed to _getChartElementText
  5. changed role to ROLE_CHART for ExcelChart
  6. added classes for the objects that do some processing. ExcelChartElementBase handles default prompts for other elements.

I need your help for reporting color for the series. I have provided the similar implementation that is found in excel for color reporting, but NVDA announces rgb(red=x,green = y , blue = z).
Please check def script_reportCurrentChartElementColor(self,gesture) in ExcelChartElementBase

the branch is at in_t1987_f7
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:8 follow-up: Changed 4 months ago by dineshkaushal

Forgot to ask:

where can I find the code for element list for browse mode? I tried, but I didn't find it in excel.py in branch next and t2975.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 months ago by mdcurran

Replying to dineshkaushal:
Great.
For colors, you need to use the RGB object's name property.

ui.message ( _( "Series color: {} ").format(colors.RGB.fromCOLORREF(int( self.excelChartObject.SeriesCollection( self.arg1 ).Interior.Color ) ).name  ) )

comment:10 in reply to: ↑ 8 ; follow-up: Changed 4 months ago by mdcurran

Replying to dineshkaushal:
New abstracted browse mode and elements list is part of #2975. Branch t2975.
However, its only been added to Microsoft Word so far. No work has been done on Excel.
Most of the abstraction is in browseMode.py, but you can see a real implementation in NVDAObjects/window/winword.py.

comment:11 in reply to: ↑ 9 Changed 4 months ago by dineshkaushal

Fixed the reporting of colors using the name property.
Thank you very much

comment:12 in reply to: ↑ 10 Changed 3 months ago by dineshkaushal

I have started working on integrating browseMode. I am seeking a clarification.

Do we need to implement TextInfo? as per the following code, it looks that we need to implement TextInfo.

class TextInfoQuickNavItem(QuickNavItem):

def activate(self):

self.textInfo.activate()

Should we discuss these issues on this ticket or on #2975?
Replying to mdcurran:

Replying to dineshkaushal:
New abstracted browse mode and elements list is part of #2975. Branch t2975.
However, its only been added to Microsoft Word so far. No work has been done on Excel.
Most of the abstraction is in browseMode.py, but you can see a real implementation in NVDAObjects/window/winword.py.

comment:13 follow-up: Changed 3 months ago by jteh

No, you shouldn't implement a TextInfo. TextInfoQuickNavItem provides a base implementation of several QuickNavItem methods to avoid duplication if you do have a TextInfo. If you don't, you inherit from QuickNavItem directly and implement those methods (such as {{{lt_}} (less than comparison), label, activate, isChild, moveTo, etc.) yourself. See the QuickNavItem class for details.

comment:14 in reply to: ↑ 13 Changed 3 months ago by dineshkaushal

Then will I also implement

class BrowseModeTreeInterceptor(treeInterceptorHandler.TreeInterceptor):

Because script BrowseModeTreeInterceptor.activatePosition calls self._activatePosition(info)

comment:15 follow-up: Changed 3 months ago by jteh

Hmm. We'll need to discuss that. For now, just subclass BrowseModeTreeInterceptor. If you want enter to work, just override script_activatePosition. If we change anything related to this, it won't have a significant impact on your code.

comment:16 in reply to: ↑ 15 Changed 3 months ago by dineshkaushal

I have done first implementation, but I am unable to make the code work. After implementing the code, no excel keys are working.
branch in_t1987_bm
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

Changed 3 months ago by dineshkaushal

For comment 17

comment:17 Changed 3 months ago by dineshkaushal

The current implementation works somewhat, but there is an error on selecting current chart. The following error is there.

AttributeError: 'ExcelBrowseModeTreeInterceptor' object has no attribute 'makeTextInfo'

See the attached log file for more details.

branch in_t1987_bm

​​https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:18 Changed 3 months ago by mdcurran

Just a heads up that QuicknavItem has been slitely changed in master, changeset:e7c90a3 to fix #4767. This will affect this ticket, so you'll want to merge master and made the necessary change.

  • QuickNavItem.moveTo now only should do the move, but not any reporting. It also therefore no longer takes any keyword arguments.
  • QuicknavItem.report method has been introduced, this method should handle reporting of the item. It takes a readUnit keyword argument like moveTo did, which you may or may not use.

comment:19 follow-up: Changed 2 months ago by dineshkaushal

I merged master with in_t1987_bm, but I could not find the change.

I then checked out master, and I can still not find the changed definition for QuickNavItem in browseMode.py file.

Which branch should I checkout?

comment:20 in reply to: ↑ 19 Changed 2 months ago by mdcurran

Replying to dineshkaushal:
A current checkout of master for me, source/browsemode.py: the new report method starts at line 103.
The moveTo method is at line 111.

comment:21 follow-up: Changed 2 months ago by dineshkaushal

I got the changes from master, but I am still getting the error for MakeTextInfo

comment:22 in reply to: ↑ 21 ; follow-up: Changed 2 months ago by mdcurran

Replying to dineshkaushal:
The makeTextInfo error is not related to the other changes. However, I'll look at that makeTextInfo bug today. I meant to do this a while ago.

comment:23 Changed 2 months ago by mdcurran

  • Blocked by 4838 added

comment:24 in reply to: ↑ 22 Changed 2 months ago by mdcurran

Replying to mdcurran:
Please merge t4838 into your branch. This will stop the makeTextInfo error.
Also a few other things:

  • In my copy of in_t1987_bm, it seems excelChart is not imported in source/NVDAObjects/window/excel.py. You won't be able to import excelChart at the top as you'll get a circular import error as excelChart imports excel and tries to use excelBase. But instead you can import excelChart in _getSelection just before you use excelChart.ExcelChart.

comment:25 Changed 2 months ago by dineshkaushal

Added chart, comment and formula listing options in the browse mode.

branch in_t1987_bm
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:26 Changed 7 weeks ago by mdcurran

Some final code review:

  • As you've initially copied some of the QuicknavItem stuff from winword.py, some of the docstrings and comments are still mentioning Microsoft Word. Please rewrite these to refer to Excel.
  • It may be an idea to rename collectionFromRange to collectionFromWorksheet as that is what it now deals with in Excel.
  • Would it be an idea to create a base class for ExcelCommentQuickNavItem, ExcelFormulaQuickNavItem etc, that implements __lt__ moveTo, and isAfterSelection, so that the same code isn't duplicated for all those classes? Though, please excuse me if the code in each is slightly different. If so then don't worry about this. But from a quick glance it looked pretty much the same.

comment:27 Changed 7 weeks ago by dineshkaushal

Thanks Mike,
I have fixed all the doc strings refering to word, created ExcelRangeBasedQuickNavItem class as base class for ExcelCommentQuickNavItem and ExcelFormulaQuickNavItem, and renamed collectionFromRange to collectionFromWorksheet.

comment:28 Changed 7 weeks ago by mdcurran

I've now branched in_t1987_bm to t1987 on NV Access.
I've made the following changes:

  • Fixed broken submodules. When pulling or merging from this branch, do a git submodule update after to ensure you have the correct submodules and that you don't accidentally commit old ones.
  • Excel's treeInterceptor now is created always, though is set to passThrough by default. Also the elementsList script has been overridden on the treeInterceptor to allow it to work in passthrough mode. In short, now when in Excel, you can press NVDZA+f7 to get the elemts list with out having to press NVDA+space first.
  • I rewrote ExcelQuickNavIterator.iterate as the code was picking up some incorrect cells due to Excel's rante object's not handling item indexing in a good way. It seems indexing simply accesses adjacent cells in the sheet, rather than in the actual range. Therefore, for now we just iterate over the range itself. This works fine for the elements list, but this would have to be rethought if we wanted to start supporting quick navigation keys from arbitrary locations in a sheet.
  • Excel's treeInterceptor now only stays alive while this worksheet is the active sheet. This means that when switching sheets, elements list will show the correct data for the sheet.
    • NVDA now fires a focus event in moveTo for comments/formulars/charts, as Excel's activate method does not do this itself. Therefore, now NVDA announces hat you land on after moving to it from the elements list.

Take a look at my work and provide any feedback. If everyone is happy, then I can prepare to merge this into next.

comment:29 Changed 6 weeks ago by dineshkaushal

Testing is done, you can now integrate this with next.

Thanks for your help.

comment:30 Changed 6 weeks ago by Michael Curran <mick@nvaccess.org>

  • Status changed from assigned to incubating

In f372f6ef6f974a6509fddfd0592af1f99f718faa:

Merge branch 't1987' into next. Incubates #1987

comment:31 Changed 6 weeks ago by Michael Curran <mick@nvaccess.org>

In 99683fbd296aa0dfc11cba0d125ca6b8f45d78b8:

Merge branch 't1987' into next. Incubates #1987

comment:32 Changed 5 weeks ago by jteh

  • Keywords needsCodeReview removed

comment:33 Changed 12 days ago by mdcurran

were there any outstanding bugs on this implementation? I thought one was mentioned on a call but can't quite remember.

Note: See TracTickets for help on using tickets.