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.

Sign up for GitHub

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

Jump to bottom

Braille doesn't scroll to same-line search result in Notepad #5410

Closed
nvaccessAuto opened this issue Oct 11, 2015 · 12 comments
Closed

Braille doesn't scroll to same-line search result in Notepad #5410

nvaccessAuto opened this issue Oct 11, 2015 · 12 comments
Assignees
@jcsteh
Labels
bug component/braille
Milestone
2016.1

Comments

@nvaccessAuto
Copy link

Reported by dkager on 2015-10-11 20:03
STR:

  • Type a line with the same phrase in it multiple times, at the start of the line and later on with enough other text inbetween.
  • Press Ctrl+F and search for the phrase.
  • Press Escape. Braille shows the selected text, i.e. the search phrase.
  • Press F3.
  • Braille does not scroll to the newly selected text, i.e. the second occurrence of the search phrase.
@nvaccessAuto
Copy link
Author

Comment 1 by dkager on 2015-10-15 19:17
I was a bit too specific, this happens in plain ol' Notepad too. Uneducated guess: maybe in EditableTextWithAutoSelectDetection.
Changes:
Changed title from "Braille doesn't scroll to same-line search result in Notepad++ (Scintilla)" to "Braille doesn't scroll to same-line search result in Notepad"

@nvaccessAuto
Copy link
Author

Comment 2 by dkager on 2015-10-18 12:01
Only happens when braille is tethered to focus. If tethered to review, the display is scrolled but instead of the selection indicator (non-blinking dots 7 & 8) you obviously only see the review cursor.

@nvaccessAuto
Copy link
Author

Comment 3 by dkager on 2015-10-18 18:31
Well, here is a trivial fix.
If there is a selection, then there is no regular cursor. Thus, the braille display is not scrolled to the cursor. To work around this at least in TextInfo regions, the display is also scrolled to the start of the selection if there is one. I hope this makes sense.

@nvaccessAuto
Copy link
Author

Comment 4 by jteh on 2015-10-21 05:07
Thanks for the patch. However, this won't quite work in all cases. TextInfoRegion._selectionStart is a text position, not a braille position, so this won't work as expected if you're using anything other than computer braille.

Also, I'd rather not have TextInfoRegion specific code there. One solution is to set brailleCursorPos and have a showCursor attribute which could be set to False for selection. However, Since some users want selection to be indicated with dots 7 and 8 for list items (#1953), I guess the best solution is to have selectionStart/End and brailleSelectionStart/End attributes in Region and have Region.update handle the underlining instead of TextInfoRegion.update. The patch for #1953 is then pretty trivial.

Let me know if you're willing to do this. Otherwise, I'll do it when I get some time.
Changes:
Milestone changed from None to 2016.1

@nvaccessAuto
Copy link
Author

Comment 5 by dkager on 2015-10-21 06:42
I can have a look but as usual cannot say exactly when. In the weekend of Nov 14th at the very latest, well in time for the milestone. It makes sense to do this in conjunction with #5198 I think, as you indicate by assigning both to 2016.1.

@nvaccessAuto
Copy link
Author

Comment 6 by dkager on 2015-10-24 18:59
I updated the branch I linked to in comment:3. It uses the new approach where braille.Region manages selectionStart/End. It also fixes another case of braille not scrolling to the selection that I overlooked earlier. Since #5198 can't be auto-merged with this I also did that, in t5410_t5198. This made testing a bit easier. The end result appears to do what it was meant to.

@nvaccessAuto nvaccessAuto added this to the 2016.1 milestone Nov 10, 2015
dkager added a commit to dkager/nvda that referenced this issue Nov 10, 2015
@dkager
Fix and refactor the patch for nvaccess#5410:
d465953 
* Move the marking of a selection to braille.Region.
* Add Region.brailleSelectionStart/End.
* If there is a selection but not a cursor, e.g. in a TextInfoRegion, scroll the braille display to the selection start.
@dkager
Copy link
Collaborator

dkager commented Nov 13, 2015

Code is now here on GitHub. I'll drop the BitBucket repo linked to in earlier comments.

dkager added a commit to dkager/nvda that referenced this issue Nov 14, 2015
@dkager
If no cursor is found, try scrolling to the selection. Requires nvacc…
8c5dded 
…ess#5410.
dkager added a commit to dkager/nvda that referenced this issue Nov 14, 2015
@dkager
If no cursor is found, try scrolling to the selection. Requires nvacc…
b5e7906 
…ess#5410.
@dkager dkager mentioned this issue Nov 14, 2015
Merged
jcsteh pushed a commit that referenced this issue Nov 25, 2015
@dkager @jcsteh
When using a braille display and text is selected on the current line…
a5adfe2 
… (e.g. when searching in a text editor for text which occurs on the same line), the braille display will be scrolled if appropriate.

On a pending caret move if there is no cursor, try to scroll the braille display to the selection.

* Move the marking of a selection to braille.Region.
* Add Region.brailleSelectionStart/End.
* If there is a selection but not a cursor, e.g. in a TextInfoRegion, scroll the braille display to the selection start.

Fixes #5410.
@nvaccessAuto
Copy link
Author

Incubated in f53054b.

@jcsteh jcsteh mentioned this issue Nov 25, 2015
Closed
dkager added a commit to dkager/nvda that referenced this issue Nov 25, 2015
@dkager
Oops, I think I overlooked something in nvaccess#5410.
0ca49f1
@jcsteh
Copy link
Contributor

jcsteh commented Nov 26, 2015

Ug. I was reviewing #5198 and discovered that the selection indicator still shows when the cursor is disabled. I just realised that even with this fixed, the selection indicator doesn't appear/disappear when you toggle the option from a script, since the region isn't updated.

  • I don't want to update all buffers every time we dismiss a message.
  • I guess you could update the last region from within the show cursor script. That might result in an unnecessary update, but at least it's isolated.
  • Alternatively, you could move selection drawing into BrailleHandler alongside cursor drawing. You'd still need to notify BrailleHandler, though, as there won't be a blink timer to do the work for you.

@dkager
Copy link
Collaborator

dkager commented Nov 26, 2015

Very quick comment (more this afternoon when I have time to read the other comments in #5198): it wasn’t my intention to also disable the selection indicator when the cursor is toggled off. It is arguably not the same thing as a cursor indicator. Even without a blinking cursor I might still want to know what I selected in an edit control.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, November 26, 2015 06:34
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Braille doesn't scroll to same-line search result in Notepad (#5410)

Ug. I was reviewing #5198 #5198 and discovered that the selection indicator still shows when the cursor is disabled. I just realised that even with this fixed, the selection indicator doesn't appear/disappear when you toggle the option from a script, since the region isn't updated.

  • I don't want to update all buffers every time we dismiss a message.
  • I guess you could update the last region from within the show cursor script. That might result in an unnecessary update, but at least it's isolated.
  • Alternatively, you could move selection drawing into BrailleHandler alongside cursor drawing. You'd still need to notify BrailleHandler, though, as there won't be a blink timer to do the work for you.

—
Reply to this email directly or view it on GitHub #5410 (comment) . https://github.com/notifications/beacon/AC9y8bAc6Hg5Kk7OvgL_TELx8qj3sksNks5pJpFNgaJpZM4Gh7VP.gif

@jcsteh
Copy link
Contributor

jcsteh commented Nov 26, 2015

I'm confused. In #5198, I wrote:

I think the command should toggle both the cursor and selection.

And you replied:

I agree. ... SuperNova also uses one command to toggle both.

So, is this a re-think or am I missing something?

@dkager
Copy link
Collaborator

dkager commented Nov 26, 2015

This is a re-think, I don’t want to fall into the trap of emulating an existing product. I’ll give it some serious thought and come up with a proper argument in #5198.

From: James Teh [mailto:notifications@github.com]
Sent: Thursday, November 26, 2015 07:32
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Braille doesn't scroll to same-line search result in Notepad (#5410)

I'm confused. In #5198 #5198 , I wrote:

I think the command should toggle both the cursor and selection.

And you replied:

I agree. ... SuperNova also uses one command to toggle both.

So, is this a re-think or am I missing something?

—
Reply to this email directly or view it on GitHub #5410 (comment) . https://github.com/notifications/beacon/AC9y8U1E0Qnd_J0zAAx_jc8fGdBjCas6ks5pJp7XgaJpZM4Gh7VP.gif

@jcsteh jcsteh closed this as completed in a5afa2c Dec 10, 2015
@dkager dkager mentioned this issue Feb 22, 2016
Closed
michaelDCurran pushed a commit that referenced this issue Jul 24, 2016
@dkager @michaelDCurran
Add a global command to move braille to the current focus (#5491)
4c671a6 
* On a pending caret move if there is no cursor, try to scroll the braille display to the selection.

* Move the marking of a selection to braille.Region.
* Add Region.brailleSelectionStart/End.
* If there is a selection but not a cursor, e.g. in a TextInfoRegion, scroll the braille display to the selection start.

* Add a global command to move braille to the focus, and the caret or cursor within the focus object if possible.
* If tethered to review, move braille to the focus object.
* Otherwise, move braille to the main buffer's last region, which should contain the focus. If it has a cursor/caret, scroll to it. If instead it has a selection, scroll to that.

* Check that there are actually some regions before selecting one.

* If no cursor is found, try scrolling to the selection. Requires #5410.

* Make the script documentation a bit clearer.

* Sync submodules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jcsteh jcsteh

Labels
bug component/braille
Projects
None yet
Milestone
2016.1
Development

No branches or pull requests
3 participants
@dkager @jcsteh @nvaccessAuto