Skip to content

Integrate basic braille viewer#10173

Merged
feerrenrut merged 45 commits into
masterfrom
i7788-integrateBrailleViewer
Dec 4, 2019
Merged

Integrate basic braille viewer#10173
feerrenrut merged 45 commits into
masterfrom
i7788-integrateBrailleViewer

Conversation

@feerrenrut
Copy link
Copy Markdown
Contributor

@feerrenrut feerrenrut commented Sep 8, 2019

Link to issue number:

#7788

Summary of the issue:

Having a braille viewer tool integrated into NVDA is quite helpful for developers who wish to test with braille but do not have a braille device. This tool may also be useful when two people are using a computer, one with braille and one sighted (for instance a parent, reading with their child)

Description of how this pull request fixes the issue:

Introduces a simple braille viewer tool.

Testing performed:

Tested permutations of:

  • Enable on startup true / false
  • Starting / exiting NVDA
  • Toggling the braille viewer from the tools menu.

As per #7788 (comment) tested an empty message no longer results in an error.

Not tested
Using the brailleViewer in conjunction with another display. I expect this to work, and the output should match.

Known issues with pull request:

Scrolling gestures (braille_scroll_forward, braille_scroll_back) must be set manually, there are no buttons in the GUI to do this. Scrolling with buttons on the GUI will be quite a lot of work and won't be covered in this PR.

Routing is not supported, for similar reasons to buttons on the GUI for scrolling.

  • User guide needs to be updated

Change log entry:

New features:

- Braille Viewer tool, allows viewing braille output via an on-screen window. (#7788)

feerrenrut and others added 28 commits December 6, 2017 13:44
Missing super call on AutoPropertyObject children no longer causes
failure for CachedGetter properties. Previously a class that inherits
from AutoPropertyObject and forgot to call super in the __init__
function, would result in a failure for CachedGetter properties.
…ewer

This implementation of brailleViewerTool relies on #7833 being fixed
Callbacks inform the dependents of brailleViewer of creation and
destruction.
- Introduce several more unit tests.
- Try to clarify intent of existing unit tests.
- Add logic to allow handlers to have "required parameters". These are
parameters with no default value set.
- Add logic to catch and raise an exception when:
  - The required parameters are not supplied (either through positional
  args or keyword args)
  - The handler does not accept enough positional args.
Merge remote-tracking branch 'origin/pr/8208' into i7788-integrateBrailleViewer
Creating multiple dialogs at the same time seems to cause a crash in wx
with the dialogs are destroyed.
Frame gives a taskbar icon, and an item in alt+tab menu.
The brailleViewer impl of the brailleDriver did not satisfy the requirements of the base class in lots of ways.
@LeonarddeR
Copy link
Copy Markdown
Collaborator

LeonarddeR commented Sep 9, 2019

Could you have a look at the extension points I created in the braille module as part of #9917? In short:

  • braille.handler.pre_writeCells could be used to update the braille viewer's contents
  • braille.handler.filter_displaySize can be used to set the display size when there is no display connected. Optionally, the braille viewer can control the number of cells that are send to a physical display if the physical display is too big for the contents of the braille viewer to fit on screen.

note that in its current form, the braille viewer is very likely to conflict with how braille is handled in NVDA Remote. Both would be able to work seamlessly with these extension points.

Comment thread source/braille.py
Comment thread source/braille.py Outdated
Comment thread source/braille.py Outdated
Comment thread source/brailleDisplayDrivers/noBraille.py Outdated
Comment thread source/brailleViewer/__init__.py Outdated
Comment thread source/brailleViewer/__init__.py
Comment thread source/braille.py
Comment thread source/config/configSpec.py Outdated
Comment thread source/gui/__init__.py Outdated
Comment thread user_docs/en/userGuide.t2t Outdated
@feerrenrut feerrenrut marked this pull request as ready for review September 13, 2019 14:16
Comment thread source/braille.py
Comment thread source/brailleViewer/__init__.py Outdated
Comment thread source/brailleViewer/brailleViewerGui.py
Comment thread source/brailleViewer/brailleViewerGui.py Outdated
Comment thread source/config/configSpec.py Outdated
Comment thread source/brailleViewer/brailleViewerGui.py
Copy link
Copy Markdown
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Apart from my comment about resizing the gui when going from no display with braille viewer (40 cells) to a display with 80 cells, this looks ok to me.

@DrSooom
Copy link
Copy Markdown

DrSooom commented Sep 20, 2019

Why do you don't use Consolas for both lines? Consolas is much easier to read instead of Courier New.

@feerrenrut feerrenrut self-assigned this Nov 1, 2019
@AppVeyorBot
Copy link
Copy Markdown

PR introduces Flake8 errors 😲

See test results for Failed build of commit f4c73faaef

@feerrenrut
Copy link
Copy Markdown
Contributor Author

Why do you don't use Consolas for both lines? Consolas is much easier to read instead of Courier New.

Mostly this came down to trying to find a font that would align the braille characters with the raw text characters. At the time Courier New was the closest match I could find. I have done some further investigation on this, I think that the braille was always falling back to using Segoe UI font. Instead, I have included a custom font, which has a monospace variation with Braille symbol support. This font, "Free mono", had some it's own issues with braille symbols, but being GPL3 I was able to modify and fix them. Because we are using the same monospace font for both the braille and the raw text, the widths of the characters will match, reducing the number of things that will make them out of alignment. There is still the problem of cells used to indicate numbers or capitals (excuse my ignorance of terminology here please), but I think these should be left as a problem to solve another day.

@LeonarddeR @JulienCochuyt I think I have fixed the issues with changing the number of cells now. If you are able to test this again please? I would like to merge this next week.

Copy link
Copy Markdown
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

I think overall, this looks ok.
Re alligning the braille dots with the text, this shouldn't be that difficult when using brailleToRawPos and rawToBraillePos properly. Makes sense to do that in a follow up pr, though

Comment thread source/gui/__init__.py Outdated
onBrailleViewerChangedState relies on the frame.sysTrayIcon member variable
which we are in the process of constructing. Instead, we can just set the
checked state at construction time based on whether the
brailleViewer is active rather than using the callback.
@feerrenrut feerrenrut merged commit 1863d23 into master Dec 4, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Dec 4, 2019
feerrenrut added a commit that referenced this pull request Dec 4, 2019
@feerrenrut feerrenrut deleted the i7788-integrateBrailleViewer branch January 17, 2020 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants