Count \t as a character in speech.isBlank. #14102
Conversation
See test results for failed build of commit ef312f3f0e |
|
Hi @TheQuinbox For me it's kind of bad. Thanks |
|
The same way you differentiate between a blank line and a line containing only spaces. I see no difference here, they're both whitespace |
|
I was just wondering exactly the same thing. Surely this needs some kind of
toggle.
Brian
…--
***@***.***
Sent via blueyonder.(Virgin media)
Please address personal E-mail to:-
***@***.***, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From: "Rowen" ***@***.***>
To: "nvaccess/nvda" ***@***.***>
Cc: "Subscribed" ***@***.***>
Sent: Sunday, September 04, 2022 1:53 AM
Subject: Re: [nvaccess/nvda] Count \t as a character in speech.isBlank. (PR
#14102)
Hi @TheQuinbox
For me it's kind of bad.
If so how can I differentiate between a completely blank line and a line
containing a tab?
Thanks
--
Reply to this email directly or view it on GitHub:
#14102 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
|
No Tabs can be more than one space.
Brian
…--
***@***.***
Sent via blueyonder.(Virgin media)
Please address personal E-mail to:-
***@***.***, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From: "Quin" ***@***.***>
To: "nvaccess/nvda" ***@***.***>
Cc: "Subscribed" ***@***.***>
Sent: Sunday, September 04, 2022 3:09 AM
Subject: Re: [nvaccess/nvda] Count \t as a character in speech.isBlank. (PR
#14102)
The same way you differentiate between a blank line and a line containing
only spaces. I see no difference here, they're both whitespace
--
Reply to this email directly or view it on GitHub:
#14102 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
|
Why does it matter how many spaces it is? Go ahead and try this:
|
|
@michaelDCurran - do you think we should accept this PR? @TheQuinbox - please fix up the system tests. Note the behaviour described in the "TODO" comments should be the behaviour that occurs as result of this PR. |
|
It certainly better standardizes NVDA's behaviour for whitespace characters. |
|
I sadly don't have the time to fix this up. I assumed the AppVeyor tests failing was just because they do that, I saw it for other PRs that got merged, so ignored it. With exams and such I just don't have the time to add a config option and all this, but someone else can pick this up if they feel so inclined. |
@michaelDCurran - I'm surprised that we would consider this an API breaking change. I can't imagine a scenario which would break add-ons, it would just be an intentional change of behaviour which means they may process tabs like they would process other whitespace characters. This could create a change of behaviour in the add-on, but I don't think we have considered a change like this to be a breakage of compatibility in the past. If API breakage is a concern, I suggest this PR be resurrected for 2023.1 with the system tests fixed.
It was recently raised that we actually had #13431 scheduled as work to do. We were not aware as the author of the PR did not put it in the PR description initially. There is also an alternative PR #13483 |
|
As I say, from a pure usability point of view I do believe we should
address this as it will make the experience more consistent.
However, I do consider this a possible add-on compatibility breaking
change because there are add-ons out there that enhance NVDA's indenting
reporting support. In deed one specifically mentioned on this pr, and
there was one in the past to add tones support, though that has since
been integrated into NVDA.
Again, I think we should take the change from a usability perspective,
but I just want to acknowledge that there could be add-ons that assume a
certain behaviour.
Another option is to leave that set as is, and replace any internal
references to it with `str.isspace()` which if we agree to ad tab, then
really we are now saying any whitespace.
The easiest and safest option is to go with this pr, but for 23.1.
|
|
@michaelDCurran, should I also document this change when the time comes, then? @seanbudd, how should I go about fixing the system tests? I can't see what broke (not sure how to use Appveyor). |
|
@TheQuinbox yes, breaking changes must be documented. To be able to fix these system tests you should try running them locally. |
|
Is there still a desire for this? If the author is too busy, then I think I could quickly put this together. I am familiar with the failing tests since they are the same ones I had to change for #13483 |
|
For me, a line with a tab or various spaces is not a blank line... |
|
@ruifontes so are you saying the current behavior of a line with just spaces being read as "blank" is incorrect? What would you suggest should be spoken instead? |
|
Nothing or space for 1 space symbol or x spaces... |
You already have the option for turning that on. If you set indentation reporting to "report indentation with speech" then it will read a line with just spaces as exactly that. This wouldn't change that |
|
But why I have to hear indentation if I don't want that? |
In your previous comment you said you don't want to hear "blank" if there are spaces onn a line. You said instead you want to hear x spaces. That is the behavior you get when you turn on "report indentation with speech". |
|
@SamKacer, please do not put in my mouth words I have not said... |
|
I agree with @ruifontes. I am a programmer, and a line with just whitespace is
an error that I want to address most of the time.
That said, if I could change tab to be spoken with symbol pronunciation on such
lines, I would probably be fine with that.
Regarding your question @SamKacer: in the best world, I would have lines of four
spaces spoken as "4 space", as it does for multiple punctuations such as
slashes.
|
|
@SamKacer wrote:
In your previous comment you said you don't want to hear "blank" if there are spaces onn a line. You said instead you want to hear x spaces. That is the behavior you get when you turn on "report indentation with speech".
The point @@ruifontes is making, is that hearing when a supposedly blank line
actually contains spaces, is a different situation to wanting to hear all
indentation, all the time.
As I work with lots of code, I have indentation turned on in my main profile.
But it is kind of a PITA on websites and in other contexts, where it doesn't
actually matter.
If I am editing a text file that isn't code in Notepad or Word, hearing
indentation is rarely useful. However, knowing that a rogue line got some spaces
on it is still important. What if I later choose to add something to that line?
Not knowing it had spaces on it already could be important.
Otherwise, though, in the rest of that document, spoken indentation would be
annoying. And the loud beeps of beeping indentation make it sound like I'm
playing a game, which is annoying to people around me.
In @ruifontes's case, he can already "see" indentation via the braille display.
Why should he have to hear it too, just to know that a blank line has
"invisible" (to the braille display) spaces?
|
|
@ruifontes Apologies, I didn't mean to put any words in your mouth. I just saw you write the following and that's how I interpreted it. I think I get now what you meant from @XLTechie explanation
|
|
@XLTechie Ah ok, thanks. I think I get it now. Essentially what you 2 want is like indentation reportingwith speech, but only for blank lines |
|
@XLTechie Ah ok, thanks. I think I get it now. Essentially what you 2 want is like indentation reportingwith speech, but only for blank lines
I think if you're going to change the current behavior, that's how I would
prefer it to be personally.
Ultimately I speak only for myself though.
|
|
Could you please comment in an open issue or open a new one? All these comments will be easily lost or unread. |
|
Could you please comment in an open issue or open a new one? All these comments will be easily lost or unread.
I've heard that before. Why exactly is that?
I still get notifications for them, just as if this was a still open issue.
Apparently so do all the other participants. In fact, I didn't even notice it
was closed.
Why will the comments be lost? GitHub doesn't delete comments posted to closed
issues. In fact, it is often the way to get issues re-opened.
In this case it's a PR, so it won't get re-opened, but the comment thread still
seems valid for the participants, and anyone directed to look at this PR's
comments later on (just as would happen with an open issue) will see them.
I have no objection to a new or open issue, I am just curious about the
vehemence of this recurring request, as it is not consistent with my experience
of GitHub notifications.
|
|
NV Access does not track closed PRs or closed issues. If there are outstanding requests on a closed issue or PR, it won't be addressed by us. Users looking for an active issue with NVDA are less unlikely to search closed issues |
|
@seanbudd Hi, I started on here because I was asking if this PR is still desired. I saw that the issue was that author didn't fix the system tests. I offered to make this changes and fix the tests, but asked here first. Thought it was appropriate |
Link to issue number:
Alternative to #13483
Fixes #13431
related to mltony/nvda-indent-nav#6
Summary of the issue:
speech.isBlank() wasn't counting tab as a blank character. This lead to two issues:
Description of user facing changes
If a line contains only tabs, it will now be reported as blank.
Description of development approach
added "\t" to speech.BLANK_CHUNK_CHARS
Testing strategy:
Turned off indentation reporting, and attempted to read a line containing only a tab.
Known issues with pull request:
None
Change log entries:
Changes
Code Review Checklist: