keyboards icon indicating copy to clipboard operation
keyboards copied to clipboard

[karambolpoular] Poular arab

Open sven-oly opened this issue 1 year ago • 3 comments

sven-oly avatar Oct 09 '24 05:10 sven-oly

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

keyman-server avatar Oct 09 '24 05:10 keyman-server

@sven-oly Thanks for the keyboard submission. I'm glad you were able to help the author with this. There are a few issues with this commit.

  • One is that with our updating our system to using Keyman 17 format, we will need you to update to Keyman Developer 17. This is because the .kps file needs to have a description (used to be in the keyboard_info file). Also, the LICENSE.md must be included in the .kps and then referenced in the Details tab.

  • Another issue is that there are a lot of files that should be removed from the PR:

    • build_targets_temp.txt (this is in your root folder below experimental. It's not needed for the keyboard).
    • karambolpoular-codes.js (and remove from .kmn - it seems to be an empty file)
    • karambolpoular-codes.txt (and remove from .kmn - it seems to be an empty file)
    • karambolpoular-help.htm (and remove from .kmn - it seems to be an empty file)
    • karambolpoular.css (and remove from .kmn - it seems to be an empty file)
    • karambolpoular.js (This is the compiled touch layout and we don't include files that get built from source)
    • karambolpoular.kvks (This is the compiled OSK and we don't include files that get built from source)
    • karambolpoular.kmx (This is the compiled kmn and we don't include files that get built from source)
    • karambolpoular.kmp (This is the compiled package and we don't include files that get built from source)

With the above changes, in the .kmn you would remove these header statements:

store(&KMW_EMBEDJS) 'karambolpoular-code.js'
store(&KMW_EMBEDCSS) 'karambolpoular.css'
store(&KMW_HELPFILE) 'karambolpoular-help.htm'
store(&INCLUDECODES) 'karambolpoular-codes.txt'
  • Also, to make it easier to read, I'd change the targets statement to any instead of listing them all. That's not a requirement, just a suggestion!
  • The welcome.htm file doesn't include any graphics to help the user know how to type. That would be nice to add, but since you are putting this in experimental, it's not a requirement. If you don't have time to add graphics perhaps you could just indicate that the On-Screen keyboard will demonstrate to the user how to type.

LornaSIL avatar Oct 09 '24 14:10 LornaSIL

@sven-oly are you able to make these changes or do you want me to make the changes? Since these are primarily structural issues I can fix it up without modifying the keyboard itself if you wish.

LornaSIL avatar Oct 16 '24 14:10 LornaSIL

We need to verify the language and the BCP 47 code for this keyboard. fuc (Pulaar) and fuf (Pular) are both spoken in Guinea (GN). The former is largely in Senegal with a few speakers in Guinea near the border with Senegal. The latter has 4.3 milliion. Currently fuc is the representative language for the Fulfulde (Fulah) macrolanguage code ff, so ff should be used instead of fuc.

DavidLRowe avatar Oct 30 '24 22:10 DavidLRowe

Thank you, Lorna, for your helpful comments and notes. I'm working on this PR to update it as suggested.

David, I'm changing the language code as suggested.

sven-oly avatar Nov 04 '24 00:11 sven-oly

Thanks for your patience. Please take another look.

sven-oly avatar Nov 04 '24 00:11 sven-oly

I definitely made a mistake in one of my comments above. I'm sorry. You have committed the .kvk and not the .kvks file. It is karambolpoular.kvks that is the source and should be committed and karambolpoular.kvk should not be committed.

One other issue is that there is no description in the .kps file. The description (I think it's in the details tab) is used for the home page of the keyboard. You could just use the same thing you have in the README.md:

clavier poular du fouta djalon. caractères poulaar et arabe.

It doesn't have to be long, or you can include everything in the readme.htm file if you wish.

Otherwise, it looks like it is in good shape!

LornaSIL avatar Nov 04 '24 16:11 LornaSIL

Updated as suggested. Thank you!

sven-oly avatar Nov 05 '24 06:11 sven-oly

Thanks, Lorna, for these updates. Anything else to do at this time?

sven-oly avatar Nov 05 '24 17:11 sven-oly

It won't build, but based on the message, I think it's a problem with our Keyman build system and not your keyboard. So, I guess it has to wait until they fix that.

@mcdurdin

karambolpoular.kpj - fatal KM05001: Unexpected exception: RangeError: Invalid time value
16:15:52   
16:15:52       This error has been automatically reported to the Keyman team.
16:15:52   Call stack:
16:15:52         Identifier:  ada9d440ced142d3986773767d1da821
16:15:52   RangeError: Invalid time value
16:15:52         Application: Keyman Developer
16:15:52       at Date.toISOString (<anonymous>)
16:15:52         Reported at: https://sentry.io/organizations/keyman/projects/keyman-developer/events/ada9d440ced142d3986773767d1da821/
16:15:52       at getLastGitCommitDate (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/util/getLastGitCommitDate.js:37:27)
16:15:52       at BuildKeyboardInfo.build (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/buildClasses/BuildKeyboardInfo.js:34:32)
16:15:52       at ProjectBuilder.buildTarget (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/buildClasses/BuildProject.js:103:37)
16:15:52       at ProjectBuilder.buildProjectTargets (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/buildClasses/BuildProject.js:84:31)
16:15:52       at ProjectBuilder.run (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/buildClasses/BuildProject.js:76:30)
16:15:52       at async build (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/build.js:138:22)
16:15:52       at async Command.buildFile (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/commands/build.js:92:14)
16:15:52       at async Command.parseAsync (C:\BuildAgent\work\aee9ab97cd908689\keyboards\node_modules\commander\lib\command.js:935:5)
16:15:52       at async run (file:///C:/BuildAgent/work/aee9ab97cd908689/keyboards/node_modules/@keymanapp/kmc/build/src/kmc.js:55:5)
16:15:53   [/c/BuildAgent/work/aee9ab97cd908689/keyboards] ## build failed

LornaSIL avatar Nov 05 '24 18:11 LornaSIL

Tracking this build failure in keymanapp/keyman#12626

mcdurdin avatar Nov 05 '24 22:11 mcdurdin

Fix in keymanapp/keyman#12627 and keymanapp/keyman#12628. In order for this compiler fix to be available on this PR, we'll need to:

  1. Approve and merge those PRs
  2. Make a 17.0 stable release
  3. Deploy the 17.0.332 (tentative release number) kmc compiler release to the keyboards repo (which means another PR on keyboards that needs to be approved and merged)
  4. Merge master into this PR in order to pick up the new compiler

Please bear with us as we go through these steps.

mcdurdin avatar Nov 05 '24 23:11 mcdurdin

Please bear with us as we go through these steps.

@mcdurdin So, you'll let me know when to check again?

LornaSIL avatar Nov 06 '24 17:11 LornaSIL

I've just opened PR #3182 which does the upgrade to 17.0.332, and also cherry-picked that change into this PR. All going well, this PR should be safe to merge after #3182 is merged.

mcdurdin avatar Nov 06 '24 22:11 mcdurdin

@sven-oly There's an ongoing discussion in CLDR about language codes for the various languages in the Fulfulde continuum (see CLDR-18071). It's not clear to me whether this is for Pulaar (ISO 639-3 code "fuc", which is also the representative language for the macrolanguage code "ff") spoken in Senegal with a bit of spillover into Guinea, or Pular (ISO 639-3 code "fuf") spoken in Guinea, with spillover into Senegal and other countries. Would it be appropriate to include the fuf-Arab code in addition to the existing ff-Arab code (equivalent to fuc-Arab)? Or perhaps it's better to stick with ff-Arab for now and add any additional codes later as things become clearer.

DavidLRowe avatar Nov 06 '24 23:11 DavidLRowe

Thanks, everyone, for the help with this PR. I'm hoping that we can resolve this soon.

Re: the language, I would be happy with "ff-Arab" rather than a region-specific tag.

Is there anything else needed?

Another question: I've submitted this PR into the "experimental". What does this mean re: availability to anyone? And how does a keyboard move from "experimentat" to "normal"?

On Wed, Nov 6, 2024 at 3:13 PM DavidLRowe @.***> wrote:

@sven-oly https://github.com/sven-oly There's an ongoing discussion in CLDR about language codes for the various languages in the Fulfulde continuum (see CLDR-18071 https://unicode-org.atlassian.net/browse/CLDR-18071). It's not clear to me whether this is for Pulaar (ISO 639-3 code "fuc", which is also the representative language for the macrolanguage code "ff") spoken in Senegal with a bit of spillover into Guinea, or Pular (ISO 639-3 code "fuf") spoken in Guinea, with spillover into Senegal and other countries. Would it be appropriate to include the fuf-Arab code in addition to the existing ff-Arab code (equivalent to fuc-Arab)? Or perhaps it's better to stick with ff-Arab for now and add any additional codes later as things become clearer.

— Reply to this email directly, view it on GitHub https://github.com/keymanapp/keyboards/pull/3143#issuecomment-2460980553, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSA2EVHCS2C6ZOC2N2Q2KDZ7KPB5AVCNFSM6AAAAABPTWESUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRQHE4DANJVGM . You are receiving this because you were mentioned.Message ID: @.***>

sven-oly avatar Nov 14 '24 21:11 sven-oly

I just deleted two files in the root of keyboards that you had changed and they shouldn't have been changed. I shouldn't have deleted them, and I don't know how to add them back now. Are you able to add them back into the Pull Request? They are package.json and package-lock.json. Otherwise, we probably need a new PR which doesn't touch those two files. If you are comfortable fixing this issue, you can, otherwise I can submit the files for you.

As far as experimental vs release there are two issues to think about:

  • If you don't want to spend time documenting the keyboard, we will accept it into the repo as it is now. In this case, welcome.htm would need to be enhanced with graphics or some description of how to use the keyboard and we need a help file to be added: experimental/k/karambolpoular/source/help/karambolpoular.php. If you were to do these two things we would be okay with it going into release rather than experimental.
  • The other issue to think about is that we will not delete or rename (foldername/filename) any keyboards from release, but in experimental if you wanted to rename karambolpoular to karambol_poular or something completely different, we wouldn't be opposed.

LornaSIL avatar Nov 14 '24 22:11 LornaSIL

I just deleted two files in the root of keyboards that you had changed and they shouldn't have been changed. I shouldn't have deleted them, and I don't know how to add them back now. Are you able to add them back into the Pull Request? They are package.json and package-lock.json. Otherwise, we probably need a new PR which doesn't touch those two files. If you are comfortable fixing this issue, you can, otherwise I can submit the files for you.

I have reverted those two commits, and brought this branch up to date with master branch:

gh pr checkout 3143
git revert 44355b99466ef35b46af2d04187b853d7baf90c8 1758649a688fe3a13e883b47cae9250b1ad1f868
git push

The reason package.json has been updated is that we need to use v17.0.332 of kmc to build this package, due to a bug fix in the compiler. However, I am currently waiting on #3182 to be approved. Once that is approved and merged, we can update this branch and that will mean package.json (and package-lock.json) will no longer be impacted. See my earlier comment on this PR:

@mcdurdin wrote in https://github.com/keymanapp/keyboards/pull/3143#issuecomment-2460924717:

I've just opened PR https://github.com/keymanapp/keyboards/pull/3182 which does the upgrade to 17.0.332, and also cherry-picked that change into this PR. All going well, this PR should be safe to merge after https://github.com/keymanapp/keyboards/pull/3182 is merged.

mcdurdin avatar Nov 14 '24 23:11 mcdurdin

OK, have completed the merge of PR #3182 and so this PR should be ready for its build and check.

mcdurdin avatar Nov 15 '24 02:11 mcdurdin

Keyboard is now online: https://keyman.com/keyboards/karambolpoular

LornaSIL avatar Nov 15 '24 19:11 LornaSIL