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

Support for uncontracted and contracted braille input. #6449

Merged
merged 29 commits into from Jul 7, 2017

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Oct 11, 2016

Beyond the main code in brailleInput to support uncontracted/contracted input, this includes the following changes:

  • The list of braille tables has been moved out of the braille module into a separate brailleTables module. Braille tables are now added with a function rather than directly adding them to the data structure. Aside from being necessary in order to specify and check whether a table is contracted, this also makes the data about tables more extensible in future.
  • As the data structure for braille tables has now changed and is no longer ordered, this was a good opportunity to sort the list of tables alphabetically when displaying them to the user.
  • brailleInput is now notified when reverting config or changing config profiles. This is necessary because brailleInput now maintains some state when the input table is changed.
  • brailleInput is now initialised before braille at startup. This is because braille depends on brailleInput to get the currently untranslated input.
  • Dot7 and dot8 are now universally bound to braille input specific scripts for erase and enter. Any braille display drivers that had bindings for backspace/enter for braille input have been adjusted accordingly.
  • In the User Guide, a "Braille" section has been added above "Application Specific Features". The "Braille Control Types and States" section has been moved to a sub-section of this, and a sub-section on Braille Input has been added.

Fixes #2439. Fixes #6054. Fixes #6113. Fixes #6935.

What's New entries:

In New Features:

- You can now type in both contracted and uncontracted braille on a braille display with a braille keyboard. See the Braille Input section of the User Guide for details. (#2439, #6054)

In Changes:

- The output and input table lists in the Braille Settings dialog are now sorted alphabetically. (#6113)
- Updated liblouis braille translator to 3.2.0. (#6935)

Beyond the main code in brailleInput to support uncontracted/contracted input, this includes the following changes:
* The list of braille tables has been moved out of the braille module into a separate brailleTables module. Braille tables are now added with a function rather than directly adding them to the data structure. Aside from being necessary in order to specify and check whether a table is contracted, this also makes the data about tables more extensible in future.
* As the data structure for braille tables has now changed and is no longer ordered, this was a good opportunity to sort the list of tables alphabetically when displaying them to the user.
* brailleInput is now notified when reverting config or changing config profiles. This is necessary because brailleInput now maintains some state when the input table is changed.
* brailleInput is now initialised before braille at startup. This is because braille depends on brailleInput to get the currently untranslated input.
* Dot7 and dot8 are now universally bound to braille input specific scripts for erase and enter. Any braille display drivers that had bindings for backspace/enter for braille input have been adjusted accordingly.
@jcsteh
Copy link
Contributor Author

jcsteh commented Oct 11, 2016

@feerrenrut, would you mind reviewing this please? Note that I've tried to explain some of the more obscure changes in the description of this pull request.

@@ -951,6 +680,15 @@ def update(self):
chunk.collapse()
chunk.setEndPoint(sel, "endToStart")
self._addTextWithFields(chunk, formatConfig)
# If the user is entering braille, place any untranslated braille before the selection.
import brailleInput
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this import local and not at the top of the file? Perhaps add a comment

def routeTo(self, braillePos):
if self._brailleInputStart is not None and self._brailleInputStart <= braillePos <= self._brailleInputEnd:
# The user is moving within untranslated braille input.
import brailleInput
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, not at the top of the file?

if text:
rawInputStart = len(self.rawText)
self._addFieldText(text, None, separate=False)
rawInputEnd = len(self.rawText)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? rawImputStart and rawInputEnd are both being initialised to the same thing:len(self.rawText). If so I think the intent is clearer if it is written rawInputEnd=rawInputStart

@@ -985,7 +723,22 @@ def update(self):
self.focusToHardLeft = self._isMultiline()
super(TextInfoRegion, self).update()

if rawInputStart is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to also check that rawInputEnd is not None

@@ -270,10 +270,10 @@ def display(self, cells):
"kb:end": ("br(braillenote):space+d4+d5",),
"kb:control+home": ("br(braillenote):space+d1+d2+d3",),
"kb:control+end": ("br(braillenote):space+d4+d5+d6",),
"kb:enter": ("br(braillenote):space+d8",),
"braille_enter": ("br(braillenote):space+d8",),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this change is a new line conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird. This doesn't even show up in diffs from command line git. I reckon this is a GitHub bogus thingy. :(


def script_braille_enter(self, gesture):
brailleInput.handler.enter()
script_braille_enter.__doc__= _("Translates any braille input and presses the enter key")
Copy link
Contributor

Choose a reason for hiding this comment

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

missing translators comment

@@ -1459,10 +1463,9 @@ def makeSettings(self, settingsSizer):
sizer = wx.BoxSizer(wx.HORIZONTAL)
# Translators: The label for a setting in braille settings to select the input table (the braille table used to type braille characters on a braille keyboard).
label = wx.StaticText(self, wx.ID_ANY, label=_("&Input table:"))
self.inputTableNames = [table[0] for table in braille.INPUT_TABLES]
self.inputTableList = wx.Choice(self, wx.ID_ANY, choices=[table[1] for table in braille.INPUT_TABLES])
self.inputTableList = wx.Choice(self, wx.ID_ANY, choices=tableChoices)
Copy link
Contributor

Choose a reason for hiding this comment

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

wx.ID_ANY is unnecessary, I think this might conflict with next

source/speech.py Outdated
@type number: int
"""
global _suppressSpeakTypedCharactersData
_suppressSpeakTypedCharactersData = (number, time.time())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be called twice before number has had a chance to decrement to 0?

source/speech.py Outdated
timeOk = time.time() - supTime <= 0.1
suppress = number > 0 and timeOk
number -= 1
if number > 0 and timeOk:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic could be cleaned up so the condition does not need to be repeated. It would have the implication that _suppressSpeakTypedCharactersData exists for longer, and number is allowed to reach zero.

eg:

if _suppressSpeakTypedCharactersData:
    ...
    timeOk = time.time() - supTime <= 0.1

    suppress = number > 0 and timeOk
    if suppress:
        _suppressSpeakTypedCharactersData = (--number, supTime)
    else:
        _suppressSpeakTypedCharactersData = None
else:
    suppress = False
if not suppress and config.conf["keyboard"]["speakTypedCharacters"] and ord(ch)>=32:
    speakSpelling(realChar)

source/speech.py Outdated
_suppressSpeakTypedCharactersData = None
else:
suppress = False
if not suppress and config.conf["keyboard"]["speakTypedCharacters"] and ord(ch)>=32:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing 32 is a limit for non-printable characters? A named constant would make this more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite true. However, as that's very much unrelated code that existed previously (I just added an extra condition), I'd prefer to address this somewhere else.

…e, typing two cells then pressing backspace.

This occurred because cellsWithText was being updated even for the space at the end of a word. At this point, we're starting a new word, so cellsWithText needs to be empty.
@jcsteh
Copy link
Contributor Author

jcsteh commented Oct 12, 2016

@feerrenrut, I'm done with this batch of changes. I addressed your code review comments plus some other stuff noted below. Would you mind taking a look again?

@derekriemer reported a bug to me via IRC. STR:

  1. Enable input with UEB grade 2.
  2. Type "b" and press space.
  3. Press dot 6 twice.
  4. Press backspace.

This causes an exception. I've fixed this in 0b643d6.

I've also addressed a couple of other issues @derekriemer reported in recent comments on #2439.

@jcsteh
Copy link
Contributor Author

jcsteh commented Oct 12, 2016

@feerrenrut, I'm done with this batch of changes.

Actually, I just pushed one more commit to mask characters in protected fields (e.g. password fields). Thanks @derekriemer for catching!

jcsteh added a commit that referenced this pull request Oct 13, 2016
@nvaccessAuto nvaccessAuto assigned jcsteh and unassigned feerrenrut Oct 13, 2016
… several pending upstream changes related to braille input.
This necessitated the ability to have tables specific to either output or input, rather than assuming all tables can handle both.
* Always set noUndefinedDots so undefined dots (such as indicators that mean nothing by themselves) don't get translated to, for example, "\456/" for dots 4 5 6.
* Set partialTrans when reporting contracted cells, thus avoiding the "zz" hack which was breaking French where z is actually a contraction.
@jcsteh
Copy link
Contributor Author

jcsteh commented Nov 3, 2016

Holding this back from the 2016.4 release, as there are some major outstanding issues:

  • Investigate/fix effect of undefined dots on subsequent translation. For example, with UEB g2, ⠰⠁⠃ gets translated to "about" instead of "ab" with noUndefinedDots. Without that mode, you get "\56/ab".
  • When there is untranslated input, routing braille outside of the untranslated input should clear the untranslated input.
  • Further UEB back-translation fixes from APH.

@jcsteh jcsteh requested a review from feerrenrut June 19, 2017 04:45
…so, remove "grade 1" from the description, since there's no other grade.
@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 19, 2017

@feerrenrut, this is finally ready for another round of review. There have been a lot of changes and quite a long time since you last reviewed it, so you may want to just review from scratch. In case it's helpful, though, the changes since your last review are 5ff07e5b..ff6f627a.

#: * fileName: The file name of the table.
#: * displayname: The name of the table as displayed to the user. This should be translatable.
#: * contracted: C{True} if the table is contracted, C{False} if uncontracted.
BrailleTable = collections.namedtuple("BrailleTable", ("fileName", "displayName", "contracted", "output", "input"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring should also have output and input

source/speech.py Outdated
_suppressSpeakTypedCharactersTime = None
else:
suppress = False
if not suppress and config.conf["keyboard"]["speakTypedCharacters"] and ord(ch)>=32:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace the value 32 with a name please? I'm not really sure what this is testing for

#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) 2008-2016 NV Access Limited, Joseph Lee
Copy link
Collaborator

Choose a reason for hiding this comment

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

update copyright year

@jcsteh jcsteh requested a review from feerrenrut June 20, 2017 04:21
…rs; we got rid of that in #6962. Make it possible to get the display text from a braille keyboard gesture identifier (as used in the Input Gestures dialog).
@jcsteh jcsteh requested a review from feerrenrut June 21, 2017 05:12
@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 21, 2017

@feerrenrut, can you please review the latest commit? Sorry. :(

return self._makeDisplayText(self.dots, self.space)

@classmethod
def getDisplayTextForIdentifier(cls, identifier):
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this called?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a docstring on this to explain what its for? Also, it would be nice to see a couple of examples of what identifier might be.

jcsteh added a commit that referenced this pull request Jun 22, 2017
@derekriemer
Copy link
Collaborator

fixes #6956

@derekriemer
Copy link
Collaborator

I'm amazed at how many tickets this autoclosed. What a huge chunk of work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants