Add option for ignoring blank lines for report line indentation by SamKacer · Pull Request #15057 · nvaccess/nvda

SamKacer · 2023-06-26T20:17:52Z

Link to issue number:

Summary of the issue:

As elaborated in #13394, line indentation reporting can be too noisy in contexts where indentation changes for blank lines aren't meaningful and distract from the actually meaningful indentation changes, such as is the case when reading source code for Python and many other programming languages.

Description of user facing changes

A new checkbox is added to Document formatting, "Ignore blank lines for line indentation reporting". By default it is off, in which case line indentation reporting behaves as it does currently. If the user ticks the checkbox, then indentation changes are not reported for blank lines.

There is also a new unassigned global command for toggling the setting.

Description of development approach

new config property: ignoreBlankLinesForRLI = boolean(default=false)
- short for ignoreBlankLinesForReportLineIndentation, which was too long and causing Flake8 issues
new settings panel checkbox for toggling ignoreBlankLinesForRLI
in speech.speech.getTextInfoSpeech, if ignoreBlankLinesForRLI is true and the line is blank, the block of code that inserts the indentation announcement and updates the indentation cache is skipped, otherwise it behaves as usual
user guide updated with brief info on new option
added unassigned global command for toggling ignoreBlankLinesForRLI
added system test for confirming the correct behavior

Testing strategy:

I added a new test in symbolPronunciationTests that confirms the correct behavior by comparing speech output when the new setting is on/off.

I also tested manually by reading the following text with "Ignore blank lines for line indentation reporting" turned off and on. In either case make sure to have line indentation reporting switched on. To quickly find the new setting, open document formatting settings with NVDA + Shift + D and press Alt + I to get to "line indentation reporting", then press tab to get to "Ignore blank lines for line indentation reporting". Alternatively, you can press alt + B twice.

Test text:

def foo():
	x = 42

	return x

def bar():

With current NVDA or with this change and "Ignore blank lines for line indentation reporting" turned off, the text should report 4 indentation changes like:

def foo():
tab x = 42
no indent blank
tab return x
no indent blank
def bar():

With "Ignore blank lines for line indentation reporting" turned on, it should instead report just 2 indentation changes like:

def foo():
tab x = 42
blank
return x

no indent def bar():

Known issues with pull request:

None that I am aware of.

Change log entries:

New features

Add new option to document formatting settings, "Ignore blank lines for line indentation reporting" (Add option to not announce indentation change for blank lines to Document Formatting Settings #13394 )
Changes
Bug fixes
For Developers

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
Security precautions taken.

AppVeyorBot · 2023-06-26T21:03:37Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/kgkmebpw3597rjyu/artifacts/output/nvda_snapshot_pr15057-28504,85bf451a.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 19.2,
FINISH_END 0.2

See test results for failed build of commit 85bf451aa5

XLTechie · 2023-06-26T21:31:23Z

Please add an accelerator key for this checkbox. I suggest Alt+b.

Also, would an unassigned gesture to toggle it be useful?

brunoprietog · 2023-06-26T21:35:53Z

Nice! I hope this will be merged into NVDA. Just a question, instead of the indentation read change occurring on the last line, will it be possible to have NVDA say something like this?

def foo():
tab x = 42
blank
return x
no indent blank
def bar():

That makes a lot more sense to me. Thanks for bringing this to NVDA!

XLTechie · 2023-06-26T21:41:02Z

In testing this build, it performs in every way as I would expect.

Tested with tones and speech (same).
Tested switching to an editor window while on a line with an indent in that editor window.
Tested changing to a window while on an indented line, at the same indent level as the window I departed.
Tested moving to an indented line from a blank line, immediately after switching windows.

While I haven't reviewed the code, the behavior is good IMO, and a useful feature to have.

XLTechie · 2023-06-26T22:07:23Z

@brunoprietog How would it know which blanks were safe to ignore, and which blanks to reset on? I agree that your wish would make it a more useful feature, but it seems impossible to implement.

That said, if it were a combobox with options of:

Speak normally (default)
Ignore
Ignore only one line

Then that third option could turn something like:

def foo():
	x = 42
	return x


def bar():
	pass

Into:

def foo():
tab  x = 42
return x
blank
no indent blank
def bar():
tab  pass

Which is more Pythonic (consistent with pep-8) for top-level functions anyway, though won't help you with methods.

brunoprietog · 2023-06-27T00:07:06Z

It's difficult. What occurs to me is that as the user listens to the lines, NVDA checks what is the indentation level of the next line that is not blank in the direction the user navigated (up or down).

If the user presses the up or down arrow, NVDA should check the indentation level of the next line that is not blank, in the direction in which the user navigated (up or down).

If the indentation level is lower than the current one, on the next blank line NVDA should immediately report the indentation level change, showing that it is now lower, even if it is on a blank line.
If the indentation level is higher than the current one, NVDA should notify the user of the indentation change to the higher level when the user reaches that line.

Here is an example:

def foo
	pass



def bar
	pass

def baz


	pass

Reading it from top to bottom, NVDA should say this:

def foo
tab pass
no indent blank
blank
blank
def bar
tab pass
no indent blank
def baz
blank
blank
tab pass

Reading it from the bottom up, NVDA should say this:

tab pass
no indent blank
blank
def baz
blank
tab pass
no indent def bar
blank
blank
blank
tab pass
no indent def foo

That's more intuitive for me, at least in my case

cary-rowen · 2023-06-27T00:12:33Z

Hi @SamKacer

This is a nice change and I like it.
I believe this will be beneficial to all, Whether this option can be checked by default.

Thanks

XLTechie · 2023-06-27T01:01:14Z

NVDA has never tried to be predictive about what navigation the user might do next. I understand why you want this, and it even seems reasonable. But that is a more controversial change than the original request I think. I would rather see this go forward as is (or with my one-line-only modification), then see the proposal to be predictive as a feature request after this feature is established. All kinds of things open up if we're willing to predict what the user might want to know or do next, but I think that will need deeper discussion than is called for by this simple change. A proposal like that has potentially concerning side-effects, and will need exploration.

LeonarddeR · 2023-06-27T05:30:31Z

How ought this to behave when indentation reporting with tones is on?

SamKacer · 2023-06-27T10:52:30Z

@XLTechie wrote

Please add an accelerator key for this checkbox. I suggest Alt+b.

Also, would an unassigned gesture to toggle it be useful?

Thanks, will do those when I have time. This is my first time adding any new config property, so please let me know about anything I missed. Actually, how do I add the accelerator key? I haven't noticed that when making my changes. Also in which file are the commands for toggling setting properties defined?

to do:

add accelerator key for new setting
add unassigned toggle command for new setting

XLTechie · 2023-06-27T11:30:49Z

how do I add the accelerator key? I haven't noticed that when making my changes.

Ampersand before the letter in the setting name. So if it was "Announce blanks", and you wanted the key to be alt+b, you would write it as "Announce &blanks". Btw, while you're in gui/settingsDialogs.py, please consider fixing the spelling of "panel" on lines 2393 and 2400.

Also in which file are the commands for toggling setting properties defined?

globalCommands.py See script_toggleProgressBarOutput for an example. Also see script_reportLinkDestinationInWindow, for an example of proper typing, which isn't done in the other one.

XLTechie · 2023-06-27T11:32:15Z

How ought this to behave when indentation reporting with tones is on?

Based on testing it, it behaves exactly as I expected. There are no de-indent beeps on the lines that are blank.

SamKacer · 2023-06-27T13:46:28Z

In regards to @brunoprietog request. He has already asked this in #13394 and I gave a very long reply there in this comment. Linking in case someone wants full context.

Thanks for the suggestion, but this would require a significant overhaul of the indentation reporting code. I am not entirely sure if I would even prefer that behavior. I think I might prefer if the rules for indentation reporting are kept simpler without special cases like this. In any case, I'm not sure the amount of effort to implement would be that worth it.

SamKacer · 2023-06-27T13:48:46Z

How ought this to behave when indentation reporting with tones is on?

@LeonarddeR the behavior should be the same whether you are using tones or speech

AppVeyorBot · 2023-06-27T19:26:37Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/jv1apas61d197b65/artifacts/output/nvda_snapshot_pr15057-28511,22744216.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.8,
FINISH_END 0.1

See test results for failed build of commit 2274421602

AppVeyorBot · 2023-06-29T17:38:06Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/hkn3hdnnrttw0csh/artifacts/output/nvda_snapshot_pr15057-28534,14abbf5d.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 25.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.3,
FINISH_END 0.1

See test results for failed build of commit 14abbf5d78

AppVeyorBot · 2023-06-29T19:24:13Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/wq9ngnct0d2ftu5v/artifacts/output/nvda_snapshot_pr15057-28535,7b637323.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.1

See test results for failed build of commit 7b637323a4

SamKacer · 2023-06-30T18:46:33Z

Ok, this is officially ready for review.

I implemented the 2 features @XLTechie suggested. I added a system test. I fixed all the Flake8 issues. The summary is up to date.

Please let me know if anything else is missing or if anything could be improved. Thanks

AppVeyorBot · 2023-07-06T09:53:40Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/25buk5ro851ll26y/artifacts/output/nvda_snapshot_pr15057-28616,acdc8d32.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.1

See test results for failed build of commit acdc8d32b4

…ReportLineIndentation

Qchristensen

User guide reads well, thanks!

…ReportLineIndentation

Follow-up of #15057 Summary of the issue: In the GUI, the recent trend is to disable (grey out) the options that are not applicable due to the values of other options. New option "Ignore blank lines for line indentation reporting" has no sense when report indentation is OFF. Thus it should be greyed out when report indent is off. This is especially needed to reduce the number of tab presses when navigating in the Doc formatting settings dialog which contains a lot of items. Description of user facing changes Option "Ignore blank lines for lineis greyed out when report indent is off and enabled when other indent reporting types are configured. While at it, in this PR, I have also changed the context help target for "Ignore blank lines" option. Before it was jumping by default to "Document formatting settings" paragraph; now it jumps to "Line indentation reporting" paragraph, which contains more precise information on this topic.

SamKacer added 4 commits June 25, 2023 16:56

Implement ignore blank lines for report line indentation

f8f05c7

Add setting checkbox for ignoreBlankLinesForReportLineIndentation

777d347

Update user doc about ignore blank lines

0ed82e3

Add additional info on ignore blank lines in user guide

e28d007

SamKacer requested review from a team as code owners June 26, 2023 20:17

SamKacer requested review from Qchristensen and seanbudd June 26, 2023 20:17

SamKacer marked this pull request as draft June 26, 2023 20:18

SamKacer added 3 commits June 27, 2023 14:55

Add mnemonic for ignore blank lines and fix some spellings

052d2ce

Add unassigned command for toggling ignore blank lines

3b80b6d

Revise user guide for ignore blank lines

5d9b8cc

SamKacer added 2 commits June 29, 2023 17:47

Add system test for ignore blank lines setting

9781821

Remove trailing whitespace

c4e6912

SamKacer added 2 commits June 29, 2023 19:29

Fix Flake8 issues

b94b8eb

Fix error in ignore blank lines system test

61e2ad9

SamKacer added 2 commits June 30, 2023 08:04

Fix system test and Flake8

8b9c3ef

Shorten config prop name to ignoreBlankLinesForRLI

41d2c3c

SamKacer marked this pull request as ready for review June 30, 2023 18:43

seanbudd requested a review from michaelDCurran July 3, 2023 23:40

seanbudd reviewed Jul 6, 2023

View reviewed changes

seanbudd marked this pull request as draft July 6, 2023 05:46

SamKacer force-pushed the ignoreBlankLinesForReportLineIndentation branch from e6744be to 41d2c3c Compare July 6, 2023 16:31

SamKacer and others added 3 commits July 7, 2023 08:54

Make expression more readable

9a939e7

Merge remote-tracking branch 'origin/master' into ignoreBlankLinesFor…

9029fc5

…ReportLineIndentation

fix up tests - add symLevel sensible default

0a3a56a

seanbudd marked this pull request as ready for review July 18, 2023 01:13

seanbudd approved these changes Jul 18, 2023

View reviewed changes

seanbudd added the merge-early Merge Early in a developer cycle label Jul 21, 2023

seanbudd added this to the 2023.3 milestone Jul 21, 2023

Qchristensen approved these changes Jul 24, 2023

View reviewed changes

seanbudd added 2 commits July 24, 2023 15:23

Merge remote-tracking branch 'origin/master' into ignoreBlankLinesFor…

34fb605

…ReportLineIndentation

update changes

6ce5eac

seanbudd merged commit 78628be into nvaccess:master Jul 24, 2023

CyrilleB79 mentioned this pull request Sep 21, 2023

Disable "Ignore blank lines" checkbox when report indent is off #15494

Merged

5 tasks

Uh oh!

Conversation

SamKacer commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Test text:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jun 26, 2023

Uh oh!

XLTechie commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoprietog commented Jun 26, 2023

Uh oh!

XLTechie commented Jun 26, 2023

Uh oh!

XLTechie commented Jun 26, 2023

Uh oh!

brunoprietog commented Jun 27, 2023

Uh oh!

cary-rowen commented Jun 27, 2023

Uh oh!

XLTechie commented Jun 27, 2023 via email

Uh oh!

LeonarddeR commented Jun 27, 2023

Uh oh!

SamKacer commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XLTechie commented Jun 27, 2023 via email

Uh oh!

XLTechie commented Jun 27, 2023 via email

Uh oh!

SamKacer commented Jun 27, 2023

Uh oh!

SamKacer commented Jun 27, 2023

Uh oh!

AppVeyorBot commented Jun 27, 2023

Uh oh!

AppVeyorBot commented Jun 29, 2023

Uh oh!

AppVeyorBot commented Jun 29, 2023

Uh oh!

SamKacer commented Jun 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jul 6, 2023

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SamKacer commented Jun 26, 2023 •

edited

Loading

XLTechie commented Jun 26, 2023 •

edited

Loading

SamKacer commented Jun 27, 2023 •

edited

Loading