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

Limit cursor blink rate #6470

Closed
derekriemer opened this Issue Oct 17, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@derekriemer
Collaborator

derekriemer commented Oct 17, 2016

Warning: the following STR may harm your display, don't try it with anything under 30 ms or so.

  1. braille settings.
  2. set cursor blink rate to 1 millisecond.
  3. press okay.
    Your display's cursor will go nuts. Can this be limited to something like 150 ms to prevent damage to the pins being forced up and down so rapidly? I haven't seen damage ever, but accidentally did this, and it didn't sound good for the display.
@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Oct 17, 2016

Contributor

This is an easy fix, and it seems unlikely anyone would want to set their cursor blink rate below say 200 ms (5 times a second), therefore P2.

When implementing

  • take into account the introduction of spin controls for this setting: see #6417
  • test the behaviour of NVDA when a previously set "too low" value is present. It's ok for this to round it up to the new minimum.
Contributor

feerrenrut commented Oct 17, 2016

This is an easy fix, and it seems unlikely anyone would want to set their cursor blink rate below say 200 ms (5 times a second), therefore P2.

When implementing

  • take into account the introduction of spin controls for this setting: see #6417
  • test the behaviour of NVDA when a previously set "too low" value is present. It's ok for this to round it up to the new minimum.
@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Oct 17, 2016

Collaborator

@feerrenrut note that 0 is used for don't blink the cursor

Collaborator

derekriemer commented Oct 17, 2016

@feerrenrut note that 0 is used for don't blink the cursor

feerrenrut added a commit that referenced this issue Nov 14, 2016

Limit braile cursor blink rate
Fix for #6470, limit the minimum blink rate to 200ms

@feerrenrut feerrenrut self-assigned this Nov 14, 2016

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Nov 14, 2016

Contributor

@derekriemer Thanks for pointing that out. Looking at the code, there does not seem to be any explicit decision for this behaviour, I think it more likely that this is a "happy accident" and probably a quirk of calling wx.PyTimer.Start() with a zero value.

When the value is set to zero, I assume the cursor is present (but not blinking)? As opposed to being disabled which could be achieved with the show cursor option.
In order to implement this, I think we would need to add another checkbox.

Is this a valuable feature? Considering this is undocumented behaviour, are people currently disabling the blink for the cursor by setting it to zero?

Contributor

feerrenrut commented Nov 14, 2016

@derekriemer Thanks for pointing that out. Looking at the code, there does not seem to be any explicit decision for this behaviour, I think it more likely that this is a "happy accident" and probably a quirk of calling wx.PyTimer.Start() with a zero value.

When the value is set to zero, I assume the cursor is present (but not blinking)? As opposed to being disabled which could be achieved with the show cursor option.
In order to implement this, I think we would need to add another checkbox.

Is this a valuable feature? Considering this is undocumented behaviour, are people currently disabling the blink for the cursor by setting it to zero?

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Nov 14, 2016

Contributor

@derekriemer Thanks for pointing that out. Looking at the code, there does not seem to be any explicit decision for this behaviour, I think it more likely that this is a "happy accident" and probably a quirk of calling wx.PyTimer.Start() with a zero value.

I probably should have thought of this. :) Actually, it's not an accident. See around line 1517 of braille.py:

        blinkRate = config.conf["braille"]["cursorBlinkRate"]
        if blinkRate:
            self._cursorBlinkTimer = wx.PyTimer(self._blink)

Note the if blinkRate. That evaluates to False if blinkRate is 0, which means the timer won't get set.

When the value is set to zero, I assume the cursor is present (but not blinking)?

Correct.

Is this a valuable feature?

I think some users will want it, but maybe @dkager could shed more light?

Contributor

jcsteh commented Nov 14, 2016

@derekriemer Thanks for pointing that out. Looking at the code, there does not seem to be any explicit decision for this behaviour, I think it more likely that this is a "happy accident" and probably a quirk of calling wx.PyTimer.Start() with a zero value.

I probably should have thought of this. :) Actually, it's not an accident. See around line 1517 of braille.py:

        blinkRate = config.conf["braille"]["cursorBlinkRate"]
        if blinkRate:
            self._cursorBlinkTimer = wx.PyTimer(self._blink)

Note the if blinkRate. That evaluates to False if blinkRate is 0, which means the timer won't get set.

When the value is set to zero, I assume the cursor is present (but not blinking)?

Correct.

Is this a valuable feature?

I think some users will want it, but maybe @dkager could shed more light?

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Nov 14, 2016

Contributor

It wouldn't be hard to add an explicit option for this, however given its undocumented we should consider whether we should do this.

Contributor

feerrenrut commented Nov 14, 2016

It wouldn't be hard to add an explicit option for this, however given its undocumented we should consider whether we should do this.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Nov 14, 2016

Contributor
Contributor

jcsteh commented Nov 14, 2016

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Nov 14, 2016

Contributor

Ok i'll extend my PR to cover this.

Contributor

feerrenrut commented Nov 14, 2016

Ok i'll extend my PR to cover this.

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Nov 14, 2016

Collaborator

Off the top of my head, SuperNova has different cursor shapes and has a blinking and a non-blinking version of each. That seems to be a bit muddy, the shape and the blinking are two separate things. The "0 to disable" semantics make more sense. I agree 200 ms is a sane minimum.

Would rate limiting braille in general make be useful? E.g. when rapidly scrolling through a menu the display goes nuts too.

Collaborator

dkager commented Nov 14, 2016

Off the top of my head, SuperNova has different cursor shapes and has a blinking and a non-blinking version of each. That seems to be a bit muddy, the shape and the blinking are two separate things. The "0 to disable" semantics make more sense. I agree 200 ms is a sane minimum.

Would rate limiting braille in general make be useful? E.g. when rapidly scrolling through a menu the display goes nuts too.

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Nov 16, 2016

Contributor

Rate limiting the output of braille generally might make sense, but I think that should be handled as a different issue. While it could potentially share / change the implementation of this issue, it raises other questions and there are more things to consider than with limiting the cursor blink rate.

If you would prefer not to have a new check box as an explicit way to disable cursor blinking please comment on the pull request #6558. Or if you would like to try out this implementation to see if it works for you take a look at branch i6470-LimitCursorBlinkRate

Contributor

feerrenrut commented Nov 16, 2016

Rate limiting the output of braille generally might make sense, but I think that should be handled as a different issue. While it could potentially share / change the implementation of this issue, it raises other questions and there are more things to consider than with limiting the cursor blink rate.

If you would prefer not to have a new check box as an explicit way to disable cursor blinking please comment on the pull request #6558. Or if you would like to try out this implementation to see if it works for you take a look at branch i6470-LimitCursorBlinkRate

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Nov 16, 2016

Collaborator

Yes, rate limiting in general is a separate issue. It was just a side thought here.
I usually have the cursor disabled altogether, but will try the branch you mentioned. Thanks!

Collaborator

dkager commented Nov 16, 2016

Yes, rate limiting in general is a separate issue. It was just a side thought here.
I usually have the cursor disabled altogether, but will try the branch you mentioned. Thanks!

feerrenrut added a commit that referenced this issue Dec 29, 2016

Fix #6470, limit the minimum blink rate to 200ms
Introduce a way to use the GUI to disable cursor blinking.

Rely on config update step to fix min values

The config schema updates should now fix values below the minimum.
Use validation data from config for min / max values in spin controls

feerrenrut added a commit that referenced this issue Jan 5, 2017

Incubates #6558
Re issue #6470
Merge remote-tracking branch 'origin/i6470-LimitCursorBlinkRate' into next

Conflicts:
	source/config/__init__.py

To resolve, a line added to the configSchema needed to be moved into the
configSpec.py file. The line was:
	readNumbersAs = integer(default=0,min=0,max=3)

feerrenrut added a commit that referenced this issue Jan 19, 2017

Update changes file for PR #6558
- The minimum braille cursor blink rate is now 200ms. Profiles or configurations with values below this will be increased to 200ms. (Issue #6470)
- A check box has been added to the braille settings dialog to allow enabling/disabling braille cursor blinking. Previously a value of zero was used to achieve this. (Issue #6470)
- Profiles and configuration files are now automatically upgraded to meet the requirements of schema modifications. If there is an error during upgrade, a notification is shown, the configuration is reset, and the old configuration file is available in the NVDA log at 'Info' level. (Issue #6470)

@feerrenrut feerrenrut added this to the 2017.1 milestone Jan 19, 2017

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