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 line number reporting in Visual Studio #13604

Merged
merged 11 commits into from
Apr 21, 2022
Merged

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Apr 13, 2022

Link to issue number:

Closes #13574

Summary of the issue:

In Visual Studio, when line number reporting is enabled in VS itself, line numbers are part of the text. Therefore they are reported regardless of NVDA's line reporting settings. This also causes indentation reporting to fail.

Description of how this pull request fixes the issue:

This pr isolates the line number from the text and ensures that it is reported appropriately when line number reporting in NVDA is on.

Testing strategy:

With line reporting enabled in Visual studio

  • Ensure that line numbers are reported when enabled in NVDA
  • Ensure that line numbers are not reported when disabled in NVDA

Known issues with pull request:

Line number reporting does not work when line numbers are disabled in Visual Studio.

Change log entries:

Bug fixes

  • When Visual Studio is set up to show line numbers in the editor, NVDA's setting to report line numbers is now respected.
  • This also fixes line indentation not being reported correctly. (Indentation is not reported by NVDA in Visual Studio 2022 #13574)
    • Note that for line number reporting to work, showing line numbers must still be enabled in Visual Studio.

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

@AppVeyorBot
Copy link

See test results for failed build of commit 7afececdbb

@CyrilleB79
Copy link
Collaborator

Testing strategy:
With line reporting enabled in Visual studio

Would it be also worth testing with line number not displayed in Visual studio and not reported by NVDA?
Especially for lines beginning with a number followed by spaces or tabs.

Change log entries:
Bug fixes

  • When Visual Studio is set up to show line numbers in the editor, NVDA"s setting to report line numbers is now reported.

Could you rephrase this? It's not the setting that is reported but the line numbers.

I think that these two points should be a sub-list and not separated items of the change log. Separated items do not make sense.

Thanks

@LeonarddeR
Copy link
Collaborator Author

Would it be also worth testing with line number not displayed in Visual studio and not reported by NVDA?

Sure, good catch, though this behaviour is implicitly tested with text ranges that are positioned somewhere else than the start of a line.

Especially for lines beginning with a number followed by spaces or tabs.

This is not necessary, as the UIA implementation returns the line number for a collapsed text range. That's also a reason why I consider this water proof. If a line starts with a number followed by a space, a collapsed range at the start of the line will be empty when line number reporting is turned of.

@akash07k
Copy link

@LeonarddeR
Any try build please? I want to test this PR

@akash07k
Copy link

Sadly now it doesn't even report the line numbers.
For making it announce the line numbers we need to turn on the line numbers from NVDA document formatting settings explicitly along with turning on the line numbers on from Visual Studio Text Editor settings.
Then too, strangely now it announces like: "line 1, line 2, line 3" instead of "1, 2, 3" etc.
Also, it doesn't report the indentation at all.
For making it announce the indentation, we are required to turn off the line numbers from both of the sides.
@LeonarddeR

@LeonarddeR
Copy link
Collaborator Author

I'll have a look here.
What version of Visual Studio are you using?

@LeonarddeR
Copy link
Collaborator Author

Sadly now it doesn't even report the line numbers. For making it announce the line numbers we need to turn on the line numbers from NVDA document formatting settings explicitly along with turning on the line numbers on from Visual Studio Text Editor settings.

This is is as expected. See the changes log entry. We simply can't report line numbers when they are hidden in Visual Studio.

Then too, strangely now it announces like: "line 1, line 2, line 3" instead of "1, 2, 3" etc.

This isn't a bug as well. This is how line reporting works in NVDA. You will notice that it will work equally in other text editors.

Also, it doesn't report the indentation at all. For making it announce the indentation, we are required to turn off the line numbers from both of the sides. @LeonarddeR

Good catch, I should have tested that more thoroughly. Just pushed a fix for this.

@akash07k
Copy link

akash07k commented Apr 14, 2022 via email

@akash07k
Copy link

akash07k commented Apr 14, 2022 via email

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit ce35702962

@LeonarddeR
Copy link
Collaborator Author

So will it keep on reporting "line 1, line 2" etc?

Correct. Changing this is a different question that falls beyond the scope of this issue.

@cary-rowen
Copy link
Contributor

@akash07k
The reporting method of line numbers is defined in nvda's interface language translation file nvda.po.

@akash07k
Copy link

@cary-rowen So can we modify this translation via addon or we'll have to modify the source itself?

@XLTechie
Copy link
Collaborator

XLTechie commented Apr 14, 2022 via email

@cary-rowen
Copy link
Contributor

@akash07k
We could simplify the way line numbers are reported by modifying nvda.po, but that's not necessarily the most correct solution, and we could create a separate issue to discuss this.

@akash07k
Copy link

akash07k commented Apr 14, 2022 via email

@akash07k
Copy link

@cary-rowen very true. I'll create the separate issue for it in morning.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 19, 2022
@LeonarddeR
Copy link
Collaborator Author

I just pushed a somewhat different approach that ensures the line number is not reported when copying, selecting or deleting text. This also removes the need to tweak _getTextWithFields, which is certainly a good thing.

@AppVeyorBot
Copy link

See test results for failed build of commit fe7e42eef8

@akash07k
Copy link

akash07k commented Apr 19, 2022 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Apr 19, 2022 via email

try:
formatField.field['line-number'] = int(lineNumberStr)
except ValueError:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this log something? in what cases is lineNumberStr not an integer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect this to fail, though it certainly makes sense to log something.

@AppVeyorBot
Copy link

See test results for failed build of commit 4452b74205

@seanbudd seanbudd merged commit ef31b1d into nvaccess:master Apr 21, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Apr 21, 2022
@akash07k
Copy link

@LeonarddeR
I tested it today, however, even though if line numbers are off in NVDA's settings, still it reports the line numbers, and if we turn on the line number reporting from NVDA's settings along with the indentation, then too it doesn't report any indentation.
It seems that something is still broken up here.
Help please?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Apr 23, 2022 via email

@akash07k
Copy link

@LeonarddeR ya buddy. I've tested and indeed, I'm on latest alpha.
NVDA version alpha-25262,a55f9a62

@lukaszgo1
Copy link
Contributor

@akash07k Please note that while this fix has been merged to Alpha, due to failing system tests (unrelated to this PR) it failed to build, therefore this is not yet included in a binary version. You need to wait for the next Alpha.

@akash07k
Copy link

@lukaszgo1 Ah, got it. I'll wait then. thanks!

@LeonarddeR
Copy link
Collaborator Author

I just tried this with latest Alpha and all works like a charm here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indentation is not reported by NVDA in Visual Studio 2022
9 participants