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

Braille message timeout, 0 to disable #6669

Closed
dkager opened this Issue Dec 23, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@dkager
Collaborator

dkager commented Dec 23, 2016

IMO the value 0 should lead to no timeout. Currently this value disables messages altogether, which isn't very useful. If it is needed to disable messages it should be a global toggle that also affects speech.

To make this work:

  • Implement in braille.py.
  • Change the setting label to "Message timeout (sec, 0 to disable)".

Related: #6470

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jan 3, 2017

Contributor
Contributor

jcsteh commented Jan 3, 2017

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Jan 3, 2017

Collaborator

Do you have a desire for keeping them there forever?

Not really, just long enough to read them if they are a bit more complex than usual text. A longer timeout is fine, though to me "0 to disable" feels more natural than "0 to completely turn off the feature". As a compromise, could we update the label to include "(0 to disable messages)"?

Collaborator

dkager commented Jan 3, 2017

Do you have a desire for keeping them there forever?

Not really, just long enough to read them if they are a bit more complex than usual text. A longer timeout is fine, though to me "0 to disable" feels more natural than "0 to completely turn off the feature". As a compromise, could we update the label to include "(0 to disable messages)"?

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Jan 3, 2017

Collaborator
Collaborator

derekriemer commented Jan 3, 2017

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jan 3, 2017

Contributor

@dkager commented on 4 Jan. 2017, 2:18 am AEST:

A longer timeout is fine, though to me "0 to disable" feels more natural than "0 to completely turn off the feature". As a compromise, could we update the label to include "(0 to disable messages)"?

I guess that's an option. Still, perhaps this whole magic value thing is just too ugly and we should just have an explicit check box. @feerrenrut, @Qchristensen, thoughts?

@derekriemer commented on 4 Jan. 2017, 3:07 am AEST:

Random idea, but why not make the timeout increase a little if the user
presses the right advancer button.

It already does. In fact, it resets the timeout completely if the user scrolls either right or left.

Contributor

jcsteh commented Jan 3, 2017

@dkager commented on 4 Jan. 2017, 2:18 am AEST:

A longer timeout is fine, though to me "0 to disable" feels more natural than "0 to completely turn off the feature". As a compromise, could we update the label to include "(0 to disable messages)"?

I guess that's an option. Still, perhaps this whole magic value thing is just too ugly and we should just have an explicit check box. @feerrenrut, @Qchristensen, thoughts?

@derekriemer commented on 4 Jan. 2017, 3:07 am AEST:

Random idea, but why not make the timeout increase a little if the user
presses the right advancer button.

It already does. In fact, it resets the timeout completely if the user scrolls either right or left.

@Qchristensen

This comment has been minimized.

Show comment
Hide comment
@Qchristensen

Qchristensen Jan 4, 2017

I can see both arguments to what 0 should mean here. There is a logic to 0 disabling messages, as if messages time out after 0 seconds, then you would never get a chance to read them anyway. I think the label should specify what a value of 0 means if not that messages instantly disappear.

A checkbox to specify whether to display messages and then a value for how long to display them would be one extra control but would probably be the clearest option.

Qchristensen commented Jan 4, 2017

I can see both arguments to what 0 should mean here. There is a logic to 0 disabling messages, as if messages time out after 0 seconds, then you would never get a chance to read them anyway. I think the label should specify what a value of 0 means if not that messages instantly disappear.

A checkbox to specify whether to display messages and then a value for how long to display them would be one extra control but would probably be the clearest option.

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Jan 16, 2017

Contributor

An alternative might be to change the way that the values are reported. In some applications "slider" type controls have special values at each end, the minimum value might report as "messages disabled" and the maximum value might report as "no timeout" and in between are the values 1 - X. For this to work there needs to be a label, or read only text control (or similar) that shows the current value of the slider.

However if we only need one special value a checkbox would be simpler.

Contributor

feerrenrut commented Jan 16, 2017

An alternative might be to change the way that the values are reported. In some applications "slider" type controls have special values at each end, the minimum value might report as "messages disabled" and the maximum value might report as "no timeout" and in between are the values 1 - X. For this to work there needs to be a label, or read only text control (or similar) that shows the current value of the slider.

However if we only need one special value a checkbox would be simpler.

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Jan 16, 2017

Collaborator

I'd go checkbox, also because no other setting currently uses special sliders as far as I know.

Collaborator

dkager commented Jan 16, 2017

I'd go checkbox, also because no other setting currently uses special sliders as far as I know.

feerrenrut added a commit that referenced this issue Apr 27, 2017

Incubate #6777
Re Issue: #6669
Merge branch 'dkager-i6669' into next

@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone May 30, 2017

feerrenrut added a commit that referenced this issue May 30, 2017

Update changes file for several PRs
- PR #7169 : Editable div elements in Chrome are no longer have their label reported as their value while in browse mode. (Issue #7153)
- PR #6396 : An unbound gesture (script_restart) has been added to allow NVDA to be restarted quickly. (PR #6396)
- PR #6777 : A Braille setting has been added to "show messages indefinitely". (Issue #6669)
- PR #7133 : Pressing end while in browse mode of an empty Microsoft Word document no longer causes a runtime error. (Issue #7009)
- PR #6868 : The keyboard layout can now be set from the NVDA Welcome dialog. (Issue #6863)
- PR #6813 : The names of "landmarks" are abbreviated in Braille (Issue #3975)
@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager May 30, 2017

Collaborator

@feerrenrut I found a corner case that yields an error. It's pretty innocent but should still be addressed. Apologies for only finding this on master!

STR:

  1. Make sure the default profile does not have the setting to show messages indefinitely.
  2. Create an app-specific config profile and choose the new feature to show messages indefinitely.
  3. Go to the application and trigger a message, e.g. by pressing NVDA+t to report the title bar.
  4. Go to the desktop or switch out of the application.

Expected: the message clears.
Actual: it does, but an error is raised:

ERROR - eventHandler.executeEvent (18:06:08):
error executing event: gainFocus on <NVDAObjects.IAccessible.sysListView32.ListItem object at 0x04F6CE10> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 143, in executeEvent
  File "eventHandler.pyc", line 91, in __init__
  File "eventHandler.pyc", line 98, in next
  File "appModules\explorer.pyc", line 280, in event_gainFocus
  File "eventHandler.pyc", line 98, in next
  File "NVDAObjects\__init__.pyc", line 900, in event_gainFocus
  File "braille.pyc", line 1676, in handleGainFocus
  File "braille.pyc", line 1692, in _doNewObject
  File "braille.pyc", line 1667, in _dismissMessage
AttributeError: 'NoneType' object has no attribute 'Stop'

To fix this and make the code a bit more robust too I think we need to change the following line in braille.BrailleHandler._dismissMessage from:

if not config.conf["braille"]["noMessageTimeout"]:

To:

if self._messageCallLater:
Collaborator

dkager commented May 30, 2017

@feerrenrut I found a corner case that yields an error. It's pretty innocent but should still be addressed. Apologies for only finding this on master!

STR:

  1. Make sure the default profile does not have the setting to show messages indefinitely.
  2. Create an app-specific config profile and choose the new feature to show messages indefinitely.
  3. Go to the application and trigger a message, e.g. by pressing NVDA+t to report the title bar.
  4. Go to the desktop or switch out of the application.

Expected: the message clears.
Actual: it does, but an error is raised:

ERROR - eventHandler.executeEvent (18:06:08):
error executing event: gainFocus on <NVDAObjects.IAccessible.sysListView32.ListItem object at 0x04F6CE10> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 143, in executeEvent
  File "eventHandler.pyc", line 91, in __init__
  File "eventHandler.pyc", line 98, in next
  File "appModules\explorer.pyc", line 280, in event_gainFocus
  File "eventHandler.pyc", line 98, in next
  File "NVDAObjects\__init__.pyc", line 900, in event_gainFocus
  File "braille.pyc", line 1676, in handleGainFocus
  File "braille.pyc", line 1692, in _doNewObject
  File "braille.pyc", line 1667, in _dismissMessage
AttributeError: 'NoneType' object has no attribute 'Stop'

To fix this and make the code a bit more robust too I think we need to change the following line in braille.BrailleHandler._dismissMessage from:

if not config.conf["braille"]["noMessageTimeout"]:

To:

if self._messageCallLater:
@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager May 30, 2017

Collaborator

Here is a patch:

diff --git a/source/braille.py b/source/braille.py
index a377834cf..06baebe6f 100644
--- a/source/braille.py
+++ b/source/braille.py
@@ -1663,7 +1663,7 @@ class BrailleHandler(baseObject.AutoPropertyObject):
 		"""
 		self.buffer.clear()
 		self.buffer = self.mainBuffer
-		if not config.conf["braille"]["noMessageTimeout"]:
+		if self._messageCallLater:
 			self._messageCallLater.Stop()
 			self._messageCallLater = None
 		self.update()

This patch preserves the original timeout, or no timeout if that option was enabled.
Note that the message being shown is likely to disappear when changing config profiles, since this usually implies a focus change.

Collaborator

dkager commented May 30, 2017

Here is a patch:

diff --git a/source/braille.py b/source/braille.py
index a377834cf..06baebe6f 100644
--- a/source/braille.py
+++ b/source/braille.py
@@ -1663,7 +1663,7 @@ class BrailleHandler(baseObject.AutoPropertyObject):
 		"""
 		self.buffer.clear()
 		self.buffer = self.mainBuffer
-		if not config.conf["braille"]["noMessageTimeout"]:
+		if self._messageCallLater:
 			self._messageCallLater.Stop()
 			self._messageCallLater = None
 		self.update()

This patch preserves the original timeout, or no timeout if that option was enabled.
Note that the message being shown is likely to disappear when changing config profiles, since this usually implies a focus change.

feerrenrut added a commit that referenced this issue May 31, 2017

Fix exception: using indefinite braille messages
This fixes an exception when using the indefinite braille messages
feature. For more information see comment on PR #6669
(#6669 (comment))

STR:
- Make sure the default profile does not have the setting to show messages indefinitely.
- Create an app-specific config profile and choose the new feature to show messages indefinitely.
- Go to the application and trigger a message, e.g. by pressing NVDA+t to report the title bar.
- Go to the desktop or switch out of the application.
@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut May 31, 2017

Contributor

Thanks @dkager, I have tested this and updated in master, and will merge master into next. Please see PR #7230

Contributor

feerrenrut commented May 31, 2017

Thanks @dkager, I have tested this and updated in master, and will merge master into next. Please see PR #7230

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