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

Bengali Probhat layout #8887

Merged
merged 10 commits into from Mar 15, 2022
Merged

Bengali Probhat layout #8887

merged 10 commits into from Mar 15, 2022

Conversation

uroybd
Copy link
Contributor

@uroybd uroybd commented Mar 10, 2022

This PR contains codes for a popular Bengali keyboard layout named Probhat.

However, I don't really know how to register this in the app to be able to use this.


This change is Reviewable

@Frenzie Frenzie added this to the 2022.03 milestone Mar 10, 2022
@Frenzie
Copy link
Member

Frenzie commented Mar 10, 2022

We'll have a look, but I don't have time to right now. :-)

@poire-z
Copy link
Contributor

poire-z commented Mar 10, 2022

Just curious: do you get all your bengali glyphs displayed correctly?
From which of our shipped fonts do they come ?
Noto Sans Devanagari ? https://fonts.google.com/noto/specimen/Noto+Sans+Devanagari does not mention Bengali among its supported languages (if it is a language, might just be a script, not familiar at all with all those indic scripts&languages :).
Or from our FreeSans or FreeSerif, which has support for many scripts ? (but with possibly smaller/thiner glyphs).

@uroybd
Copy link
Contributor Author

uroybd commented Mar 10, 2022

Just curious: do you get all your bengali glyphs displayed correctly? From which of our shipped fonts do they come ? Noto Sans Devanagari ? https://fonts.google.com/noto/specimen/Noto+Sans+Devanagari does not mention Bengali among its supported languages (if it is a language, might just be a script, not familiar at all with all those indic scripts&languages :). Or from our FreeSans or FreeSerif, which has support for many scripts ? (but with possibly smaller/thiner glyphs).

No, Noto Sans Bengali is the font we need: https://fonts.google.com/noto/specimen/Noto+Sans+Bengali
It is standard in Android system for a long time though.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 10, 2022

Should I add Noto Sans Bengali In the fallback fonts?

@mergen3107
Copy link
Contributor

@uroybd
Welcome to the KOReader Keyboard Tinkering Club® :D

From your question thread:
What if I need more keys in the 3rd row?:
You can just add new keys as strings in table (a table starts with a single { and ends with its own },), but make sure to adjust the widths of other rows if necessary. For example have a look at Turkish layout.

Should I add Noto Sans Bengali In the fallback fonts?

Probably yes, because KOReader is supported not only on Androids.

@poire-z
Copy link
Contributor

poire-z commented Mar 10, 2022

Should I add Noto Sans Bengali In the fallback fonts?

Not yet.
You might have it on Android, but we won't have it on Kobo/Kindle/etc...
So, we might need to add the font itself to those shipped with KOReader.

But testing, it looks to me like we have support for Bengali in both FreeSans and FreeSerif:
FreeSans (used before FreeSerif, so you should get these glyphs on your keyboard):
image

FreeSerif:
image

So, that means your keyboard should at least show some stuff on Kobo and Kindle.

The question is: is that good looking and complete enough ?
(Shipping new fonts is not fun (needs some work), and increase the package size - so it depends on the user base - which I guess is quite huge for Bengali :)

@uroybd
Copy link
Contributor Author

uroybd commented Mar 10, 2022

The question is: is that good looking and complete enough ? (Shipping new fonts is not fun (needs some work), and increase the package size - so it depends on the user base - which I guess is quite huge for Bengali :)

Bengali speakers are virtually everywhere, tropical reproduction is to be blamed. ;)

I've added a PR to add font in the font repo koreader/koreader-fonts#16

@poire-z
Copy link
Contributor

poire-z commented Mar 10, 2022

You haven't answered if the FreeSans/FreeSerif are good enough or not.
We mostly add the fonts themselves when we have a UI language that needs it. For your case, your keyboard could be fine with just our FreeSans.
So, please convince us that shipping NotoSansBengali is really needed :)

(We have a Hebrew UI language, and we don't even ship a Hebrew font - FreeSans or FreeSerif seems fine enough.)

@uroybd
Copy link
Contributor Author

uroybd commented Mar 10, 2022

You haven't answered if the FreeSans/FreeSerif are good enough or not. We mostly add the fonts themselves when we have a UI language that needs it. For your case, your keyboard could be fine with just our FreeSans. So, please convince us that shipping NotoSansBengali is really needed :)

Sorry, missed that. It is not good enough. Bengali has a complex rendering system. In your examples, many conjunction glyphs are not rendering properly. I'm adding an example to show how it should be using a proper font:

Screenshot_2022-03-10-16-20-20-459_org koreader launcher

The rendering is not a system issue for kindle or kobo because I've seen users using these devices to read Bengali. So I believe, with proper fonts everything will be fine.

@poire-z
Copy link
Contributor

poire-z commented Mar 10, 2022

Btw, since #8722, if a user drops (or already has) a Noto Sans Bengali UI font in there, it will be automatically used as a fallback font.
So, shipping or not this font is a bit orthogonal to your keyboard.
We didn't ship any font for Myanmar, we just allowed users to help themselves.
Bengali user base is probably larger, so I let @NiLuJe & @Frenzie the last word on shipping or not the one you added in koreader/koreader-fonts#16.

@Frenzie
Copy link
Member

Frenzie commented Mar 10, 2022

I don't really recall the discussion around Myanmar but some 180 kB isn't the worst. That being said also compare that abandoned PR @pazos did a while back.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 10, 2022

Btw, since #8722, if a user drops (or already has) a Noto Sans Bengali UI font in there, it will be automatically used as a fallback font.
So, shipping or not this font is a bit orthogonal to your keyboard.
We didn't ship any font for Myanmar, we just allowed users to help themselves.
Bengali user base is probably larger, so I let @NiLuJe & @Frenzie the last word on shipping or not the one you added in koreader/koreader-fonts#16.

Shipping this font is not an absolute necessity. We have seen that the FreeSans and FreeSerif already provide the glyphs. We don't use complex Conjunction glyphs on the keyboard. They get generated automatically by the font. So, we don't need Noto Sans Bengali for the keyboard itself.

However, considering the userbase and file size, you can add this font. Like Devanagari, Bengali script is also being used by Asaamia and some other languages.

@NiLuJe
Copy link
Member

NiLuJe commented Mar 10, 2022

Yeah, Bengali probably makes sense, especially given the potential user base ;).

1. Fixed bug that was force closing the keyboard.
2. Fixed key sizes.
3. Added appropriate alt label.
4. Rename Locale label for 'bn' from 'বাঙালি' to 'বাংলা'.
@uroybd
Copy link
Contributor Author

uroybd commented Mar 14, 2022

I've setup a dev environment and fixed all the bugs related to this PR. Tested it for some hours. Everything seems to be fine. Tweaked the key sizes and alt labels.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

Is there anything else I can do before you can merge this?

@poire-z
Copy link
Contributor

poire-z commented Mar 15, 2022

Bump the koreader-fonts submodule, in this PR as it is quite related :)

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

Bump the koreader-fonts submodule, in this PR as it is quite related :)

Done. :)

@poire-z
Copy link
Contributor

poire-z commented Mar 15, 2022

Something yet to fix to avoid CI failures on later unrelated PRs:

Luacheck results
Checking frontend/ui/data/keyboardlayouts/bn_keyboard.lua 1 warning

    frontend/ui/data/keyboardlayouts/bn_keyboard.lua:138:7: unused variable 'zwj'

Total: 1 warning / 0 errors in 485 files

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

Should I Squash these commits for cleanliness?

@poire-z
Copy link
Contributor

poire-z commented Mar 15, 2022

Should I Squash these commits for cleanliness?

We will squash them when merging, so no need for you to do it (and we keep the history of the work in there with the individual commits). Unless you prefer to have more than one commit in what we merge if you think they deserved to be multiple individual unrelated ones ?

Still an issue with CI:

[ RUN      ] spec/front/unit/document_spec.lua @ 73: EPUB document module should register droid sans fallback
spec/front/unit/document_spec.lua:87: Expected objects to be the same.
Passed in:
(table: 0x7f00df4e4560) {
  [1] = 'Droid Sans Mono'
  [2] = 'FreeSans'
  [3] = 'FreeSerif'
  [4] = 'Noto Naskh Arabic'
  [5] = 'Noto Sans'
  [6] = 'Noto Sans Arabic UI'
 *[7] = 'Noto Sans Bengali UI'
  [8] = 'Noto Sans CJK SC'
  [9] = 'Noto Sans Devanagari UI'
  [10] = 'Noto Serif' }
Expected:
(table: 0x7f00df4d7b58) {
  [1] = 'Droid Sans Mono'
  [2] = 'FreeSans'
  [3] = 'FreeSerif'
  [4] = 'Noto Naskh Arabic'
  [5] = 'Noto Sans'
  [6] = 'Noto Sans Arabic UI'
 *[7] = 'Noto Sans CJK SC'
  [8] = 'Noto Sans Devanagari UI'
  [9] = 'Noto Sans Bengali UI'
  [10] = 'Noto Serif' }

stack traceback:
	spec/front/unit/document_spec.lua:87: in function <spec/front/unit/document_spec.lua:73>

[  FAILED  ] spec/front/unit/document_spec.lua @ 73: EPUB document module should register droid sans fallback (0.27 ms)

(Looks like there's some kind of alphabetical ordering somewhere, and you added Bengali after Devanagari like you did good in other places - but it should be before somewhere in there :)

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

Fixed the font ordering. :)

@poire-z
Copy link
Contributor

poire-z commented Mar 15, 2022

You changed more places than were needed (it was only need in spec/, for the test itself).

Dunno the differences in the amount of readers and content for Devanagari vs. Bengali, and which deserves to be first in the various lists. (If in doubt, let Bengali be polite and place itself after long-time-there Devangari :)
There's one place where you didn't change the order, frontend/ui/font.lua, where Devanagari is still before Bengali.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

I intended to do this in alphabetical order. I personally have no problem having Bengali after Devanagari.

I've added Bengali after Devanagari in all Lua files for consistency this time.

@uroybd
Copy link
Contributor Author

uroybd commented Mar 15, 2022

spec/front/unit/document_spec.lua:87: Expected objects to be the same.
Passed in:
(table: 0x7f6a80ebc6e8) {
  [1] = 'Droid Sans Mono'
  [2] = 'FreeSans'
  [3] = 'FreeSerif'
  [4] = 'Noto Naskh Arabic'
  [5] = 'Noto Sans'
  [6] = 'Noto Sans Arabic UI'
 *[7] = 'Noto Sans Bengali UI'
  [8] = 'Noto Sans CJK SC'
  [9] = 'Noto Sans Devanagari UI'
  [10] = 'Noto Serif' }
Expected:
(table: 0x7f6a80ed3508) {
  [1] = 'Droid Sans Mono'
  [2] = 'FreeSans'
  [3] = 'FreeSerif'
  [4] = 'Noto Naskh Arabic'
  [5] = 'Noto Sans'
  [6] = 'Noto Sans Arabic UI'
 *[7] = 'Noto Sans CJK SC'
  [8] = 'Noto Sans Devanagari UI'
  [9] = 'Noto Sans Bengali UI'
  [10] = 'Noto Serif' }

stack traceback:
	spec/front/unit/document_spec.lua:87: in function <spec/front/unit/document_spec.lua:73>

Now, this is weird. I've searched the codebase. Nowhere is Noto Sans Bengali UI except after Noto Sans Devanagari UI. Am I missing something?

@poire-z poire-z merged commit 357bc65 into koreader:master Mar 15, 2022
@Frenzie Frenzie linked an issue Mar 17, 2022 that may be closed by this pull request
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.

FR: Bengali Keyboard Layout
5 participants