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

Add Korean keyboard (2-beolsik) #5053

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
3 participants
@limerainne
Copy link
Contributor

commented Jun 1, 2019

Dear KOReader developers,

I want to add Korean keyboard (in 2-beolsik; most popular and somewhat simple to implement) that I implemented, regarding many references on the web, to the KOReader.

Here is the screenshot of the keyboard:
KR

and here is some working example:
Typing with Korean keyboard

Because Korean characters, called Hangul, have to be typed with combinations of some consonnants and vowels, hence Korean keyboard requires some automata code to do those combinations. It seems that keyboards implemented in KOReader did not required automata things, I did not found handler functions required for processing events with automata code.

Therefore, I made those changes that are not matching your code quality standard:

  • I don't know adequete location, so I placed automata code in the same directory of keyboard data.
  • To fetch keyboard event in my custom event handler code, I appended some code to virtualkeyboard.lua that:
    • Hooked addChars() and delChar() functions, and if Korean characters are typed, then they will be handled with Korean automata
    • Other events to reset Korean automata

Please consider my request, and please modify it if its code style, etc. was not appropriate..

Thank you!

@Frenzie

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I think it might be clearer to create a new directory named something like keyboardhandler for the actual code? I don't have time to look at it much this weekend; pinging @poire-z

@Frenzie
Copy link
Member

left a comment

Thanks! It looks nice; pity I don't speak Korean. :-)

-- Hangul Syllables --
----------------------

HgSylbls = {

This comment has been minimized.

Copy link
@Frenzie

Frenzie Jun 1, 2019

Member

Should be local. That should also fix the bulk of the luacheck errors.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

You type super fast! :)

No issue with your code quality, it's rather clean and readable.

I think it might be clearer to create a new directory named something like keyboardhandler for the actual code?

Either that (but when it will contain a single file, it's a bit unwelcome), or rename ko_KR_HgKit.lua to something that will visually talk to other developers, like:

ko_KR_helper.lua
ko_KR_keyboard.lua

To fetch keyboard event in my custom event handler code, I appended some code to virtualkeyboard.lua

Given that there are only 8 VirtualKeyboard small methods that just wrap calls to self.inputbox (a InputText object) methods, may be you can even avoid modifying them by just wrapping self.inputbox in an object defined in your ko_KR_HgKit.lua, and in VirtualKeyboard:init() just have:

if keyboard.wrapInputBox then
    self.inputbox = keyboard.wrapInputBox(self.inputbox)
end

(and may be only reset the previous state of it in that wrapInputBox() function when a new one is created, and let VirtualKeyboard:onCloseWidget() untouched?).
And have in your ko_KR files something like:

local InputBoxWrapper = {
    real_inputbox = nil
}
function InputBoxWrapper:addChar(key)
  -- do your state stuff
  self.real_inputbox.addChar(your_composed_key)
end
function InputBoxWrapper:upLine(key)
  self.real_inputbox:upLine()
  _resetStateCustomHandler()
end

and in the big return { have that wrapping function in it:

    wrapInputBox = function(inputbox)
        _resetStateCustomHandler() -- reset previous state
        return InputBoxWrapper:new({
            real_inputbox = inputbox,
        })
    end

That would leave VirtualKeyboard quite untouched with no notion of that state specific to the korean keyboard.
But if it's not as easy as I envisioned above, I'm fine with this PR as it is.

@limerainne limerainne force-pushed the limerainne:virkbd_korean branch 2 times, most recently from ef44d3b to 1878cb4 Jun 2, 2019

@limerainne

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

First of all, I should have run luacheck before making PR 😿

I've fixed PR commit from previous one, details are:

  • Renamed the helper classes file to ko_KR_helper.lua
    For now, as @poire-z said, creating dedicated directory (for Korean keyboard helper only) would be overwork. But in the future with more keyboards implemented which require helper classes, there might be a chance to create dedicated directory for helper classes :)

  • Fixed lint warnings/errors from luacheck
    As @Frenzie told, I put local keyword to local variables to fix too much error messages 😃 Also, there was a typo: False 😅

  • Minimized changes to VirtualKeyboard, and made a InputBox wrapper in Korean keyboard code
    As @Frenzie told, I placed code for wrapping InputBox only in VirtualKeyboard. The inputbox handler was moved to keyboard code.

    Unfortunately, I couldn't simplify or reduce target functions to hook for keyboard event. There are 12 (+4 from previous version) hooks exist :(

    If an user moves cursor in the input box (or making other events), previous states about combining single character has to be dropped (or resetting helper state), for avoiding previous character popping out. I thought that resetting state is required when these events are fired:

    • Moving cursors with arrow keys, touching screen, etc.
    • Delete single line or whole box
    • Closing input box (➡️ As @poire-z said, this event can be substituted with VirtualKeyboard:init() call.)

    But I couldn't find useful functions used commonly in above events, hence I had to wrap each event handlers. I think that other contributers in the future could fix these inefficiencies 😅

Please consider revised PR. Thank you!

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Thanks!

hence I had to wrap each event handlers. I think that other contributers in the future could fix these inefficiencies sweat_smile

Well, that looks quite ok to me (some hardcore Lua hacker could probably achieve something similar by tweaking metatable stuff).

Just one thing, as it seems you are not replacing the original inputbox (as I suggested) but just wrapping its interesting methods (which is better!), you could just, for clarity, dont return inputbox and just use in virtualkeyboard.lua:

     if keyboard.wrapInputBox then
-        self.inputbox = keyboard.wrapInputBox(self.inputbox)
+        keyboard.wrapInputBox(self.inputbox)
     end

(that way, we know we're not getting a new object, and don't have to wonder if there's stuff to do in the Korean keyboard code when we think about that inputbox)

@limerainne limerainne force-pushed the limerainne:virkbd_korean branch from 1878cb4 to 404fbf4 Jun 2, 2019

@poire-z

poire-z approved these changes Jun 2, 2019

@limerainne

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

Ah, thank you for approving this PR, @poire-z!

@Frenzie Frenzie merged commit 53b6e3d into koreader:master Jun 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie added this to the 2019.06 milestone Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.