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

Input Gestures dialog: Async filtering for smoother response #10307

Merged
merged 8 commits into from Jul 2, 2020

Conversation

JulienCochuyt
Copy link
Collaborator

@JulienCochuyt JulienCochuyt commented Oct 1, 2019

Link to issue number:

None.

Summary of the issue:

In the Input Gesture dialog, speed typing a filter string leads to slow responsiveness from the UI and NVDA itself.

Description of how this pull request fixes the issue:

Slightly delay the refresh of the gesture tree by 300 ms.
Encapsulate the tree refresh with Freeze / Thaw

Testing performed:

Known issues with pull request:

Change log entry:

I'm not sure this deserves to be announced.
Otherwise:

Section: Bug fixes
In the Input Gestures dialog, speed typing a filter string now longer makes NVDA lag.

@CyrilleB79
Copy link
Contributor

CyrilleB79 commented Oct 1, 2019

+1
I have noted this lag and was thinking to open an issue for that. So IMO the change log entry is important.

@feerrenrut
Copy link
Member

feerrenrut commented Oct 2, 2019

This looks good, and is certainly an improvement! Though on my computer, I think 300ms feels like too long. I had a moment of doubt that filtering was working while typing.

Would you mind doing some timing measurements to see where the delay comes from? Is it the filtering we perform, or the redrawing of the control? Perhaps there are some other ways to optimize these. If the drawing is slow we could look at using virtual items, this was quite successful in the symbols dialog

@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented Oct 2, 2019

@feerrenrut: The virtual items technique is interesting and indeed pretty successful on the symbols dialog.
In the present case, I'm afraid two factors come into play:

  • The filtering implementation by itself is quite slow.
  • The typed search terms are usually much lengthy than in the symbols dialog.
    This PR addresses these two factors by simply ensuring the filtering is indeed run only when you end typing.
    Nevertheless, I can try to provide precise measurements and maybe reduce the timer to 200 ms or so.

Also, I'd be rather interested if this change could get adopted quite shortly, as I'm currently prototyping a much bigger impact on the same dialog to fix simultaneously #3605, #3848 and #10302.

@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented Oct 2, 2019

Alright, my bad, the drawing part takes up much more time than I thought compared to the filtering itself.
I'll have a closer look at the VirtualTree approach…

@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented Oct 5, 2019

@feerrenrut: Refactored using VirtualTree and applied your suggestion from #10308 (comment)
We may have mixed those threads a little :)

The filtering operation is now ran in a separate cancelable thread, and ends by triggering the actual tree refresh back in the wx main thread.
The tree refresh is delayed by 300 ms unless the filter was cleared.
Expanding the categories is also cancelled as soon as a new filtering starts.

NVDA does not stutter at all during filtering on my machine, even when speed typing short common filter terms and speech rate set to pretty high.

@feerrenrut
Copy link
Member

feerrenrut commented Oct 7, 2019

I find that sometimes when typing one or two characters the GUI locks while expanding / contracting items.

Perhaps if there is above a certain limit of results (perhaps 10?) we don't expand the options. My guess is that if the user doesn't know exactly what to filter for, and gets many results, it will be helpful to browse the results by category anyway. If we do this, it would be nice if the categories told you how many results in each category (as an indication the filtering worked). EG: "Braille (5 results) collapsed 1 of 18 level 0"

What do you think?

@JulienCochuyt JulienCochuyt changed the title Input Gestures dialog: Delay applying filter for smoother response Input Gestures dialog: Async filtering for smoother response Oct 28, 2019
@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented Oct 28, 2019

Perhaps if there is above a certain limit of results (perhaps 10?) we don't expand the options. My guess is that if the user doesn't know exactly what to filter for, and gets many results, it will be helpful to browse the results by category anyway. If we do this, it would be nice if the categories told you how many results in each category (as an indication the filtering worked). EG: "Braille (5 results) collapsed 1 of 18 level 0"

Expanding categories only when there are 10 results or less actually seems to allow for dropping the delay altogether.
The new version is ready for you to please review once more.

@CyrilleB79
Copy link
Contributor

CyrilleB79 commented Jan 21, 2020

I have just tested the last try build. Very nice!

Here with Zoomtext 11 and NVDA 2019.3beta2, filtering is very laggy. It takes about 10 seconds (ten seconds !) between the end of quick typing and the tree being updated, when I try to type "activer" (i.e. "enable" in French). Without Zoomtext, it is a bit laggy but not so much about 2-3 second.

With the try build of this PR, there is no noticeable delay! Very good job.

There is just a little remaining issue in this PR. Anyway, if this issue could not be fixed soon or easily, I would prefer see this PR merged, even with this little issue.

Steps to reproduce:

  • Open gesture dialog
  • In the filter field, type something, e.g. "braille"
  • Press control+home and then Delete to delete the content of the filter field
  • Type slowly "enable" (with English NVDA UI)

Actual result:

Strangely "selection removed" message is heard when typing the "b" letter.

Expected result:

selection removed should be heard when typing the Delete key.

I understand that this PR cannot be merged anymore in NVDA 2019.3. Anyway, I hope to see it merged very soon in 2020.1.

@CyrilleB79
Copy link
Contributor

CyrilleB79 commented Apr 7, 2020

I hope this PR is not delayed due to my last comment about delayed "selection removed" message.

@JulienCochuyt do you plan to investigate/fix this little issue or not? Again, IMO it is not a blocking issue, but just let us know.

@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented May 4, 2020

@CyrilleB79 wrote:

do you plan to investigate/fix this little issue or not? Again, IMO it is not a blocking issue, but just let us know.

Sorry for the late answer.
Thanks for your support, and your thorough testing.
I must confess I have no clue what causes this message to be delayed.
It is sent by editableText.EditableText.detectPossibleSelectionChange which, at this stage considers the selection has changed from "enab" to "", which of course is wrong.
There might be a race condition somewhere in how events are triggered/handled...

@feerrenrut
Copy link
Member

feerrenrut commented Jul 2, 2020

Testing this out, it seems to work really well. Good job @JulienCochuyt.

@CyrilleB79 I can replicate the issue you refer to in #10307 (comment)

The announced "selection removed" does not seem to be timing based, it happens consistently when the third character is typed, even if I type "e" "n" "backspace" "n" "a" it happens on the "a".

When this is merged please create a bug for this, it will need separate investigation.

Copy link
Member

@feerrenrut feerrenrut left a comment

def __init__(self, parent):
super().__init__(
parent,
size=wx.Size(600, 400),
Copy link
Member

@feerrenrut feerrenrut Jul 2, 2020

Choose a reason for hiding this comment

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

Until wx properly supports DPI settings we need to accommodate manually. See the scale size helper. To test change your DPI setting in windows, log out and back in, then re-run NVDA.

Though I now note that this is just moved code. I'll accept this without changes.

@feerrenrut feerrenrut merged commit 02c481d into nvaccess:master Jul 2, 2020
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants