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

Adds new German braille tables #11268

Merged
merged 8 commits into from Jul 6, 2020

Conversation

aaclause
Copy link
Contributor

@aaclause aaclause commented Jun 17, 2020

Link to issue number:

Fixes #11263

Summary of the issue:

New German braille tables are missing in the GUI.

Description of how this pull request fixes the issue:

Adds entries for "de-g0-bidi.utb" and "de-g1-bidi.ctb".

Testing performed:

Ran from sources. Loaded all these tables.

Known issues with pull request:

None

Change log entry:

@aaclause
Copy link
Contributor Author

aaclause commented Jun 17, 2020

@Futyn-Maker I changed the label for "uk.utb" from "Ukrainian" to "Ukrainian Grade 1". Is it OK for you?

@Futyn-Maker
Copy link

Futyn-Maker commented Jun 17, 2020

@Andre9642 Yes, I think it's OK, although in Russia and Ukraine we prefer "literary braille", but it's not important.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 17, 2020

I am adding the author of that tables here, @BueVest should these tables favorized? Should DE-G0.utb and DE-G1.ctb be excluded from screen reader in favor of DE-G0-Bidi.utb and DE-G1-Bidi.ctb? I still see a comment in the newer tables that this is still experimental and should not be included in the core files yet.
Also there is still a comment in de-g0-bidi-core.uti which says that a certain line should be removed as soon as liblouis/liblouis#717
is solved. That issue is now solved, so are these german tables now complete?

@feerrenrut feerrenrut requested a review from leonardder Jun 18, 2020
Copy link
Collaborator

@leonardder leonardder left a comment

from #11263 (comment)

I'm a bit confused about those new German tables. How experimental are they really?

note that we once choose to replace en-us-comp8.ctb by en-us-comp8-ext.utb because the latter was more complete and extended on the former, so it wouldn't do any harm when it was replaced.

What are the intentions of these German tables? Are they meant to replace the old tables? Are their file names persistent, i.e. meant to stay even when the tables are no longer experimental?

@leonardder
Copy link
Collaborator

leonardder commented Jun 18, 2020

Additional details in #11263

As we are very close to 2020.2, I think I prefer the german tables to be left out of this.

@feerrenrut
Copy link
Member

feerrenrut commented Jun 18, 2020

@leonardder What about the added Ukrainian table, would it be safe to add for 2020.2?

@Futyn-Maker
Copy link

Futyn-Maker commented Jun 18, 2020

@feerrenrut There isn't any oficial standards of Ukrainian computer braille, so, we've based in on Russian computer braille set with some changings of punctuations. I believe that it's better than nothing...

@leonardder
Copy link
Collaborator

leonardder commented Jun 18, 2020

I"m ok with the Ukrainian ones.

There's still some debate going on about the German tables, see liblouis/liblouis#911. @BueVest owns them.

@BueVest
Copy link
Contributor

BueVest commented Jun 18, 2020

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 18, 2020

@BueVest so do you see the bidirectional G0 and G1 tables as stable enough to be included in NVDA? Or would you advice to wait until the work on G0 and G1 tables is finished? To me they looked prety stable but you might have tested them already also in comparison with the forward translation G0 and G1 tables.

@feerrenrut
Copy link
Member

feerrenrut commented Jun 19, 2020

Given the amount of discussion here about the German braille tables, I think it is best to open a new PR just for the Ukrainian tables, and allow the discussion to continue here. The new PR (for the Ukrainian tables) should target the beta branch please.

@Futyn-Maker
Copy link

Futyn-Maker commented Jun 19, 2020

@feerrenrut What problems have You found about Ukrainian tables? The new Ukrainian computer braille table based on Russian computer braille code and now works absolutely correct. Personally I don't see any sense to continue discussions about Ukrainian tables...

Andrey

@feerrenrut
Copy link
Member

feerrenrut commented Jun 19, 2020

Not discussion, but if they are to be included in 2020.2 then a new PR must be created. This PR is mostly talking about the German tables, it might as well stay that way.

Currently the changes in this PR are for the Ukrainian tables, but the description and discussion is about Germain tables.

@BueVest
Copy link
Contributor

BueVest commented Jun 19, 2020

Copy link
Member

@feerrenrut feerrenrut left a comment

This PR doesn't include the German tables, @Andre9642 could you please re-introduce those?

@aaclause aaclause changed the title Adds missing braille tables Adds new German braille tables Jun 19, 2020
@BueVest
Copy link
Contributor

BueVest commented Jun 19, 2020

Perhaps, you should also disable the old german tables as input tables. If you check the metadata of those tables, it says "direction: forward", meaning that the tables should only be used for forward translation and not back-translation, i.e. Braille input.
'Just a suggestion.

@BueVest
Copy link
Contributor

BueVest commented Jun 19, 2020

Sorry, of course, this only applies to the grade 0, 1 and 2 tables (de-g0.utb, de-g1.ctb and de-g2.ctb), not the 6 and 8 dots computer Braille tables.

@feerrenrut feerrenrut requested a review from leonardder Jun 23, 2020
Copy link
Member

@feerrenrut feerrenrut left a comment

I think that this looks ok (thanks @Andre9642!) but I would like to get the opinion of @leonardder since he is much more familiar with braille and libluis.

Copy link
Collaborator

@leonardder leonardder left a comment

I agree with @BueVest that it makes sense to disable input for the old German tables if it's really that broken as he describes. See below.

I see why we want these tables integrated and that there's real demand for it, but I'm still a bit worried about the naming, (improved back-translation, experimental). Looking at it from a user perspective:

  1. Does a user understand what's meant with back-translation? May be input fits better here, as they are already familiar with that.
  2. If only back translation was improved, I'd say let's kick out the old German tables and replace those by these. However, it seems that forward translation is also changed with regard to accented letters, according to liblouis/liblouis#911 (comment). I really dig the idea to call these tables detailed as introduced by @bertfrees in liblouis/liblouis#911 (comment)

Altogether, the discussion in liblouis/liblouis#911 really makes me believe that many things about these tables are not set in stone. If we had custom braille tables, I'd probably be against having them in core like this.

I am aware of the fact that my review isn't at all helpful in deciding what to do with this pr. May be @michaelDCurran can also spread his light on this?

source/brailleTables.py Outdated Show resolved Hide resolved
source/brailleTables.py Outdated Show resolved Hide resolved
source/brailleTables.py Outdated Show resolved Hide resolved
@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jun 23, 2020

I believe in the past we were adding new braille tables into the GUI only if someone has requested them to be added - as far as I see there was no such request in this case.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 23, 2020

@leonardder the changes in forward translation are related to tilde, dash and some other symbols which in the old german table cause some interpolations with other characters. It seems this tables are improving it and even though it is experimental, I would say there is nothing in there which would break the current user experience. Imo the only way to make a braille table really robust is extensive user testing, different braille displays etc. Thus, I vote for these tables to be included into NVDA, at least in the alpha version for a while. If it's not working well for users, then they could be replaced back by the old tables. But since @BueVest is the creator of these tables, I think he tested them of course and his statement here should have enough weight.

Regarding NVDA's Gui, I would vote for introducing a new combo box in the braille settings called "bidirectional tables" and include there all stable braille tables which allow both input and output. If the meaning of bidirectional tables is documented properly in the user guide, I don't see any significant confusion caused by this.

@leonardder
Copy link
Collaborator

leonardder commented Jun 23, 2020

I would vote for introducing a new combo box in the braille settings called "bidirectional tables" and include there all stable braille tables which allow both input and output.

How would this fit in the UX? I'm afraid this would add even more confusion.

I believe in the past we were adding new braille tables into the GUI only if someone has requested them to be added - as far as I see there was no such request in this case.

I think this applies to most cases, yes.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 23, 2020

aaclause and others added 3 commits Jun 23, 2020
Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 23, 2020

@BueVest to which extent does solve your tables the following issues?
liblouis/liblouis#363
and
liblouis/liblouis#597

@BueVest
Copy link
Contributor

BueVest commented Jun 23, 2020

@BueVest
Copy link
Contributor

BueVest commented Jun 23, 2020

@bertfrees
Copy link

bertfrees commented Jun 23, 2020

Chiming in... Bue, I don't think Leonard was talking about file names. Files names are clearly also an issue, but a unrelated one.

For the user interface, I recommend using as much as possible what is in the "display-name" (and/or "index-name") metadata of the Liblouis tables. This field was created specifically to be used in applications like NVDA, and to create some consistency across applications. I can't stress enough how much I would appreciate it if you would use our metadata instead of your own names.

But anyway, I'm digressing...

The naming of the German tables in question, and the corresponding Danish tables that Bue mentioned, is indeed an issue that we are aware of. Their names should indeed be harmonized, and the concerns that Leonard has about the "back-translatable" are right. I think the most likely scenario at this point is that they'll be labelled "detailed" in the next version of Liblouis.

@leonardder
Copy link
Collaborator

leonardder commented Jun 24, 2020

I can't stress enough how much I would appreciate it if you would use our metadata instead of your own names.

I see why that's preferred. Doing this automatically is a bit difficult though, as we want the table descriptions to be translatable.

leonardder
leonardder previously requested changes Jun 24, 2020
Copy link
Collaborator

@leonardder leonardder left a comment

Alright, let's crack the nut. I'm ok with this if the following two changes could be applied with regard to naming, if @BueVest also agrees.

source/brailleTables.py Outdated Show resolved Hide resolved
source/brailleTables.py Outdated Show resolved Hide resolved
@bertfrees
Copy link

bertfrees commented Jun 24, 2020

Doing this automatically is a bit difficult though, as we want the table descriptions to be translatable.

Yes, I suspected that was the case. It doesn't need to be automatic though.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 24, 2020

@BueVest as regarding your thoughts about grade 2 tables, they are out of scope for this PR. So you could intermediarily edit the current grade 2 table to solve it, but I would say it's not a high priority. If the bidirectional grade 2 table is created, they this might be considered.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 24, 2020

One further thing that I think it is important to discuss, if this table is bidirectional but it is displayed under output tables in braille settings, this could create confusions.
@leonardder do you have a suggestion how this could be solved? I mean if an additional combo box for bidirectional tables is not a solution here? There should be at least the possibility to choose "no input braille table" in the input braille table combo box in NVDA's braille settings. Otherwise the backward translation from bidirectional braille table and from a chosen input braille table could interfere here which could have some issues. Or am I wrong?

feerrenrut and others added 2 commits Jul 6, 2020
See review actions nvaccess#11268 (review)

Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
See review actions nvaccess#11268 (review)

Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
@feerrenrut
Copy link
Member

feerrenrut commented Jul 6, 2020

I haven't seen any opposition to the suggestions made by @leonardder, so I will accept them and merge this PR.

@feerrenrut feerrenrut dismissed leonardder’s stale review Jul 6, 2020

Changes have been made.

@feerrenrut feerrenrut added component/braille component/liblouis Issues related to liblouis, such as liblouis updates and braille table additions/changes labels Jul 6, 2020
@feerrenrut feerrenrut merged commit a426c5a into nvaccess:master Jul 6, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.3 milestone Jul 6, 2020
@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 8, 2020

@BueVest does it make sense to keep the older german output tables for Basisschrift (g0I and Vollschrift (g1)? Or should these be also removed? I am asking because the coresponding input tables are also replaced now by your bidirectional tables but the old output tables are still in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/braille component/liblouis Issues related to liblouis, such as liblouis updates and braille table additions/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing braille tables in the Braille NVDA Settings
9 participants