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 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

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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)

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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:

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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",),

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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

This comment has been minimized.

@jcsteh

jcsteh Oct 11, 2016
Author Contributor

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")

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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)

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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

@type number: int
"""
global _suppressSpeakTypedCharactersData
_suppressSpeakTypedCharactersData = (number, time.time())

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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

timeOk = time.time() - supTime <= 0.1
suppress = number > 0 and timeOk
number -= 1
if number > 0 and timeOk:

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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)
_suppressSpeakTypedCharactersData = None
else:
suppress = False
if not suppress and config.conf["keyboard"]["speakTypedCharacters"] and ord(ch)>=32:

This comment has been minimized.

@feerrenrut

feerrenrut Oct 11, 2016
Contributor

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

This comment has been minimized.

@jcsteh

jcsteh Oct 11, 2016
Author Contributor

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.

jcsteh added 4 commits Oct 11, 2016
…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 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 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
jcsteh added 3 commits Oct 17, 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 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.
…so, remove "grade 1" from the description, since there's no other grade.
@jcsteh
Copy link
Contributor Author

@jcsteh 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"))

This comment has been minimized.

@feerrenrut

feerrenrut Jun 19, 2017
Contributor

This docstring should also have output and input

_suppressSpeakTypedCharactersTime = None
else:
suppress = False
if not suppress and config.conf["keyboard"]["speakTypedCharacters"] and ord(ch)>=32:

This comment has been minimized.

@feerrenrut

feerrenrut Jun 19, 2017
Contributor

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

This comment has been minimized.

@derekriemer

derekriemer Jun 19, 2017
Collaborator

update copyright year

@jcsteh jcsteh requested a review from feerrenrut Jun 20, 2017
jcsteh added a commit that referenced this pull request Jun 20, 2017
…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 Jun 21, 2017
@jcsteh
Copy link
Contributor Author

@jcsteh 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):

This comment has been minimized.

@feerrenrut

feerrenrut Jun 21, 2017
Contributor

Where is this called?

This comment has been minimized.

@feerrenrut

feerrenrut Jun 21, 2017
Contributor

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
…res dialog. Incubates #6449 (issue #2439).
@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Jun 28, 2017

fixes #6956

@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Jul 8, 2017

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants