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

Increase default caret move timeout from 30 to 100 ms and make it configurable #7201

Merged
merged 5 commits into from
Jul 10, 2017
Merged

Increase default caret move timeout from 30 to 100 ms and make it configurable #7201

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a65c5b9
Increase default caret move timeout from 30 to 100 ms and make it con…
jcsteh May 24, 2017
5da54fe
Review actions.
jcsteh Jun 1, 2017
c967b21
Avoid full caret move timeout when pressing the delete key by treatin…
jcsteh Jun 21, 2017
e526792
Fix regression of cursor tracking in Chrome caused by c967b21a. Chrom…
jcsteh Jun 22, 2017
4a908f3
Fix regression of Automatic focus mode for caret movement (#7323) cau…
jcsteh Jun 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/config/configSpec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# -*- coding: UTF-8 -*-
#A part of NonVisual Desktop Access (NVDA)
#Copyright (C) 2006-2016 NV Access Limited
#Copyright (C) 2006-2017 NV Access Limited
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.

Expand Down Expand Up @@ -192,6 +192,9 @@

[upgrade]
newLaptopKeyboardLayout = boolean(default=false)

[editableText]
caretMoveTimeout = integer(min=0, max=2000, default=100)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be tempted to mention the units as part of the name for this config option, something like caretMoveTimeoutMS or caretMoveTimeoutMilliSec

""").format(latestSchemaVersion=latestSchemaVersion)

#: The configuration specification
Expand Down
26 changes: 20 additions & 6 deletions source/editableText.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#A part of NonVisual Desktop Access (NVDA)
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.
#Copyright (C) 2006-2012 NV Access Limited
#Copyright (C) 2006-2017 NV Access Limited

"""Common support for editable text.
@note: If you want editable text functionality for an NVDAObject,
Expand All @@ -21,6 +21,7 @@
from scriptHandler import isScriptWaiting, willSayAllResume
import textInfos
import controlTypes
from logHandler import log

class EditableText(ScriptableObject):
"""Provides scripts to report appropriately when moving the caret in editable text fields.
Expand All @@ -42,21 +43,30 @@ class EditableText(ScriptableObject):
#: Whether or not to announce text found before the caret on a new line (e.g. auto numbering)
announceNewLineText=True

def _hasCaretMoved(self, bookmark, retryInterval=0.01, timeout=0.03):
def _hasCaretMoved(self, bookmark, retryInterval=0.01, timeout=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

In the description of this PR you mention backwards compat for this function I think? Changing timeout to None will risk breaking that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's certainly a valid point, but I think it's a negligible/acceptable risk.

  1. It's most likely that a caller would call this function either accepting the defaults or passing its own timeout, both of which will still work as expected.
  2. This doesn't call super, so there's no chance that some method will receive None that wasn't expecting it.
  3. The only risk I can think of is that a subclass might have hard-coded the old 0.03 default, in which case they won't get the user's configured value. However, at worst, this means some subclass won't behave consistently with the rest of NVDA, and this would be pretty clear from the debug log if caret movement started timing out. I also think the likelihood of someone having done this is unlikely.
  4. Since it's a private method, maintaining backwards compat is a courtesy anyway, not an obligation. :)

"""
Waits for the caret to move, for a timeout to elapse, or for a new focus event or script to be queued.
@param bookmark: a bookmark representing the position of the caret before it was instructed to move
@type bookmark: bookmark
@param retryInterval: the interval of time in seconds this method should wait before checking the caret each time.
@type retryInterval: float
@param timeout: the over all amount of time in seconds the method should wait before giving up completely.
@param timeout: the over all amount of time in seconds the method should wait before giving up completely,
C{None} to use the value from the configuration.
@type timeout: float
@return: a tuple containing a boolean denoting whether this method timed out, and a TextInfo representing the old or updated caret position or None if interupted by a script or focus event.
@rtype: tuple
"""
"""
if timeout is None:
timeout = config.conf["editableText"]["caretMoveTimeout"]
else:
# This function's arguments are in seconds, but we want ms.
timeout *= 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Since timeout is documented on the function to be in seconds, I think it could be easy to miss that its converted into milliseconds. Perhaps store the milliseconds in a new var, timeoutMs

# time.sleep accepts seconds, so retryInterval is in seconds.
# Convert to integer ms to avoid floating point precision errors when adding to elapsed.
retryMs = int(retryInterval * 1000)
elapsed = 0
newInfo=None
while elapsed < timeout:
while True:
if isScriptWaiting():
return (False,None)
api.processPendingEvents(processEventQueue=False)
Expand All @@ -70,9 +80,13 @@ def _hasCaretMoved(self, bookmark, retryInterval=0.01, timeout=0.03):
newInfo=None
else:
if newBookmark!=bookmark:
log.debug("Caret moved. Elapsed: %d ms" % elapsed)
return (True,newInfo)
if elapsed >= timeout:
break
time.sleep(retryInterval)
elapsed += retryInterval
elapsed += retryMs
log.debug("Caret didn't move before timeout. Elapsed: %d ms" % elapsed)
return (False,newInfo)

def _caretScriptPostMovedHelper(self, speakUnit, gesture, info=None):
Expand Down