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

Replaced messages, items and dialog title relating to comments in excel to refer to notes, plus some userguide work #11311

Merged
merged 15 commits into from Jul 6, 2020

Conversation

Adriani90
Copy link
Collaborator

@Adriani90 Adriani90 commented Jun 28, 2020

Link to issue number:

fixes #10923
fixes #4864
Addresses discussions in #11153.

Summary of the issue:

Outdated system requirements and missing note for braille timeout.
The main issue addressed here however, is the new controltype in MS Excel since version 2016, which introduces new style comments, allowing for replying to comments and creating conversations on a specific cell or cell range. Thus, the comments as they were called in previous MS Office versions are now called "notes". The new style comment editing dialog is different from the notes editing dialog. The notes editing dialog is the former comments editing dialog, which remains also inaccessible in MS Office 2016, 365 and newer.

Description of how this pull request fixes the issue:

  1. Updated system requirements and added a small note to braille timeout (very small change thus I included it in this PR)
  2. Changed all messages in MS Excel, as well as coresponding dialog title for the NVDA specific comment editing dialog (shift+f2) and the item label in elements list, now refering to notes instead of comments. Changed also the key stroke in MS Excel to report notes from nvda+alt+c to nvda+alt+n.

Testing performed:

Tested in MS Excel:

  • That in elements list, the item is called notes instead of comments
  • That pressing nvda+alt+n reports the note added via the notes editing dialog and the NVDA specific notes editing dialog (shift+f2)
  • Checked that the NVDA specific note editing dialog (shift+f2) is refering to notes and not to comments.
    Tested in Microsoft Word and Powerpoint:
  • That NVDA+alt+c still reports the comments as before.

Known issues with pull request:

  • When focusing a cell with notes, NVDA still reports "has comment" while it should now report "has note". This is because the controltype for controls which contain a comment is the same controltype used in MS Word and Powerpoint, thus it is defined in controltypes as one type with the label "has comment".
  • The comment controltype in Excel should be separated from the controltype in MS Word and Powerpoint, changing its label to "has note"
  • A new controltype for MS Excel should have the label "has comment" which should report the cells with new style comments. For that new controltype, the key stroke nvda+alt+c should be assigned, to be consistent with MS Word.

Other notes

  • In MS Word, the new style comments and the notes are not splited, so they are the same. This means we must adjust how NVDA treats this in Excel only.
  • The downgrade is that people using Excel 2013 or older will still get notes reported although in those versions of MS Excel they are still called comments.

Change log entry:

Section: Changes,

  • NVDA now reports the correct terminology for notes in MS Excel

…el, they are called notes. Support for new style comment is not added in NVDA yet
@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 28, 2020

@feerrenrut, @leonardder could you take a look at this before I request an official review? I think this is something that should be discussed in general, since this issue is somehow blocked until NVDA supports the new style comments in MS Excel (see #9195).

Perhaps this can more easily be fixed if an appmodule can support different versions of an app.

@AppVeyorBot

This comment has been minimized.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 28, 2020

I got a flake8 error, ET128 (flake8-tabs) unexpected number of tabs and spaces at start of call line (expected 2 tabs and 23 spaces, got 3 tabs and 0 spaces).

I think this specific error should be ignored since NVDA uses mostly tabs only in its code syntax.

@codeofdusk
Copy link
Contributor

codeofdusk commented Jun 29, 2020

I don't see a need to have the keystroke for reading a note be specific to Excel. Can't we use NVDA+alt+c for comments/notes/etc everywhere?

@feerrenrut feerrenrut requested a review from Qchristensen Jun 29, 2020
Copy link
Member

@feerrenrut feerrenrut left a comment

It would be easier to review this as 3 separate PR's. Two of them could have been merged immediately. In future PR's please submit separate PR's for separate issues, even if the changes are small.

source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@feerrenrut
Copy link
Member

feerrenrut commented Jun 30, 2020

I'm going to mark this PR as a draft for now. Please mark it "ready for review" when the CI and review comments have been addressed. Thanks for your work on this @Adriani90

@feerrenrut feerrenrut marked this pull request as draft Jun 30, 2020
@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 30, 2020

@feerrenrut regarding script decorator, my understanding was that this should be preferably used when addin gestures to input gestures dialog. These gestures are not in the input gestures dialog as far as I can see.

I've tried to convert some of them into script decorator, but I get a type error when I tried to convert the list of gestures for the script. changeSelection into script decorator for example.

I tried this:

	@script(
		gestures = ["kb:tab", "kb:shift+tab", "kb:enter", "kb:numpadEnter", "kb:shift+enter", "kb:shift+numpadEnter", "kb:upArrow", "kb:downArrow", "kb:leftArrow", "kb:rightArrow", "kb:control+upArrow", "kb:control+downArrow", "kb:control+leftArrow", "kb:control+rightArrow", "kb:home", "kb:end", "kb:control+home", "kb:control+end", "kb:shift+upArrow", "kb:shift+downArrow", "kb:shift+leftArrow", "kb:shift+rightArrow", "kb:shift+control+upArrow", "kb:shift+control+downArrow", "kb:shift+control+leftArrow", "kb:shift+control+rightArrow", "kb:shift+home", "kb:shift+end", "kb:shift+control+home", "kb:shift+control+end", "kb:shift+space", "kb:control+space", "kb:pageUp", "kb:pageDown", "kb:shift+pageUp", "kb:shift+pageDown", "kb:alt+pageUp", "kb:alt+pageDown", "kb:alt+shift+pageUp", "kb:alt+shift+pageDown", "kb:control+shift+8", "kb:control+pageUp", "kb:control+pageDown", "kb:control+a", "kb:control+v", "kb:shift+f11"]
	)
	def script_changeSelection(self,gestures):

I would be very grateful for some hint here ;)

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jun 30, 2020

@Adriani90 wrote:

I would be very grateful for some hint here ;)

You should wrote this:

def script_changeSelection(self, gesture):

not this:

def script_changeSelection(self,gestures):

The second parameter passed to each script is not a list of al keyboard shortcuts to which it is assigned but the object re[presenting a gesture (in this case a key press) which invoked it.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 30, 2020

Thanks @lukaszgo1 for the clarification. I changed it, but there is still the same error,

  File "NVDAObjects\window\excel.py", line 446, in <module>
    class ExcelBrowseModeTreeInterceptor(browseMode.BrowseModeTreeInterceptor):
  File "baseObject.py", line 179, in __new__
    gestures[gesture] = scriptName
TypeError: unhashable type: 'list'

I have the feeling that this should be very trivial to fix, but I as a spare time python beginner am still struggling with simple things like this ;)

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jun 30, 2020

Sorry for not noticing it when reading for the first time - I've assumed you've imported script from scriptHandler but it seems you haven't. You should decorate this method as follows:
@scriptHandler.script(...

Also when listing all keyboard shortcuts to which this script is assigned it is a good idea to use tuple rather than list - as we do not need mutability in this case and tuple uses less memory.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 30, 2020

Thanks for the good tipp, I converted all the script decorators to touples, this solved the error. It seems it's because in Python 3 lists are not hashable, or at least this is how I understood it.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 30, 2020

Overall converting to script decorator saved almost 20 lines in that file.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jun 30, 2020

I tested in MS Excel 365 and 2010, all key strokes are working as expected and all messages are reported correctly (i.e. not on a note, cell x set as column header, etc.).

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@feerrenrut
Copy link
Member

feerrenrut commented Jul 1, 2020

@Adriani90 I hope you are aware that you can run the lint check locally scons lint base=origin/master. It is much faster than pushing and waiting for an appveyor build.

source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jul 2, 2020

@feerrenrut thank you very much for the suggestion. Is there a possibility to specify in which file the lint should be performed? When I do base=origine/master it fetches all files and this takes very long time.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jul 2, 2020

You should do lint base=master not origin/master. Alternatively you should synchronize your remote master with the NV Access copy.

@AppVeyorBot

This comment has been minimized.

@Adriani90
Copy link
Collaborator Author

Adriani90 commented Jul 2, 2020

So finally all the actions should be addressed now, the following problem still remains:

  1. With this PR, "Has notes" is also announced in earlier versions of MS Office. We need two separate controltypes in controltypes.py, one which announces "has comment" in MS Excel 2013 and earlier (can stay as it is) and one which announces "has note" in MS Excel 2016 and newer (a new one only for MS Excel).
  2. We need a new controltype in controltypes.py for MS Excel which announces "has comment" for new style comments (see NVDA does not report (new style) comments in Excel 365 #9195).

For the first problem, is it possible to copy the current controltype which is used and specify another label to it and say that this should be only used for MS Excel 2016 and newer?

source/NVDAObjects/window/excel.py Outdated Show resolved Hide resolved
@Adriani90 Adriani90 marked this pull request as ready for review Jul 5, 2020
@Adriani90 Adriani90 requested a review from feerrenrut Jul 5, 2020
Copy link
Member

@feerrenrut feerrenrut left a comment

Thanks @Adriani90

@feerrenrut feerrenrut removed the blocked label Jul 6, 2020
@AppVeyorBot

This comment has been minimized.

@feerrenrut feerrenrut merged commit e87d8ab into nvaccess:master Jul 6, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.3 milestone Jul 6, 2020
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.

User guide: update NVDA's system requirements enhance documentation for Message Timeout in user guide
6 participants