Limit braille cursor blink rate by feerrenrut · Pull Request #6558 · nvaccess/nvda

feerrenrut · 2016-11-14T05:01:54Z

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

This stops the blink rate from being set below 200ms. Configurations that are already set below 200ms will be modified, and set to 200ms.

There is some active discussion about being able to disable the braille cursor blinking, this was previously possible by setting the blink rate to 0.

feerrenrut · 2016-11-14T05:03:14Z

@jcsteh Could you take a look at this? Note the questions around disabling the cursor blinking.

feerrenrut · 2016-11-14T07:49:51Z

@jcsteh I have updated this so that there is an option to disable the cursor blinking. Since I dont have a braille display I cant really test that this works, other than the config saving / GUI side of things.

jcsteh

I'm fine with the GUI changes here.

Even though specifying 0 to disable blinking was never documented, it was definitely supported and I vaguely recall it was intentional. If so, there may be users using this. I'm wondering if we should keep the single setting in the config with a minimum of 0 and just do the separate boolean/integer with min 200 in the GUI. That way, we preserve backwards compat. That does mean the config spec doesn't enforce the minimum, though, which is a bit ugly.

There actually is a way to get the minimum value from the config spec, but it's ugly. Still, maybe we can write a function in config to hide the ugliness:

int(config.conf.validator._parse_with_caching(config.conf.spec["braille"]["cursorBlinkRate"])[2]["min"])

feerrenrut · 2016-11-21T04:11:26Z

Yep, good point. With the current approach we will lose the users "dont blink" setting and replace it with blink at 200ms. I'll look to see if there is a way that we can return the actual value (perhaps along with the exception) when the value is below the minimum. This way we get to keep the matching config spec, and upgrade the settings. In order to provide a cleaner approach perhaps its worth talking about an "upgrade" step for the config. Where we disable validation, and migrate known config spec changes. As an alternative we could never change the config spec and only introduce new items. Longer term that could get quite messy though.

Thanks for the code snippet, I actually had a bit of a look for getting that info, but gave up on it.

feerrenrut · 2016-11-25T05:25:57Z

@jcsteh I have updated this to include a config upgrade mechanism. There are a few other things I need to address on this branch but I would be interested to hear what you think of commit bc9e97d

Edit:
I forgot to mention that so far this does not backup the config files before upgrade (or between each step). I think this would be a good idea, it would give us much greater safety in the case that something went wrong during an upgrade and make it much easier to diagnose the problem. Even if there are many upgrade steps and many profiles (resulting in n*m redundancy) these files will compress well.

feerrenrut · 2016-11-30T03:32:29Z

@jcsteh I have pushed a couple of new changes to this.

I have added a helper to be able to get validation parameters, this is helpful for setting the limits on the GUI code EG the min and max for spinControls. Now these values are not repeated.
New / empty config files were previously having their schema version set to 0 and then being upgraded through to the latest. This upgrading a blank config through all steps is unnecessary since the latest schema should not require any previous, and has the potential to cause problems in the future if there are upgrade steps that rely on certain keys being present in a particular version which aren't.
We talked about archiving the old profiles, rather than this I am putting the config into the log. Yes, this will increase the size of the log and is a bit verbose but it means that if there is a problem there is a chance that we can get the config back / get an example of what caused the problem.

jcsteh

I'm very probably missing something, but you mentioned at some point that you log the old and new configs. Where do you do this? I see the validation result get logged if it fails, but I'm guessing that doesn't log the actual values? Or does it? I'm confused on this one.
Do you think this is one of those situations where it might be worth rebasing and splitting this into smaller commits based on functionality? We'd then do a full merge rather than a squash. I'm wondering whether this might be clearer given that the config changes aren't strictly related to cursor blink, even though they're necessary for this change.

jcsteh · 2016-12-16T06:26:47Z

 import textInfos
 import brailleDisplayDrivers
 import inputCore
+from validate import VdtValueTooSmallError


Import no longer needed.

jcsteh · 2016-12-16T06:31:09Z

+#A part of NonVisual Desktop Access (NVDA)
+#Copyright (C) 2016 NV Access Limited
+#This file is covered by the GNU General Public License.
+#See the file COPYING for more details.


The "coding" comment needs to be the first line of the file. We tend to put the docstring after the header comment. Also, I figured out the correct copyright for this file when working on another branch. So, the top few lines of this file should be:

# -*- coding: UTF-8 -*- #config\__init__.py #A part of NonVisual Desktop Access (NVDA) #Copyright (C) 2006-2016 NV Access Limited, Aleksey Sadovoy, Peter Vágner, Rui Batista, Zahari Yurukov #This file is covered by the GNU General Public License. #See the file COPYING for more details. """Manages NVDA configuration. """

jcsteh · 2016-12-16T06:52:30Z

 import easeOfAccess
 import winKernel
+import profileUpgrader
+from configSpec import confspec


We don't always follow this like we should, but this should be an explicit relative import, as implicit relative imports are gone in Python 3. So, do this:

from .configSpec import confspec

jcsteh · 2016-12-16T06:58:12Z

 		else:
 			try:
-				profile = ConfigObj(fn, indent_type="\t", encoding="UTF-8")
+				profile = self._loadConfig(fn) # a blank file will be created if fn does not exist


As discussed on Mumble, we don't think this does in fact create a blank file after all. However, it'd be great if you can verify this before and after this PR. Also, I'm wondering whether this changes our behaviour on a read-only file system. This would need to be tested both with and without an existing config.

jcsteh · 2016-12-18T23:25:15Z

 		self._handleProfileSwitch()

+	def _loadConfig(self, fn, fileError=False):
+		log.info("Loading config: {0}".format(fn))


I'm not certain, but I suspect fn might be a unicode string. This needs to be verified. If it is, you need to change the format template to be a unicode literal because Python won't coerce format templates from str to unicode (even though it does for the % operator).

jcsteh · 2016-12-18T23:49:06Z

+#See the file COPYING for more details.
+
+from logHandler import log
+from configSpec import latestSchemaVersion, confspec


Change to explicit relative import.

jcsteh · 2016-12-18T23:49:26Z

+from configSpec import latestSchemaVersion, confspec
+from configobj import flatten_errors
+from copy import deepcopy
+import profileUpgradeSteps


Change to explicit relative import.

jcsteh · 2016-12-18T23:50:02Z

+	oldConfSpec = profile.configspec
+	profile.configspec = confspec
+	result = profile.validate(validator, preserve_errors=True)
+	profile.confspec = oldConfSpec


Should this be profile.configspec?

jcsteh · 2016-12-18T23:51:49Z

 	updateCheck = None
 import inputCore
 import nvdaControls
+from validate import VdtValueTooSmallError


Unused import.

jcsteh · 2016-12-19T00:05:37Z

 It applies to the system caret and review cursor, but not to the selection indicator.

+==== Blink Cursor ====
+This option allows the braille cursor to blink. If blinking is turned off the braille cursor will just be static.


Please split this onto two lines, one per sentence.

I understand what you mean by "static", but I'm not sure many users will. Perhaps we should say "If blinking is turned off, the cursor will be constantly shown." Or we could change "constantly shown" to "always up" (i.e. the cursor dots are always up on the display).

When a schema is updated, config files (base config and profiles) can be upgraded to meet the new schema. The `schemaVersion` (in configSpec.py) should be incremented, and a new function should be added to the `profileUpgradeSteps.py` file to make the changes required to config files before they are loaded. After all upgrade steps are made the file is validated, and any errors will be in the log.

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

jcsteh · 2017-01-04T03:39:47Z

 from validate import Validator
-from logHandler import log
+from logHandler import log, levelNames
+from logging import DEBUG


This is also accessible as log.DEBUG (as opposed to the method log.debug). I thought I'd let you know in case you found it nicer to read, but I'm also happy for it to be left as is.

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)

derekriemer · 2017-01-06T09:40:22Z

+		spec = conf.spec
+		for nextKey in keyPath:
+			spec = spec[nextKey]
+		return conf.validator._parse_with_caching(spec)[2][validationParameter]


Is this a bug? (Hint: many for voice settings).

What i mean here is that in parts of the config, we have the parameter
[[__many__]]
and I don't see a way to get the min or max from those. Could this be triggering the bug that was mentioned on NVDA-devel? 1 and 2

I don't think this could be the cause of any current problems:

This is a new function that is only being used in one place right now (cursor blink rate) and that place doesn't involve a __many__.

Even though __many__ is used for speech synths, synths build their own specific config spec which overrides this.

The issue shown in the links to the NVDA-devel list were caused by a merge error (my fault). A line was missed from the config spec.

In order to get the validation parameters from a [[__many__]] section the specific name of the section must be used. As you would if you were to get a config value from that section. For an example of this see Line 511 in source/gui/settingsDialogs.py which looks like:
min=int(config.conf.getConfigValidationParameter(["speech", getSynth().name, "capPitchChange"], "min"))

derekriemer · 2017-01-06T09:59:32Z

+
 ==== Cursor Blink Rate (ms) ====
 This option is a numerical field that allows you to change the blink rate of the cursor in milliseconds.



Should the minimum value be mentioned?

- 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 assigned jcsteh Nov 14, 2016

feerrenrut mentioned this pull request Nov 16, 2016

Limit cursor blink rate #6470

Closed

jcsteh reviewed Nov 21, 2016

View reviewed changes

jcsteh requested changes Dec 19, 2016

View reviewed changes

feerrenrut added 2 commits December 29, 2016 13:26

feerrenrut force-pushed the i6470-LimitCursorBlinkRate branch from 076259c to 820c02f Compare December 29, 2016 05:38

feerrenrut requested a review from jcsteh December 29, 2016 05:43

feerrenrut mentioned this pull request Dec 30, 2016

Provide easy way to restart NVDA with Debug level logging enabled #6689

Closed

jcsteh approved these changes Jan 4, 2017

View reviewed changes

nvaccessAuto added the incubating label Jan 5, 2017

nvaccessAuto assigned feerrenrut and unassigned jcsteh Jan 5, 2017

derekriemer reviewed Jan 6, 2017

View reviewed changes

feerrenrut merged commit 6de9899 into master Jan 19, 2017

nvaccessAuto removed the incubating label Jan 19, 2017

nvaccessAuto added this to the 2017.1 milestone Jan 19, 2017

This was referenced May 1, 2018

Schema upgrade process improvements #7220

Open

Update configobj to 5.1.0dev #7945

Merged

feerrenrut deleted the i6470-LimitCursorBlinkRate branch January 17, 2020 09:07


		==== Cursor Blink Rate (ms) ====
		This option is a numerical field that allows you to change the blink rate of the cursor in milliseconds.

Uh oh!

Conversation

feerrenrut commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Nov 14, 2016

Uh oh!

feerrenrut commented Nov 14, 2016

Uh oh!

jcsteh left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Nov 21, 2016

Uh oh!

feerrenrut commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Nov 30, 2016

Uh oh!

jcsteh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feerrenrut commented Nov 14, 2016 •

edited

Loading

feerrenrut commented Nov 25, 2016 •

edited

Loading