Added Perso Arabic and Khudabadi Sindhi keyboards - #862
Siddharth-Talreja wants to merge 8 commits into
Conversation
This file defines a new keyboard layout for Sindhi Perso Arabic, including key mappings and metadata.
srish
left a comment
srish
left a comment
There was a problem hiding this comment.
Thanks a lot @Siddharth-Talreja for this PR! I've left suggestions for minor code changes inline. The main thing is that we are missing test cases for the two layouts in jquery.ime.test.fixtures.js and registration of the layouts in jquery.ime.inputmethods.js.
| [ 's', 'س' ], | ||
| [ 'D', 'ڊ' ], | ||
| [ 'd', 'د' ], | ||
| [ 'F', 'ڦ' ], |
There was a problem hiding this comment.
[ 'P', 'ڦ' ] (L57) and [ 'F', 'ڦ' ] (L71) are the only duplicated output in the layout, while lowercase p→پ and f→ف are distinct.
There was a problem hiding this comment.
This is based on the standard layout and duplicacies are also maintained as per the standard layout being used
srish
left a comment
srish
left a comment
There was a problem hiding this comment.
@Siddharth-Talreja Thanks for making the suggested changes. I've left two more comments inline. Most importantly we need to add test cases for the two layouts in jquery.ime.test.fixtures.js and registration of the layouts in jquery.ime.inputmethods.js.
| @@ -0,0 +1,110 @@ | |||
| ( function ( $ ) { | |||
| 'use strict'; | |||
| var sdpersoarabic = { | |||
There was a problem hiding this comment.
Variable name needs to be same as on line 109
| [ 'd', 'د' ], | ||
| [ 'F', 'ڦ' ], | ||
| [ 'f', 'ف' ], | ||
| [ 'G', 'گه' ], |
There was a problem hiding this comment.
AI is flagging errors in these lines for me. It says:
Lines 73 and 77 use ه for the aspirated letters, but the h key on line 76 gives ھ.
That means g+h and shift+G produce different text that won't match in search.
Would it make sense to change گه → گھ and جه → جھ so they're consistent?
@srish Thank You for the feedback. I wanted to know, for the test cases, do I need to just add test for individual mappings or any randomised string? And in case of mappings, are all key mappings required? |
@Siddharth-Talreja: @ToluAyodele has added some more changes to your PR. We still have some open questions. Could you maybe reply to them there #863? |
3 participants
This is connection to Phabricator Task https://phabricator.wikimedia.org/T428126
While, the task is only for Khudabadi script, I have also added perso arabic keyboard as it was missing